Skip to content
69 changes: 69 additions & 0 deletions src/Controls/tests/TestCases.HostApp/Issues/Issue36298.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
namespace Maui.Controls.Sample.Issues;

[Issue(IssueTracker.Github, 36298, "[Windows] ContentPresenter throws ArgumentException when dynamically switching RefreshView or ScrollView content.", PlatformAffected.UWP)]
public class Issue36298 : ContentPage
{
ContentView _contentHolder;
View _view1;
View _view2;

public Issue36298()
{
_view1 = new ContentView
{
Content = new RefreshView
{
Content = new Label { Text = "View 1 - RefreshView" }
}
};

_view2 = new ContentView
{
Content = new ScrollView
{
Content = new Label { Text = "View 2 - ScrollView" }
}
};

_contentHolder = new ContentView

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Regression test coverage — The regression title and production fix target ContentPresenter/IContentView reparenting, but the sample host here is a ContentView. That exercises a different setup than the reported ContentPresenter/templated-content scenario, and the direct ContentView swap can succeed without proving the ContentPresenter crash is fixed. Please make the repro use a ContentPresenter (for example via a control template) and verify that it fails on the old handler code and passes with this fix.

{
Content = _view1
};

var switchToView2Button = new Button
{
Text = "Switch to View 2",
AutomationId = "SwitchToView2"
};
switchToView2Button.Clicked += (_, _) => _contentHolder.Content = _view2;

var switchToView1Button = new Button
{
Text = "Switch to View 1",
AutomationId = "SwitchToView1"
};
switchToView1Button.Clicked += (_, _) => _contentHolder.Content = _view1;

var successLabel = new Label
{
Text = "Waiting",
AutomationId = "SuccessLabel"
};

switchToView2Button.Clicked += (_, _) => successLabel.Text = "View2";
switchToView1Button.Clicked += (_, _) => successLabel.Text = "Success";

Content = new VerticalStackLayout
{
Spacing = 10,
Padding = new Thickness(20),
Children =
{
switchToView2Button,
switchToView1Button,
_contentHolder,
successLabel
}
};
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
using NUnit.Framework;
using UITest.Appium;
using UITest.Core;

namespace Microsoft.Maui.TestCases.Tests.Issues;

public class Issue36298 : _IssuesUITest
{
public Issue36298(TestDevice device) : base(device) { }

public override string Issue => "[Windows] ContentPresenter throws ArgumentException when dynamically switching RefreshView or ScrollView content.";

[Test]
[Category(UITestCategories.Layout)]
public void SwitchingContentPresenterContentShouldNotCrash()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Regression Prevention and Test Coverage — This PR changes both ContentViewHandler.Windows.cs and BorderHandler.Windows.cs, but the added UI test only exercises the ContentView path (via _contentHolder in the HostApp page); it never places the switched content inside a Border. The near-duplicate fix in BorderHandler.Windows.cs therefore ships with zero regression-test coverage — add a Border-based scenario (or parameterize this test) so a regression in the Border code path would actually be caught.

{
// Wait for the page to load showing View 1
App.WaitForElement("SwitchToView2");

// Switch to View 2
App.Tap("SwitchToView2");
App.WaitForElement("SwitchToView1");

Comment on lines +20 to +23
// Switch back to View 1 — this triggered the ArgumentException before the fix
Comment thread
devanathan-vaithiyanathan marked this conversation as resolved.
App.Tap("SwitchToView1");
App.Tap("SwitchToView2");
App.Tap("SwitchToView1");

// Label is updated to "Success" only if the switch completed without crashing.
// WaitForElement("SuccessLabel") alone is insufficient because the label
// is present from page load; we must verify the text was actually updated.
Assert.That(App.WaitForTextToBePresentInElement("SuccessLabel", "Success"), Is.True);
}
}
33 changes: 29 additions & 4 deletions src/Core/src/Handlers/Border/BorderHandler.Windows.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using Microsoft.UI.Xaml;

namespace Microsoft.Maui.Handlers
{
Expand All @@ -20,14 +21,38 @@ static partial void UpdateContent(IBorderHandler handler)
_ = handler.VirtualView ?? throw new InvalidOperationException($"{nameof(VirtualView)} should have been set by base class.");
_ = handler.MauiContext ?? throw new InvalidOperationException($"{nameof(MauiContext)} should have been set by base class.");

handler.PlatformView.CachedChildren.Clear();
handler.PlatformView.EnsureBorderPath();

if (handler.VirtualView.PresentedContent is IView view)
{
// Detach the old handler if it exists (prevents WinUI COM exception on reuse)
view.Handler?.DisconnectHandler();
handler.PlatformView.Content = view.ToPlatform(handler.MauiContext);
var platformView = view.ToPlatform(handler.MauiContext);

// Detach from existing parent — mirrors Android RemoveFromParent / iOS RemoveFromSuperview.
// Always remove via CachedChildren directly: Content = null is a no-op when _content
// is null (e.g. ScrollViewHandler adds via paddingShim.CachedChildren.Add, not the
// Content setter), leaving the element with a live parent and causing a COM exception
// when we try to reparent it. Only clear _content when it actually tracks fwElement.
if (platformView is FrameworkElement fwElement && fwElement.Parent is not null)
{
if (fwElement.Parent is ContentPanel existingContentPanel)
{
existingContentPanel.CachedChildren.Remove(fwElement);
if (existingContentPanel.Content == fwElement)
{
existingContentPanel.Content = null;
}
}
else if (fwElement.Parent is MauiPanel existingPanel)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Handler Mapper and Property Patterns / Regression Prevention — Same incomplete parent-detection as ContentViewHandler.Windows.cs: only ContentPanel/MauiPanel parents are detached before reparenting; a WrapperView (or any other native Panel/ContentControl) parent is left untouched, so switching Border content that needs a platform container (Shadow/Clip/InputTransparent) between hosts can still throw the WinUI COM exception this PR is meant to fix. See the matching finding in ContentViewHandler.Windows.cs for the established general-purpose pattern (ViewHandlerOfT.Windows.cs.SetupContainer) that already handles arbitrary panel types and should be reused/mirrored here instead of this narrower type check.

{
existingPanel.CachedChildren.Remove(fwElement);
}
}
Comment on lines +35 to +49

handler.PlatformView.Content = platformView;
}
else
{
handler.PlatformView.Content = null;
}

}
Expand Down
34 changes: 29 additions & 5 deletions src/Core/src/Handlers/ContentView/ContentViewHandler.Windows.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using Microsoft.Maui.Graphics;
using Microsoft.UI.Xaml;

namespace Microsoft.Maui.Handlers
{
Expand All @@ -21,13 +22,36 @@ static void UpdateContent(IContentViewHandler handler)
_ = handler.VirtualView ?? throw new InvalidOperationException($"{nameof(VirtualView)} should have been set by base class.");
_ = handler.MauiContext ?? throw new InvalidOperationException($"{nameof(MauiContext)} should have been set by base class.");

handler.PlatformView.CachedChildren.Clear();

if (handler.VirtualView.PresentedContent is IView view)
{
// Detach the old handler if it exists (prevents WinUI COM exception on reuse)
view.Handler?.DisconnectHandler();
handler.PlatformView.CachedChildren.Add(view.ToPlatform(handler.MauiContext));
var platformView = view.ToPlatform(handler.MauiContext);

// Detach from existing parent — mirrors Android RemoveFromParent / iOS RemoveFromSuperview.
// Always remove via CachedChildren directly: Content = null is a no-op when _content
// is null (e.g. ScrollViewHandler adds via paddingShim.CachedChildren.Add, not the
// Content setter), leaving the element with a live parent and causing a COM exception
// when we try to reparent it. Only clear _content when it actually tracks fwElement.
if (platformView is FrameworkElement fwElement && fwElement.Parent is not null)
{
if (fwElement.Parent is ContentPanel existingContentPanel)
{
existingContentPanel.CachedChildren.Remove(fwElement);
if (existingContentPanel.Content == fwElement)
{
existingContentPanel.Content = null;
}
}
else if (fwElement.Parent is MauiPanel existingPanel)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Windows handler reparenting — This only detaches children whose current parent is a ContentPanel or MauiPanel, but the Windows handler infrastructure also reparents views through plain WinUI Panel instances (see ViewHandler<T>.SetupContainer, which falls back to (Parent as Panel)?.Children). In that case this branch falls through and handler.PlatformView.Content = platformView still hits WinUI's 'element already has a parent' exception. Please add a Panel fallback (using CachedChildren for MauiPanel, otherwise Panel.Children) and apply the same helper to the duplicated BorderHandler.Windows.cs path.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Handler Mapper and Property Patterns / Regression Prevention — The reparent-detach logic only matches ContentPanel and MauiPanel parents; any other native parent type is silently skipped, leaving the element attached to its old parent and able to reproduce the original WinUI COM exception this PR fixes. In particular, ViewHandlerOfT.Windows.cs.SetupContainer() wraps content that needs a container (Shadow, Clip, InputTransparent, etc.) in a WrapperView, which derives from Grid, not MauiPanel — so fwElement.Parent is WrapperView falls through both branches here with no detach performed. The codebase already has an established general-purpose pattern for this exact problem (PlatformView.Parent is MauiPanel mauiPanel ? mauiPanel.CachedChildren : (PlatformView.Parent as Panel)?.Children in ViewHandlerOfT.Windows.cs/ButtonExtensions.GetContent) that handles arbitrary Panel parents, not just MauiPanel. This code area was originally added in #30047 to fix #29930 (a different static-resource-style content-reuse COM exception) — since that scenario can involve content parented under any container type, this narrower ContentPanel/MauiPanel-only check risks reintroducing #29930 for content that requires a WrapperView, and this gap is not covered by the new Issue36298 test (which only exercises plain Label content with no Shadow/Clip).

{
existingPanel.CachedChildren.Remove(fwElement);
}
}
Comment on lines +34 to +48

handler.PlatformView.Content = platformView;
}
else
{
handler.PlatformView.Content = null;
}
}

Expand Down
Loading