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

Make MeasureInvalidated event work correctly #23052

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

albyrock87
Copy link
Contributor

@albyrock87 albyrock87 commented Jun 14, 2024

Description of Change

MeasureInvalidated event was working fine on Xamarin, while on MAUI we basically forgot about bubbling up the event.

This causes the issues below where the system relays on this event to do something, like resizing the cell of a collection view.

This PR brings back the MeasureInvalidated event trigger on parents.

I've adjusted Xamarin components like Layout and Page to leverage the new internal method I put on VisualElement.

I've re-enabled the collection view testing gallery regarding dynamic sizing of elements in collection view.

I don't have much time available right now, so if you need to add more tests, feel free to do it.

Issues Fixed

Fixes #15177
Fixes #20181
Fixes #22978

@albyrock87 albyrock87 requested a review from a team as a code owner June 14, 2024 10:39
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Jun 14, 2024
@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@albyrock87
Copy link
Contributor Author

@rmarinho are those failed test environment issues or are those caused by my chargers?
If it's the latter, where can I see some detail about the failures?

@rmarinho
Copy link
Member

Hi @albyrock87 seems we might have a issue with this test failing ..

ChildPagesDoNotLeak

Reference to Microsoft.Maui.Controls.ContentPage (type Microsoft.Maui.Controls.ContentPage is still alive.\n\n---- Assertion timed out

@albyrock87 albyrock87 force-pushed the issue-15177-fix branch 4 times, most recently from b86d3f4 to 583aad9 Compare June 15, 2024 08:21
@albyrock87
Copy link
Contributor Author

@rmarinho thanks for the help, I've changed the code to account for the fact that usually child elements don't get removed: I mean, the dev pops the page from the stack and that's it.

So basically I'm manually bubbling up the event from the child element to the parent.
I've ran the device tests locally and they're now passing.

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.

Things to review before merging

  • can we achieve this through handlers vs events. Measure invalidated fires through command mapper so we could propagate that way which would be more core compliant

  • should we remove all the superview.setneedslayout calls from the iOS core code, with these changes that code shouldn't be needed.

@albyrock87
Copy link
Contributor Author

@PureWeen I wanted to have a minimal impact with these changes to simply fix the bugs, but if that's the direction to take, I can refactor this to make it happen (also removing the SetNeedsLayout chain).

@albyrock87
Copy link
Contributor Author

albyrock87 commented Jun 18, 2024

After a bit of research this is what I can tell:

  1. MeasureInvalidated event is needed (also within Shell) to tell whether an immediate descendant has potentially changed its measure
  2. Native component tree does not necessary map 1:1 to MAUI components (i.e. CollectionView)
  3. MeasureInvalidated is something we (almost) ported from Xamarin: I say almost because it is not being propagated to parents
  4. MeasureInvalidated is only triggered by InvalidateMeasureInternal
  5. InvalidateMeasureInternal is used by a lot of non-native consumers (i.e. Grid.RowSpacing, or ImageElement.ImageSourceChanged)
  6. We cannot get rid of MeasureInvalidated event because
    1. It is used to detect measure changes for logical descendants (see for example Shell : flyoutHeader.MeasureInvalidated) which has potentially (almost) nothing to share with the native tree of components
    2. It is part of a widely used API from third party libraries
  7. We are indeed doing SetNeedsLayout on iOS recursively on the platform-tree but that fails in case a non-platform view maps to a complex native
  8. Android and WinUI don't need such recursive layout invalidation

That said, our goals (IMHO) should be:

  1. Support MeasureInvalidated event propagation
  2. Reduce as minimum as possible the MeasureInvalidated?.Invoke effective calls (aka MeasureInvalidated = null)
    1. Which means do NOT rely on MeasureInvalidated event to propagate the information
  3. Platform-Level code should NOT be the one navigating a non-platform hierarchy

For the above mentioned reasons I think that the approach of my PR is correct, and it's probably the only achievable choice for a SR release where we don't want to change public APIs.

For .NET9, we could:

  1. Add a IView.RequestLayout(RequestLayoutReason) command, which will be invoked from OnChildMeasureInvalidatedInternal
  2. the IView.RequestLayout command mapper will invoke SetNeedsLayout on iOS and do nothing on other platforms

This also gives us the possibility to be smart (i.e. on ScrollView) and avoid propagating events and SetNeedsLayout above.

@PureWeen
Copy link
Member

/rebase

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@danN59
Copy link

danN59 commented Jul 22, 2024

Hi, when will this be merge ?

@PureWeen
Copy link
Member

/azp run

@albyrock87
Copy link
Contributor Author

@billylo1 the fix will be available in SR9

@samhouts samhouts added fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Sep 5, 2024
@samhouts samhouts added fixed-in-9.0.0-rc.2.24503.2 and removed fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Oct 14, 2024
filipnavara added a commit to filipnavara/maui that referenced this pull request Oct 16, 2024
…sureInvalidatedBehavior app context switch
filipnavara added a commit to filipnavara/maui that referenced this pull request Oct 16, 2024
…sureInvalidatedBehavior app context switch
filipnavara added a commit to filipnavara/maui that referenced this pull request Oct 17, 2024
…sureInvalidatedBehavior app context switch
PureWeen pushed a commit to filipnavara/maui that referenced this pull request Oct 30, 2024
…sureInvalidatedBehavior app context switch
PureWeen added a commit that referenced this pull request Oct 31, 2024
* Revert PR #23052 behavior under new Microsoft.Maui.UseLegacyMeasureInvalidatedBehavior app context switch

* Align the switch name with .NET 9 switches

---------

Co-authored-by: Filip Navara <[email protected]>
github-actions bot pushed a commit that referenced this pull request Nov 4, 2024
PureWeen pushed a commit that referenced this pull request Nov 5, 2024
…t switch (#25677)

* Revert PR #23052 behavior under new Microsoft.Maui.UseLegacyMeasureInvalidatedBehavior app context switch

* Align the switch name with .NET 9 switches

---------

Co-authored-by: Filip Navara <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Nov 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-general General issues that span multiple controls, or common base classes such as View or Element community ✨ Community Contribution fixed-in-9.0.0-rc.2.24503.2
Projects
None yet
7 participants