Skip to content

[iOS] Put the UIScrollView into a UIView in a border handler #25098

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

Jump to bottom
Merged
merged 1 commit into from
Jan 9, 2025
Merged

[iOS] Put the UIScrollView into a UIView in a border handler #25098

merged 1 commit into from
Jan 9, 2025

Conversation

kubaflo
Copy link
Contributor

@kubaflo kubaflo commented Oct 4, 2024

Description of Change

Putting UIScroll view in a UIView is necessary when applying masking which is a case with the border control. However, checking from border handler is better than checking up from UiScrollViews like in here #14096

Issues Fixed

Fixes #8923
Fixes #14091
Fixes #7524
Fixes #14091
Fixes #20736
Fixes #18486
Closes #24683

@kubaflo kubaflo requested a review from a team as a code owner October 4, 2024 23:05
@kubaflo kubaflo requested review from Eilon and tj-devel709 October 4, 2024 23:05
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Oct 4, 2024
@kubaflo kubaflo requested review from rmarinho, PureWeen and jsuarezruiz and removed request for Eilon and tj-devel709 October 4, 2024 23:05
@kubaflo kubaflo mentioned this pull request Oct 4, 2024
Closed
4 tasks
@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen
PureWeen previously requested changes Oct 7, 2024
View reviewed changes
src/Core/src/Handlers/Border/BorderHandler.iOS.cs Outdated
// If the content is a UIScrollView, we need a container to handle masks and clip shapes effectively.
if (platformContent is UIScrollView && content.Handler is not null)
{
content.Handler.HasContainer = true;
Copy link
Member

Choose a reason for hiding this comment

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

I think with how HasContainer works this might be risky

Something might cause Container to reset back to false

image

I would be curious at some point to explore a way for the parent to flag the child as needing a container but for now I think we'll just have to add a view here and then add the content to that view.

@kubaflo kubaflo force-pushed the scrollview-inside-border branch from 549a89f to ef14890 Compare October 8, 2024 16:04
@rmarinho
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@kubaflo
Copy link
Contributor Author

kubaflo commented Oct 11, 2024

Screenshot 2024-10-11 at 10 37 01

Seems like an unrelated test failure

@maonaoda
Copy link
Contributor

maonaoda commented Oct 29, 2024 •
edited
Loading

This fix seems to cause the Border size to change strangely in certain situations (background back to the foreground, page deleted).
The Green one is the containerView
and The Red is Border`s Stroke

截屏2024-10-29 13 14 41

@kubaflo kubaflo mentioned this pull request Nov 10, 2024
Closed
@kubaflo kubaflo force-pushed the scrollview-inside-border branch from ef14890 to fbf3fe3 Compare November 12, 2024 13:22
@jsuarezruiz
Copy link
Contributor

/rebase

@github-actions github-actions bot force-pushed the scrollview-inside-border branch from fbf3fe3 to 6c5e001 Compare November 12, 2024 14:41
@jsuarezruiz
Copy link
Contributor

This fix seems to cause the Border size to change strangely in certain situations (background back to the foreground, page deleted). The Green one is the containerView and The Red is Border`s Stroke

截屏2024-10-29 13 14 41

Could you share a piece of XAML or C# to reproduce your UI and add an UITest to validate it?

@kubaflo
Copy link
Contributor Author

kubaflo commented Nov 12, 2024

This fix seems to cause the Border size to change strangely in certain situations (background back to the foreground, page deleted). The Green one is the containerView and The Red is Border`s Stroke

截屏2024-10-29 13 14 41

As @jsuarezruiz wrote could you please share a code with us, so we can try to replicate it? Also, I've modified this PR a bit so it would be cool if you try again and let us know the outcome. Thanks!

@PureWeen
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@kubaflo kubaflo force-pushed the scrollview-inside-border branch from 6c5e001 to 9b5f31b Compare November 12, 2024 19:12
@PureWeen
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

PureWeen commented Dec 4, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen modified the milestones: .NET 9 SR2, .NET 9 SR3 Dec 6, 2024
@kubaflo kubaflo force-pushed the scrollview-inside-border branch from 33551b3 to e7e9fa4 Compare December 7, 2024 01:06
@PureWeen
Copy link
Member

PureWeen commented Dec 7, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@kubaflo kubaflo mentioned this pull request Dec 11, 2024
Merged
@PureWeen
Copy link
Member

PureWeen commented Jan 6, 2025

/rebase

@github-actions github-actions bot force-pushed the scrollview-inside-border branch from e7e9fa4 to 22ade23 Compare January 6, 2025 16:06
@PureWeen PureWeen dismissed rmarinho’s stale review January 6, 2025 16:06

Changes Applied

@PureWeen
Copy link
Member

PureWeen commented Jan 6, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen
PureWeen requested changes Jan 6, 2025
View reviewed changes
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue20736.cs
[Test]
[Category(UITestCategories.Editor)]
[Category(UITestCategories.Border)]
public void EditorScrollingWhenEnclosedInBorder()
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is a little bit flakey on Android

https://dev.azure.com/xamarin/public/_build/results?buildId=131616&view=ms.vss-test-web.build-test-results-tab&runId=3552379&resultId=100044&paneView=debug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, it the screenshots look different on various android devices. Maybe I should disable Android for this?

@kubaflo kubaflo force-pushed the scrollview-inside-border branch from 22ade23 to e0023b4 Compare January 7, 2025 14:16
@PureWeen
Copy link
Member

PureWeen commented Jan 7, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@kubaflo kubaflo force-pushed the scrollview-inside-border branch from cb5794f to c47c3ff Compare January 8, 2025 00:05
@kubaflo kubaflo force-pushed the scrollview-inside-border branch from c47c3ff to aa275e1 Compare January 8, 2025 00:17
@PureWeen
Copy link
Member

PureWeen commented Jan 8, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen requested a review from Copilot January 8, 2025 18:13
Copilot
Copilot AI reviewed Jan 8, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 7 out of 10 changed files in this pull request and generated no comments.

Files not reviewed (3)
  • src/Controls/tests/TestCases.HostApp/Issues/Issue20736.xaml: Language not supported
  • src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt: Language not supported
  • src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt: Language not supported
Comments suppressed due to low confidence (2)

src/Core/src/Handlers/Border/BorderHandler.iOS.cs:2

  • The 'using System.Formats.Asn1;' directive is unnecessary and should be removed.
using System.Formats.Asn1;

src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs:12

  • The removal of the NeedsContainer override affects the public API. This is potentially a breaking change.
public override bool NeedsContainer

@PureWeen
Copy link
Member

PureWeen commented Jan 9, 2025

  • failing tests are unrelated

@PureWeen PureWeen merged commit da0f504 into dotnet:main Jan 9, 2025
102 of 104 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jsuarezruiz jsuarezruiz jsuarezruiz left review comments

Copilot code review Copilot Copilot left review comments

@PureWeen PureWeen PureWeen requested changes

@rmarinho rmarinho rmarinho left review comments

Assignees
No one assigned
Labels
area-controls-scrollview ScrollView community ✨ Community Contribution platform/iOS 🍎
Projects
MAUI SDK Ongoing
Status: Done
Milestone
.NET 9 SR3
5 participants
@kubaflo @jsuarezruiz @rmarinho @maonaoda @PureWeen