-
Notifications
You must be signed in to change notification settings - Fork 2k
[Windows]Fix ContentPresenter Throws System.ArgumentException When Dynamically Assigning RefreshView or ScrollView Content #36430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c2b226d
832f997
49d188c
fad4715
0d42687
97a2789
d310443
7d8dc70
6aa094e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||
| { | ||
| 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() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[major] Regression Prevention and Test Coverage — This PR changes both |
||
| { | ||
| // 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 | ||
|
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); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| using System; | ||
| using Microsoft.UI.Xaml; | ||
|
|
||
| namespace Microsoft.Maui.Handlers | ||
| { | ||
|
|
@@ -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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[major] Handler Mapper and Property Patterns / Regression Prevention — Same incomplete parent-detection as |
||
| { | ||
| existingPanel.CachedChildren.Remove(fwElement); | ||
| } | ||
| } | ||
|
Comment on lines
+35
to
+49
|
||
|
|
||
| handler.PlatformView.Content = platformView; | ||
| } | ||
| else | ||
| { | ||
| handler.PlatformView.Content = null; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| 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 | ||
| { | ||
|
|
@@ -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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[major] Windows handler reparenting — This only detaches children whose current parent is a
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[major] Handler Mapper and Property Patterns / Regression Prevention — The reparent-detach logic only matches |
||
| { | ||
| existingPanel.CachedChildren.Remove(fwElement); | ||
| } | ||
| } | ||
|
Comment on lines
+34
to
+48
|
||
|
|
||
| handler.PlatformView.Content = platformView; | ||
| } | ||
| else | ||
| { | ||
| handler.PlatformView.Content = null; | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[major] Regression test coverage — The regression title and production fix target
ContentPresenter/IContentViewreparenting, but the sample host here is aContentView. 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 aContentPresenter(for example via a control template) and verify that it fails on the old handler code and passes with this fix.