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.

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

[iOS] Fix Border Content clipping issues #10964

Merged
merged 39 commits into from
Mar 29, 2023
Merged

[iOS] Fix Border Content clipping issues #10964

merged 39 commits into from
Mar 29, 2023

Conversation

jsuarezruiz
Copy link
Contributor

@jsuarezruiz jsuarezruiz commented Oct 27, 2022

Description of Change

Fix Border Content clipping issues.

fix-clipping-ios-1
fix-clipping-ios-2

Issues Fixed

Fixes #6986
Fixes #10724
Fixes #7521
Fixes #10478
Fixes #12580
Fixes #9952
Fixes #9081
Fixes #13388
Fixes #13011
Fixes #11152

@jsuarezruiz jsuarezruiz added t/bug Something isn't working area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing platform/iOS 🍎 labels Oct 27, 2022
@jsuarezruiz jsuarezruiz marked this pull request as ready for review October 28, 2022 10:58
@jsuarezruiz jsuarezruiz marked this pull request as draft November 3, 2022 08:34
@jsuarezruiz jsuarezruiz marked this pull request as ready for review November 16, 2022 15:36
@PureWeen PureWeen requested review from PureWeen and removed request for PureWeen November 17, 2022 17:10
Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Testing with the original posters XAML and the results seem off

 <VerticalStackLayout>
        <Border StrokeThickness="20"
                            Stroke="Silver"
                            Background="White"
                            Margin="0"
                            Padding="0">
            <Border.StrokeShape>
                <RoundRectangle CornerRadius="30" />
            </Border.StrokeShape>
            <Grid HeightRequest="30" BackgroundColor="Green">
                <StackLayout Orientation="Horizontal">
                    <Frame Padding="0" HasShadow="false"
                                       BackgroundColor="Maroon"
                                       CornerRadius="20"
                                       HeightRequest="40"
                                       WidthRequest="40" VerticalOptions="Center" >
                    </Frame>
                    <Label Text="Parameters"
                                       VerticalOptions="FillAndExpand" HorizontalOptions="FillAndExpand"
                                       FontSize="Medium" FontFamily="Bold"
                                       HorizontalTextAlignment="Center" VerticalTextAlignment="Center" />
                    <Frame Padding="0" HasShadow="false" CornerRadius="0" Margin="0,0,-20,0"
                                       BackgroundColor="Blue"
                                       HeightRequest="40"
                                       WidthRequest="40" VerticalOptions="Center" >
                    </Frame>
                </StackLayout>
            </Grid>
        </Border>
    </VerticalStackLayout>

image

@jsuarezruiz
Copy link
Contributor Author

jsuarezruiz commented Nov 22, 2022

Testing with the original posters XAML and the results seem off

 <VerticalStackLayout>
        <Border StrokeThickness="20"
                            Stroke="Silver"
                            Background="White"
                            Margin="0"
                            Padding="0">
            <Border.StrokeShape>
                <RoundRectangle CornerRadius="30" />
            </Border.StrokeShape>
            <Grid HeightRequest="30" BackgroundColor="Green">
                <StackLayout Orientation="Horizontal">
                    <Frame Padding="0" HasShadow="false"
                                       BackgroundColor="Maroon"
                                       CornerRadius="20"
                                       HeightRequest="40"
                                       WidthRequest="40" VerticalOptions="Center" >
                    </Frame>
                    <Label Text="Parameters"
                                       VerticalOptions="FillAndExpand" HorizontalOptions="FillAndExpand"
                                       FontSize="Medium" FontFamily="Bold"
                                       HorizontalTextAlignment="Center" VerticalTextAlignment="Center" />
                    <Frame Padding="0" HasShadow="false" CornerRadius="0" Margin="0,0,-20,0"
                                       BackgroundColor="Blue"
                                       HeightRequest="40"
                                       WidthRequest="40" VerticalOptions="Center" >
                    </Frame>
                </StackLayout>
            </Grid>
        </Border>
    </VerticalStackLayout>

image

Tested with Catalyst and the result is the same as Android. Weird, will do more tests.
image

@rmarinho
Copy link
Member

Windows seems doesn't render the frames..

image

@rmarinho
Copy link
Member

IMG_1642

on IOS is almost fine, theres some white background

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

My last comment run on iOS was incorrect. I'm not sure why I got that result.

What I'm curious about now is some of the math and results here. I'm not super strong in this area so I'm probably missing something

iOS

I don't quite understand why we're multiplying the line width by 2. From the docs the line width straddles the path

https://developer.apple.com/documentation/coregraphics/1455270-cgcontextsetlinewidth?language=objc

The default line width is 1 unit. When stroked, the line straddles the path, with half of the total width on either side.

So, instead of doubling the width should we be increasing the width/height so we don't have to multiply by 2?

The iOS results don't seem quite correct because the blue frame is a rectangle but it's getting a circle cut out of it. It seems like that blue should be filling all this space.

image

Android

The issue I'm seeing with Android should probably be a new issue. If it is indeed an issue.

If you look at android it seems like the way the path works is the same as iOS.

image

The path of the border overlaps the surrounding grid which is basically what @rmarinho seems to have noticed here
#10964 (comment)

You can also see that the full height of the Border is 70 dp but due to the border not filling in that space you end up with white space.

image

It feels like

  • Android needs to be fixed so the width/height of the border bounds is increased by another BorderWidth of size
  • iOS should do the same and the stroke thickness shouldn't be multiplied by 2

return path;
}

internal PathF GetInnerPath(float strokeWidth)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We was clipping using the Border shape. That is correct in many cases but not always. Using a RoundRectangle, the Corner Radius value is not the same in the inner or outer. We want in this case user the inner value. For that reason, added a new method returning the same shape but with different radius.

@PureWeen
Copy link
Member

PureWeen commented Jan 9, 2023

Testing with the original posters XAML and the results seem off

 <VerticalStackLayout>
        <Border StrokeThickness="20"
                            Stroke="Silver"
                            Background="White"
                            Margin="0"
                            Padding="0">
            <Border.StrokeShape>
                <RoundRectangle CornerRadius="30" />
            </Border.StrokeShape>
            <Grid HeightRequest="30" BackgroundColor="Green">
                <StackLayout Orientation="Horizontal">
                    <Frame Padding="0" HasShadow="false"
                                       BackgroundColor="Maroon"
                                       CornerRadius="20"
                                       HeightRequest="40"
                                       WidthRequest="40" VerticalOptions="Center" >
                    </Frame>
                    <Label Text="Parameters"
                                       VerticalOptions="FillAndExpand" HorizontalOptions="FillAndExpand"
                                       FontSize="Medium" FontFamily="Bold"
                                       HorizontalTextAlignment="Center" VerticalTextAlignment="Center" />
                    <Frame Padding="0" HasShadow="false" CornerRadius="0" Margin="0,0,-20,0"
                                       BackgroundColor="Blue"
                                       HeightRequest="40"
                                       WidthRequest="40" VerticalOptions="Center" >
                    </Frame>
                </StackLayout>
            </Grid>
        </Border>
    </VerticalStackLayout>

image

Alright, I think the key piece of information I was missing here is that you need to rebase to see these results.
If I rebase this PR to main then I get the incorrect layout described here.

Here it is running on catalyst with everything rebased to main

image

@rmarinho rmarinho enabled auto-merge (squash) March 29, 2023 17:28
@rmarinho rmarinho requested review from PureWeen and removed request for PureWeen March 29, 2023 18:17
@rmarinho rmarinho merged commit 0d9ac95 into main Mar 29, 2023
@rmarinho rmarinho deleted the fix-10724 branch March 29, 2023 19:12
@hartez hartez mentioned this pull request Mar 30, 2023
@samhouts samhouts added the backport/suggested The PR author or issue review has suggested that the change should be backported. label Jun 28, 2023
@breenbob
Copy link
Contributor

Would be massively in favour of a backport of these fixes!

@jcsnider
Copy link

Couldn't agree more, for whatever it's worth.

@hartez hartez added the backport/NO This change should not be backported. It may break customers. label Jul 5, 2023
@jrahma
Copy link

jrahma commented Jul 10, 2023

I am still having the problem. Hope the fix will be available soon.

@darshanio
Copy link

Would be massively in favour of a backport of these fixes!

Agree.
Hope we get the fix soon in .Net 7.

@K0Ma19
Copy link

K0Ma19 commented Jul 18, 2023

Sorry if this is a dumb question, but will these changes be in the next update?
When can I expect an update and where can I see what happens in a future update?

@KSemenenko
Copy link
Contributor

I feel it will be .NET 8 preview 7 or so

@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing backport/NO This change should not be backported. It may break customers. backport/suggested The PR author or issue review has suggested that the change should be backported. platform/iOS 🍎 t/bug Something isn't working
Projects
None yet