-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fixed Rectangle renders as thin line instead of filled shape for small height #31651
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
base: main
Are you sure you want to change the base?
Conversation
Hey there @@Dhivya-SF4094! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
public void UpdateSizeOnlyWhenStrokeExists() | ||
{ | ||
App.WaitForElement("Issue31330BoxView"); | ||
VerifyScreenshot(); |
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.
Running a build, pending snapshot on some platforms.
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.
Test failing on Windows:
at UITest.Appium.HelperExtensions.Wait(Func`1 query, Func`2 satisfactory, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2530
at UITest.Appium.HelperExtensions.WaitForAtLeastOne(Func`1 query, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2547
at UITest.Appium.HelperExtensions.WaitForElement(IApp app, String marked, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency, Nullable`1 postTimeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 743
at Microsoft.Maui.TestCases.Tests.Issues.Issue31330.UpdateSizeOnlyWhenStrokeExists() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue31330.cs:line 18
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
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.
AutomationId = "Issue31330BoxView" | ||
}; | ||
|
||
var ellipse = new Ellipse |
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.
Can maintain the same test, but, including a Button to change at runtime the StrokeThickness value?
Start at 0, and tapping the Button, change the value to 20.
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.
@jsuarezruiz Updated the test case to include a Button that changes the StrokeThickness at runtime, starting from 0 and updating to 20 on tap.
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.
Pull Request Overview
This PR fixes a rendering issue where Rectangle shapes with small height values (e.g., 1.2px) were rendering as thin lines instead of filled rectangles. The issue occurred because the Shape.cs class was always reducing the shape bounds by the stroke thickness, even when no stroke was present.
- Modified stroke bounds adjustment logic to only apply when a stroke is actually present
- Added comprehensive UI tests to validate the fix across platforms
- Ensures small filled shapes render correctly without collapsing to imperceptible lines
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/Controls/src/Core/Shapes/Shape.cs | Added conditional check to only apply stroke inset when stroke exists |
src/Controls/tests/TestCases.HostApp/Issues/Issue31330.cs | Created UI test page demonstrating rectangle rendering with small height |
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue31330.cs | Added automated test to verify screenshot-based validation |
new RowDefinition { Height = GridLength.Star } | ||
}; | ||
|
||
Label labelText = new Label { Text = "The test passes if the edges of the circle touch the BoxView.", FontAttributes = FontAttributes.Bold }; |
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.
The label text describes a circle/BoxView test, but this UI test is specifically for Rectangle rendering with small height values. The label should describe the actual test being performed for the Rectangle shape.
Copilot uses AI. Check for mistakes.
Ellipse ellipse = new Ellipse | ||
{ | ||
Fill = Colors.Yellow, | ||
StrokeThickness = 0, | ||
WidthRequest = 100, | ||
HeightRequest = 100 | ||
}; | ||
|
||
Button button = new Button { Text = "Update StrokeThickness" }; | ||
button.Clicked += (s, e) => | ||
{ | ||
ellipse.StrokeThickness = 20; | ||
}; |
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.
The Ellipse and Button components appear unrelated to the Rectangle rendering issue being tested. These elements may confuse the test's purpose and should be removed unless they serve a specific validation purpose for this issue.
Copilot uses AI. Check for mistakes.
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
public void UpdateSizeOnlyWhenStrokeExists() | ||
{ | ||
App.WaitForElement("Issue31330BoxView"); | ||
VerifyScreenshot(); |
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.
Test failing on Windows:
at UITest.Appium.HelperExtensions.Wait(Func`1 query, Func`2 satisfactory, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2530
at UITest.Appium.HelperExtensions.WaitForAtLeastOne(Func`1 query, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 2547
at UITest.Appium.HelperExtensions.WaitForElement(IApp app, String marked, String timeoutMessage, Nullable`1 timeout, Nullable`1 retryFrequency, Nullable`1 postTimeout) in /_/src/TestUtils/src/UITest.Appium/HelperExtensions.cs:line 743
at Microsoft.Maui.TestCases.Tests.Issues.Issue31330.UpdateSizeOnlyWhenStrokeExists() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue31330.cs:line 18
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
public void UpdateSizeOnlyWhenStrokeExists() | ||
{ | ||
App.WaitForElement("Issue31330BoxView"); | ||
VerifyScreenshot(); |
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.
06493b2
to
e574466
Compare
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details:
When height is a small non-integer value (e.g., 1.2), the shape renders incorrectly as a thin stroked line instead of a filled rectangle.
Root Cause
In Shape.cs, the TransformPathForBounds method always adjusts the path bounds for stroke thickness, reducing the dimensions by the StrokeThickness value.
For small heights with the default stroke thickness (1.0px), this resulted in a tiny internal rectangle that appeared as a line rather than a filled shape, unlike BoxView which rendered at the full specified height.
Description of Change
Added a condition to only apply stroke inset when a stroke is actually present (Stroke != null && StrokeThickness > 0). This prevents the shape's interior path from collapsing to an imperceptible height when rendering small filled shapes, particularly when no stroke is applied.
Validated the behaviour in the following platforms
Issues Fixed:
Fixes #31330
Screenshots