Change iOS SetNeedsLayout propagation mechanism #26629

albyrock87 · 2024-12-14T10:14:02Z

Description of Change

I've been asked to split #25664 into separate PR.

This is the first piece which aims to change the way we propagate SetNeedsLayout by doing that only when MAUI commands.

The bug I'm solving with this PR is caused by this unwanted propagation when SetNeedsLayout is being invoked by iOS
Moving the propagation to InvalidateMeasure will enable third party libraries to remove the workarounds they've done to propagate SetNeedsLayout and don't care about propagation anymore

This also enables us to receive a SetNeedsLayout in TemplatedCell which will allow us to resize the cell without relaying on MeasureInvalidated cross platform event propagation.
There will be a follow-up PR to tackle this.

Issues Fixed

Fixes #24996

src/Controls/tests/DeviceTests/Elements/CarouselView/CarouselViewTests.cs

PureWeen · 2024-12-14T15:27:12Z

/azp run

azure-pipelines · 2024-12-14T15:27:32Z

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen · 2024-12-15T20:26:24Z

/azp run

azure-pipelines · 2024-12-15T20:26:45Z

Azure Pipelines successfully started running 3 pipeline(s).

src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt

rmarinho

HeaderAndFooterRender failing with CollectionView2 https://dev.azure.com/xamarin/public/_build/results?buildId=130764&view=ms.vss-test-web.build-test-results-tab&runId=3507758&resultId=100038&paneView=debug

src/Core/src/Platform/iOS/MauiScrollView.cs

Copilot

Copilot reviewed 8 out of 23 changed files in this pull request and generated 1 comment.

Files not reviewed (15)

src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt: Language not supported
src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt: Language not supported
src/Controls/tests/TestCases.HostApp/Issues/Issue24996.xaml: Language not supported
src/Core/src/Platform/iOS/LayoutView.cs: Evaluated as low risk
src/Controls/src/Core/Compatibility/Handlers/iOS/FrameRenderer.cs: Evaluated as low risk
src/Controls/tests/DeviceTests/Elements/CarouselView/CarouselViewTests.cs: Evaluated as low risk
src/Core/src/Platform/iOS/PageViewController.cs: Evaluated as low risk
src/Core/src/Platform/iOS/MauiView.cs: Evaluated as low risk
src/Core/src/Handlers/Layout/LayoutHandler.iOS.cs: Evaluated as low risk
src/Controls/src/Core/Compatibility/Handlers/iOS/VisualElementRenderer.cs: Evaluated as low risk
src/Core/src/Platform/iOS/MauiScrollView.cs: Evaluated as low risk
src/Controls/src/Core/Handlers/Items/iOS/MauiCollectionView.cs: Evaluated as low risk
src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs: Evaluated as low risk
src/Core/src/Platform/iOS/IMauiPlatformView.cs: Evaluated as low risk
src/Core/src/Platform/iOS/PageView.cs: Evaluated as low risk

src/Core/src/Platform/iOS/ViewExtensions.cs

Copilot

Copilot reviewed 8 out of 23 changed files in this pull request and generated no comments.

Files not reviewed (15)

src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt: Language not supported
src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt: Language not supported
src/Controls/tests/TestCases.HostApp/Issues/Issue24996.xaml: Language not supported
src/Core/src/Platform/iOS/LayoutView.cs: Evaluated as low risk
src/Controls/tests/DeviceTests/Elements/CarouselView/CarouselViewTests.cs: Evaluated as low risk
src/Controls/src/Core/Compatibility/Handlers/iOS/FrameRenderer.cs: Evaluated as low risk
src/Core/src/Platform/iOS/PageViewController.cs: Evaluated as low risk
src/Core/src/Platform/iOS/MauiView.cs: Evaluated as low risk
src/Core/src/Handlers/Layout/LayoutHandler.iOS.cs: Evaluated as low risk
src/Controls/src/Core/Compatibility/Handlers/iOS/VisualElementRenderer.cs: Evaluated as low risk
src/Core/src/Platform/iOS/MauiScrollView.cs: Evaluated as low risk
src/Controls/src/Core/Handlers/Items/iOS/MauiCollectionView.cs: Evaluated as low risk
src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs: Evaluated as low risk
src/Core/src/Platform/iOS/PageView.cs: Evaluated as low risk
src/Core/src/Platform/iOS/IMauiPlatformView.cs: Evaluated as low risk

Comments suppressed due to low confidence (3)

src/Controls/tests/TestCases.HostApp/Issues/Issue26629.cs:22

[nitpick] The variable name 'i' is ambiguous. It should be renamed to 'labelCounter'.

var i = 0;

src/Core/src/Platform/iOS/ViewExtensions.cs:288

Ensure that the new behavior introduced by the InvalidateMeasure method is covered by tests.

public static void InvalidateMeasure(this UIView platformView, IView view)

src/Core/src/Platform/iOS/ViewExtensions.cs:308

Ensure that the new behavior introduced by the InvalidateAncestorsMeasures method is covered by tests.

internal static void InvalidateAncestorsMeasures(this UIView child)

rmarinho · 2024-12-18T11:32:51Z

/azp run

azure-pipelines · 2024-12-18T11:33:14Z

Azure Pipelines successfully started running 3 pipeline(s).

rmarinho · 2024-12-19T17:05:43Z

/azp run

azure-pipelines · 2024-12-19T17:06:08Z

Azure Pipelines successfully started running 3 pipeline(s).

jsuarezruiz · 2025-03-03T10:40:26Z

/azp run

azure-pipelines · 2025-03-03T10:40:46Z

Azure Pipelines successfully started running 3 pipeline(s).

mattleibow · 2025-03-03T13:31:17Z

Is this image change expected? Also, the after still looks wrong because if the image button has no height, then why is the image small? The image was always small, but now it is less weird with all the space. Still wrong, but better wrong?

albyrock87 · 2025-03-03T13:52:46Z

Is this image change expected? Also, the after still looks wrong because if the image button has no height, then why is the image small? The image was always small, but now it is less weird with all the space. Still wrong, but better wrong?

@mattleibow this is the Windows one, so it looked correct to me.

The image has no height, true, but it also has WidthRequest="50" where the default aspect is AspectFit, so that's why the image is small, because it keeps the original aspect while matching the requested width.

PureWeen

For the PageView type can we just do something possibly a little silly until we address the "potential improvement" comment? Like, just add an internal property on MauiView that can be used to do the same thing? My worry here is that in some cases users are probably inheriting from ContentView and providing their own type for the PageHandler. So, if we just set something inside the SetVirtualView part of the PageHandler that enables the same short circuiting that won't break anyone. If we're going to change the type here, we'd probably want to do that through a new handler.
I wonder if we can just get rid of the InvalidateMeasure on IPlatformMeasureInvalidationController
- AFAICT the only types that benefits from this are ScrollView/CollectionView. Everyone else just calls SetNeedsLayout from this
- for ScrollView can the code that's in there just move to an override of SetNeedsLayout ?
- For collectionView we could just special case it inside "InvalidateAncestorsMeasures" for now since it is the only outlier. I would just be curious as we implement the code for optimizing propagation if this also becomes somewhat unnecessary. We're already special casing UIScrollView inside InvalidateAncestorsMeasures so once we evolve this solution a bit then we can hopefully shed these special cases a bit?

src/Core/src/Platform/iOS/ViewExtensions.cs

albyrock87 · 2025-03-04T14:11:00Z

@PureWeen

just set something inside the SetVirtualView part of the PageHandler that enables the same short circuiting

As per our conversation, this has been implemented through a new internal bool IsPage { get; set; } property on ContentView. PageView has been removed.

I wonder if we can just get rid of the InvalidateMeasure on IPlatformMeasureInvalidationController

I don't think we can do this, let me explain.

for ScrollView can the code that's in there just move to an override of SetNeedsLayout ?

UIScrollView natively triggers SetNeedsLayout a lot of times when the user scrolls, that doesn't mean we have to invalidate our measure constraint cache, so we need to differentiate between MAUI invalidation and native invalidation which is the exact root cause of #24996 issue.

AFAICT the only types that benefits from this are ScrollView/CollectionView. Everyone else just calls SetNeedsLayout from this

This made me think, so I've looked again at MauiView and you're right: I'm just calling SetNeedsLayout(), but now I think I should move InvalidateCacheConstraints from SetNeedsLayout to InvalidateMeasure so that's what I've done in the last commit.
Now I'm curious to see if this passes the tests: I have many failures locally, even on main so I can't verify them all.

PureWeen · 2025-03-04T14:15:58Z

/azp run

azure-pipelines · 2025-03-04T14:16:23Z

Azure Pipelines successfully started running 3 pipeline(s).

mattleibow

Added 2 questions.

src/Core/src/Platform/iOS/ImageViewExtensions.cs

mattleibow · 2025-03-04T16:06:09Z

src/Core/src/Platform/iOS/ImageViewExtensions.cs

+			double widthRatio = constrainedWidth / contentWidth;
+			double heightRatio = constrainedHeight / contentHeight;


To confirm what I am thinking, this is the size used to determine the size of the control, so you are including the padding in here because iOS does not have a [padding concept?

But, as @PureWeen mentioned - should it be used in the aspect ratio calculations? Maybe it should be added on after the size calculations? For example, if I have a landscape image, but massive padding on top/bottom that makes it become a portrait scale. Will it cause issues with stretch?

Maybe I am thinking wrong here?

Good call, I've updated the algorithm as follows:

Remove padding from constraints

Compute desired size on reduced constraints

Add padding back

PureWeen · 2025-03-04T17:50:12Z

/azp run

azure-pipelines · 2025-03-04T17:50:34Z

Azure Pipelines successfully started running 3 pipeline(s).

Changes Made

albyrock87 requested review from jfversluis, jsuarezruiz and a team as code owners December 14, 2024 10:14

dotnet-policy-service bot added the community ✨ label Dec 14, 2024

MartyIX reviewed Dec 14, 2024

View reviewed changes

src/Controls/tests/DeviceTests/Elements/CarouselView/CarouselViewTests.cs Show resolved Hide resolved

albyrock87 force-pushed the ios-set-neesd-layout-propagation-mapper branch from 2ad0ab2 to 5b5e4b9 Compare December 14, 2024 11:54

jfversluis added the area-layout label Dec 16, 2024

rmarinho reviewed Dec 16, 2024

View reviewed changes

src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt Show resolved Hide resolved

rmarinho previously requested changes Dec 17, 2024

View reviewed changes

src/Core/src/Platform/iOS/MauiScrollView.cs Outdated Show resolved Hide resolved

samhouts requested a review from Copilot December 17, 2024 18:00

Copilot AI reviewed Dec 17, 2024

View reviewed changes

src/Core/src/Platform/iOS/ViewExtensions.cs Show resolved Hide resolved

samhouts requested a review from Copilot December 17, 2024 18:05

Copilot AI reviewed Dec 17, 2024

View reviewed changes

albyrock87 force-pushed the ios-set-neesd-layout-propagation-mapper branch from 21dd5bf to e861dc4 Compare December 17, 2024 19:38

PureWeen self-assigned this Dec 18, 2024

PureWeen added this to the .NET 9 SR3 milestone Dec 18, 2024

albyrock87 force-pushed the ios-set-neesd-layout-propagation-mapper branch 6 times, most recently from 1967ec8 to ce2cc4b Compare December 19, 2024 14:27

mattleibow previously approved these changes Mar 3, 2025

View reviewed changes

PureWeen requested changes Mar 3, 2025

View reviewed changes

PureWeen reviewed Mar 3, 2025

View reviewed changes

src/Core/src/Platform/iOS/ViewExtensions.cs Outdated Show resolved Hide resolved

albyrock87 added 3 commits March 4, 2025 10:41

Standardize the way we invalidate measure on iOS

63b56d6

Updated screenshot

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

047e973

Remove PageView in favor of tagging ContentView as a page

28e4710

albyrock87 dismissed mattleibow’s stale review via 28e4710 March 4, 2025 10:14

albyrock87 force-pushed the ios-set-neesd-layout-propagation-mapper branch from aef3942 to 28e4710 Compare March 4, 2025 10:14

Remove SetNeedsLayout overrides

2d1169c

mattleibow reviewed Mar 4, 2025

View reviewed changes

Fix image view extensions padding computation

Loading
Loading status checks…

d1408f4

PureWeen approved these changes Mar 4, 2025

View reviewed changes

PureWeen merged commit 179c8c4 into dotnet:main Mar 5, 2025
122 of 124 checks passed

albyrock87 mentioned this pull request Mar 6, 2025

Improve iOS CollectionView performance by leveraging the new platform level invalidation mechanism #28225

Merged

bhavanesh2001 pushed a commit to bhavanesh2001/maui that referenced this pull request Mar 7, 2025

Change iOS SetNeedsLayout propagation mechanism (dotnet#26629)

da1eba0

NirmalKumarYuvaraj mentioned this pull request Mar 11, 2025

Fixed ScrollView inside a Grid expands width past device screen when Rotated #27839

Closed

4 tasks

rmarinho pushed a commit that referenced this pull request Mar 11, 2025

Change iOS SetNeedsLayout propagation mechanism (#26629)

0d4bc79

APopatanasov mentioned this pull request Mar 14, 2025

[iOS] [MacCatalyst] Provide an API through which SetNeedsLayout can be intercepted when InvalidateMeasure is invoked #28410

Open

This was referenced Mar 16, 2025

Bring back iOS SetNeedsLayout propagation via inheritance #28434

Closed

Stop iOS SetNeedsLayout propagation by looking at VisualElement computed Constraint #28479

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change iOS SetNeedsLayout propagation mechanism #26629

Change iOS SetNeedsLayout propagation mechanism #26629

albyrock87 commented Dec 14, 2024 •

edited

Loading

PureWeen commented Dec 14, 2024

azure-pipelines bot commented Dec 14, 2024

PureWeen commented Dec 15, 2024

azure-pipelines bot commented Dec 15, 2024

rmarinho left a comment

Copilot AI left a comment

Copilot AI left a comment

rmarinho commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

rmarinho commented Dec 19, 2024

azure-pipelines bot commented Dec 19, 2024

jsuarezruiz commented Mar 3, 2025

azure-pipelines bot commented Mar 3, 2025

mattleibow commented Mar 3, 2025

albyrock87 commented Mar 3, 2025

PureWeen left a comment

albyrock87 commented Mar 4, 2025

PureWeen commented Mar 4, 2025

azure-pipelines bot commented Mar 4, 2025

mattleibow left a comment

mattleibow Mar 4, 2025

albyrock87 Mar 4, 2025

PureWeen commented Mar 4, 2025

azure-pipelines bot commented Mar 4, 2025

		double widthRatio = constrainedWidth / contentWidth;
		double heightRatio = constrainedHeight / contentHeight;

Change iOS SetNeedsLayout propagation mechanism #26629

Change iOS SetNeedsLayout propagation mechanism #26629

Conversation

albyrock87 commented Dec 14, 2024 • edited Loading

Description of Change

Issues Fixed

PureWeen commented Dec 14, 2024

azure-pipelines bot commented Dec 14, 2024

PureWeen commented Dec 15, 2024

azure-pipelines bot commented Dec 15, 2024

rmarinho left a comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

rmarinho commented Dec 18, 2024

azure-pipelines bot commented Dec 18, 2024

rmarinho commented Dec 19, 2024

azure-pipelines bot commented Dec 19, 2024

jsuarezruiz commented Mar 3, 2025

azure-pipelines bot commented Mar 3, 2025

mattleibow commented Mar 3, 2025

albyrock87 commented Mar 3, 2025

PureWeen left a comment

Choose a reason for hiding this comment

albyrock87 commented Mar 4, 2025

PureWeen commented Mar 4, 2025

azure-pipelines bot commented Mar 4, 2025

mattleibow left a comment

Choose a reason for hiding this comment

mattleibow Mar 4, 2025

Choose a reason for hiding this comment

albyrock87 Mar 4, 2025

Choose a reason for hiding this comment

PureWeen commented Mar 4, 2025

azure-pipelines bot commented Mar 4, 2025

albyrock87 commented Dec 14, 2024 •

edited

Loading