Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Invalidate CollectionView Layout on transition to IsVisible = true #13370

Merged
merged 4 commits into from
Jan 15, 2021

Conversation

hartez
Copy link
Contributor

@hartez hartez commented Jan 12, 2021

Description of Change

If the CollectionView is the root Content element for a page on iOS and IsVisible transitions from false to true, the CollectionView's layout is not invalidating. This change invalidates the layout so that the CollectionView is refreshed.

Note that this isn't an issue if the CollectionView is inside some other view (e.g., a StackLayout); it's only an issue if it's the root of the page Content.

Issues Resolved

API Changes

None

Platforms Affected

  • iOS

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Automated UI test

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@hartez hartez added this to the 5.0.0 milestone Jan 12, 2021
@hartez hartez added a/collectionview blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. p/iOS 🍎 4.8.0 regression on 4.8.0 i/regression labels Jan 12, 2021
@hartez hartez added this to To do in vNext+1 (5.0.0) via automation Jan 12, 2021
@hartez hartez moved this from To do to Blockers in vNext+1 (5.0.0) Jan 12, 2021
@rmarinho
Copy link
Member

Are you missing a button with "RunTest" @hartez ?

@hartez
Copy link
Contributor Author

hartez commented Jan 12, 2021

Are you missing a button with "RunTest" @hartez ?

Yep, I forgot I changed the test to use Appearing. :(

@AlleSchonWeg
Copy link
Contributor

Note that this isn't an issue if the CollectionView is inside some other view (e.g., a StackLayout); it's only an issue if it's the root of the page Content.

I have the same issue, but my CollectionView is inside some other view.

@rmarinho rmarinho moved this from Blockers to In Review in vNext+1 (5.0.0) Jan 12, 2021
@hartez
Copy link
Contributor Author

hartez commented Jan 12, 2021

Note that this isn't an issue if the CollectionView is inside some other view (e.g., a StackLayout); it's only an issue if it's the root of the page Content.

I have the same issue, but my CollectionView is inside some other view.

@AlleSchonWeg I should clarify - it's not an issue if the container view is one that will be invalidated if one of its children changes its size or visibility. StackLayout, Grid, FlexLayout, etc. all recompute their layouts if a child control's visibility changes. There may be other containers which do not. I just included that note for folks who need an immediate workaround; wrapping the CollectionView in a StackLayout or Grid will fix the issue temporarily while waiting for the next release.

It's all moot after this change is merged, because the CollectionView itself will invalidate its own layout after a visibility change. So it shouldn't matter what type the container is.

@hartez
Copy link
Contributor Author

hartez commented Jan 13, 2021

Does this need to be unregistered? Wouldn't want a new memory leak.

It'd be unusual, but I suppose a situation where the user is holding on to ItemsView and reusing it in another renderer could leak the controller. Added the unsubscribe during Dispose.

Copy link
Contributor

@rachelkang rachelkang left a comment

Choose a reason for hiding this comment

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

Are these failing tests relevant?

image

@PureWeen
Copy link
Contributor

@rachelkang those failures are not relevant

@samhouts samhouts added a/binding ⛓ i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often t/bug 🐛 labels Jan 15, 2021
@rmarinho rmarinho merged commit c419953 into 4.8.0 Jan 15, 2021
vNext+1 (5.0.0) automation moved this from In Review to Done Jan 15, 2021
@rmarinho rmarinho deleted the fix-13203 branch January 15, 2021 10:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4.8.0 regression on 4.8.0 a/binding ⛓ a/collectionview blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often i/regression p/iOS 🍎 t/bug 🐛
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants