Skip to content
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.

Sign up for GitHub

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

[Windows] Fix for setting IsClippedToBounds="True" inside a Border control will crash on Windows #25506

Merged
merged 8 commits into from
Dec 5, 2024
+55 −2
Merged

[Windows] Fix for setting IsClippedToBounds="True" inside a Border control will crash on Windows #25506

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
388c3c8
Fixed 18937
Tamilarasan-Paranthaman Oct 24, 2024
c5b6682
Modified test sample.
Tamilarasan-Paranthaman Oct 28, 2024
a81a447
Updated the fix.
Tamilarasan-Paranthaman Oct 30, 2024
65b7ea7
Simplified the test case.
Tamilarasan-Paranthaman Oct 30, 2024
13ce9bc
Removed platform condition.
Tamilarasan-Paranthaman Oct 30, 2024
943c085
Changed return type.
Tamilarasan-Paranthaman Nov 8, 2024
82eaee9
ContentPanel changes.
Tamilarasan-Paranthaman Nov 26, 2024
07b2ef6
Revert unwanted changes
Tamilarasan-Paranthaman Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/Controls/tests/TestCases.HostApp/Issues/Issue18937.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?xml version="1.0" encoding="utf-8" ?>
<ContentPage
xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
x:Class="Maui.Controls.Sample.Issues.Issue18937">

<Border>
<StackLayout BackgroundColor="red"
IsClippedToBounds="True">
<Label AutomationId="Label"
Text="Label Inside the Border"
VerticalOptions="Center"
HorizontalOptions="Center"/>
</StackLayout>
</Border>

</ContentPage>
12 changes: 12 additions & 0 deletions src/Controls/tests/TestCases.HostApp/Issues/Issue18937.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
namespace Maui.Controls.Sample.Issues
{

[Issue(IssueTracker.Github, 18937, "[Windows] Setting IsClippedToBound is true inside a Border control will crash on Windows", PlatformAffected.UWP)]
public partial class Issue18937 : ContentPage
{
public Issue18937()
{
InitializeComponent();
}
}
}
21 changes: 21 additions & 0 deletions src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue18937.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
using NUnit.Framework;
using UITest.Appium;
using UITest.Core;

namespace Microsoft.Maui.TestCases.Tests.Issues
{
public class Issue18937 : _IssuesUITest
{
public Issue18937(TestDevice device) : base(device) { }

public override string Issue => "[Windows] Setting IsClippedToBound is true inside a Border control will crash on Windows";

[Test]
[Category(UITestCategories.Border)]
public void ExceptionShouldNotOccurWhenIsClippedToBoundsIsTrue()
{
var testLabel = App.WaitForElement("Label");
Assert.That(testLabel.GetText(), Is.EqualTo("Label Inside the Border"));
}
}
}
15 changes: 11 additions & 4 deletions src/Core/src/Platform/Windows/ContentPanel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Numerics;
using System.Threading.Tasks;
using Microsoft.Graphics.Canvas;
using Microsoft.Maui.Graphics;
#if MAUI_GRAPHICS_WIN2D
Expand Down Expand Up @@ -60,7 +61,7 @@ internal FrameworkElement? Content
var size = new global::Windows.Foundation.Size(Math.Max(0, actual.Width), Math.Max(0, actual.Height));

// We need to update the clip since the content's position might have changed
UpdateClip(_borderStroke?.Shape, size.Width, size.Height);
_ = UpdateClipAsync(_borderStroke?.Shape, size.Width, size.Height);

return size;
}
Expand Down Expand Up @@ -89,7 +90,7 @@ void ContentPanelSizeChanged(object sender, SizeChangedEventArgs e)
}

_borderPath.UpdatePath(_borderStroke?.Shape, width, height);
UpdateClip(_borderStroke?.Shape, width, height);
_ = UpdateClipAsync(_borderStroke?.Shape, width, height);
}

internal void EnsureBorderPath(bool containsCheck = true)
Expand Down Expand Up @@ -159,10 +160,10 @@ void UpdateBorder(IShape? strokeShape)
return;
}

UpdateClip(strokeShape, width, height);
_ = UpdateClipAsync(strokeShape, width, height);
}

void UpdateClip(IShape? borderShape, double width, double height)
async Task UpdateClipAsync(IShape? borderShape, double width, double height)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right way to approach this problem. Let me try and ping some WinAppSDK folks and see if they have any suggestions.

In the meantime: the Loaded event should be fired after layout is complete on a Visual -- maybe you can check if that has fired first before assigning the clip?

Copy link
Member

Choose a reason for hiding this comment

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

WinAppSDK folks also suggested to try switching SizeChanged to LayoutUpdated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is the right way to approach this problem. Let me try and ping some WinAppSDK folks and see if they have any suggestions.

In the meantime: the Loaded event should be fired after layout is complete on a Visual -- maybe you can check if that has fired first before assigning the clip?

With our PR fix, the Loaded event is fired before assigning the clip, ensuring that the layout is completed on the visual before applying the clip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WinAppSDK folks also suggested to try switching SizeChanged to LayoutUpdated

We ensured the functionality by calling the UpdateClip method on the LayoutUpdated event, as LayoutUpdated triggers frequently whenever the layout is updated. However, we only need to update the clip during the initial load and when the size changes. To address this, we added flags to limit updates to these specific scenarios. Despite this, after switching from the SizeChanged event to the LayoutUpdated event, the issue persists.

Copy link
Member

Choose a reason for hiding this comment

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

I've narrowed down the root cause of the issue, and reported it here: https://github.com/microsoft/WindowsAppSDK/issues/4914

We can remove all use of async. To fix this:

In LayoutPanel.cs change ArrangeOverride to use Composition.Visual.Clip:

protected override WSize ArrangeOverride(WSize finalSize)
{
	var actual = base.ArrangeOverride(finalSize);
	var visual = ElementCompositionPreview.GetElementVisual(this);
	visual.Clip = ClipsToBounds ? visual.Compositor.CreateInsetClip() : null;

	return actual;
}

Then, add a check in ContentPanel.cs to ensure the clip isn't overridden:

void UpdateClip(IShape? borderShape, double width, double height)
{
	if (Content is null)
	{
		return;
	}

	if (height <= 0 && width <= 0)
	{
		return;
	}

        // Ensure clip isn't overridden on LayoutPanel
	if (Content is LayoutPanel panel && panel.ClipsToBounds)
	{
		return;
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've narrowed down the root cause of the issue, and reported it here: https://github.com/microsoft/WindowsAppSDK/issues/4914

We can remove all use of async. To fix this:

In LayoutPanel.cs change ArrangeOverride to use Composition.Visual.Clip:

protected override WSize ArrangeOverride(WSize finalSize)
{
	var actual = base.ArrangeOverride(finalSize);
	var visual = ElementCompositionPreview.GetElementVisual(this);
	visual.Clip = ClipsToBounds ? visual.Compositor.CreateInsetClip() : null;

	return actual;
}

Then, add a check in ContentPanel.cs to ensure the clip isn't overridden:

void UpdateClip(IShape? borderShape, double width, double height)
{
	if (Content is null)
	{
		return;
	}

	if (height <= 0 && width <= 0)
	{
		return;
	}

        // Ensure clip isn't overridden on LayoutPanel
	if (Content is LayoutPanel panel && panel.ClipsToBounds)
	{
		return;
	}

As per the suggested fix, I tested it using other shapes in the border, such as Ellipse and RoundRectangle. I observed that clipping was not properly applied due to the clipping logic already being handled in the LayoutPanel and returned in the UpdateClip method of the ContentPanel class.

As a result, the additional clipping code in the UpdateClip based on the border shape was not executed, leading to improper clipping in the UI. To address this, I fixed the issue in the LayoutPanel by skipping the clipping when the parent is a Border. In such cases, clipping will now be handled by the ContentPanel.

Could you please review the changes and let me know if you have any concerns?

{
if (Content is null)
{
Expand All @@ -182,6 +183,12 @@ void UpdateClip(IShape? borderShape, double width, double height)
}

var visual = ElementCompositionPreview.GetElementVisual(Content);

visual.Clip = null;

//Adding Task.Yield ensures that all layouts are properly updated before applying the clip.
await Task.Yield();

var compositor = visual.Compositor;

PathF? clipPath;
Expand Down
Loading