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] CollectionView item sizing wrong after refresh #21736

Merged

Conversation

softeip
Copy link
Contributor

@softeip softeip commented Apr 9, 2024

Description of Change

Added reset of EstimatedItemSize and ItemSize when the size cache gets cleared

Issues Fixed

Fixes #20443

@softeip softeip requested a review from a team as a code owner April 9, 2024 20:27
@softeip softeip requested review from mattleibow and rmarinho April 9, 2024 20:27
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Apr 9, 2024
@softeip
Copy link
Contributor Author

softeip commented Apr 9, 2024

@dotnet-policy-service agree

@jsuarezruiz jsuarezruiz added platform/iOS 🍎 area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Apr 10, 2024
@softeip
Copy link
Contributor Author

softeip commented May 15, 2024

What are the next steps for this PR?

@rmarinho
Copy link
Member

/rebase

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

Can you try this with the main rebased ? It would use the new handler for collectionview2 . Also can we add a UITest? you can look at other examples with VerifyScreenshot.

@rmarinho rmarinho force-pushed the fix/CollectionViewItemWrongSizeAfterRefresh branch 2 times, most recently from 4e36d3f to 0566fa9 Compare December 11, 2024 10:23
Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

Doesn't seem to fix #24961 and I think we ca drop the ItemSize

@dotnet dotnet deleted a comment from azure-pipelines bot Dec 11, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Dec 11, 2024
@rmarinho rmarinho self-assigned this Dec 11, 2024
@rmarinho rmarinho self-requested a review December 11, 2024 14:21
@rmarinho rmarinho added this to the .NET 9 SR3 milestone Dec 11, 2024
@softeip
Copy link
Contributor Author

softeip commented Dec 11, 2024

@rmarinho I was able to reproduce this issue with my app on 8.0.92 - I had different size views in the CollectionView and tried to refresh the collection several times and scrolling up and down. At some point I was able to reproduce the issue. I believe there are some steps missing on the way to reproduce this one for sure.

Then I updated to .NET 9 and MAUI 9.0.12 and I was able to reproduce that again. So new handler does not seem to have a fix.
Here is how it looks like.
Simulator Screenshot - iPhone 15 - 2024-12-11 at 18 11 01

@rmarinho
Copy link
Member

@softeip are you using CollectionViewHandler2 ?

@softeip
Copy link
Contributor Author

softeip commented Dec 11, 2024

@rmarinho No, my bad. Added CollectionViewHandler2 manually and retested. Now I see that the issue is not reproducable no matter what I do and have many times I refresh the collection. I would say that the new handler fixes the sizing issue at least for my app.

@rmarinho
Copy link
Member

@rmarinho No, my bad. Added CollectionViewHandler2 manually and retested. Now I see that the issue is not reproducable no matter what I do and have many times I refresh the collection. I would say that the new handler fixes the sizing issue at least for my app.

Nice, yeah that matches my testing, but this will fix using the CollectionView 1 and is still the default one.

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

@rmarinho rmarinho force-pushed the fix/CollectionViewItemWrongSizeAfterRefresh branch from 352c588 to c4f7096 Compare December 13, 2024 15:42
@dotnet dotnet deleted a comment from azure-pipelines bot Dec 13, 2024
@rmarinho
Copy link
Member

OK after playing with it seems the issue is setting EstimatedSize when the CollectionView ItemsSource is disposed is the problem, we actually want clear that when our constrains change in case of the refresh for example.

@@ -113,6 +113,8 @@ internal virtual bool UpdateConstraints(CGSize size)

ClearCellSizeCache();

EstimatedItemSize = CGSize.Empty;
Copy link
Member

Choose a reason for hiding this comment

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

Do we use empty specifically to know we need to estimate the size?

Just wanting to make sure we shouldn't be using UICollectionViewFlowLayout.AutomaticSize which has a specific meaning (and is not equivalent to Empty). I am guessing we want Empty here since we do the manually measurement on CV1 still, but wanted to be sure.

Copy link
Contributor Author

@softeip softeip Dec 13, 2024

Choose a reason for hiding this comment

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

Looks like no, we should not use automatic size.
There is a call to ConstrainTo() method which is implemented in the ItemsViewLayout subclasses. Their implementation calls DetermineCellSize() which have some checks for EstimatedItemSize == CGSize.Empty. There is a comment saying about <iOS 10 fix (237 line in this file). Using UICollectionViewFlowLayout.AutomaticSize can lead to crashes and would require further fixes.

@rmarinho rmarinho merged commit d999d56 into dotnet:main Dec 16, 2024
104 checks passed
@softeip softeip deleted the fix/CollectionViewItemWrongSizeAfterRefresh branch December 16, 2024 18:44
@samhouts samhouts added the fixed-in-net8.0-nightly This may be available in a nightly release! label Dec 16, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jan 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView community ✨ Community Contribution fixed-in-net8.0-nightly This may be available in a nightly release! platform/iOS 🍎
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[iOS] CollectionView item sizing wrong after refresh
5 participants