Skip to content

fix: fix maintainVisibleContentPosition on iOS when _firstVisibleView is removed#52782

Open
itsramiel wants to merge 1 commit into
react:mainfrom
itsramiel:fix/maintainVisibleContentPosition-ios
Open

fix: fix maintainVisibleContentPosition on iOS when _firstVisibleView is removed#52782
itsramiel wants to merge 1 commit into
react:mainfrom
itsramiel:fix/maintainVisibleContentPosition-ios

Conversation

@itsramiel

Copy link
Copy Markdown

Summary:

Addresses #52757

On Fabric, when the maintainVisibleContentPosition is passed with minIndexForVisible and that index is deleted, the _firstVisibleView will point to the wrong view because of Fabric's view recycling.

The behaviour observed in #52757 is that when minIndexForVisible is 0 and that item is removed, the list jumps to the end and then scrolls back all the way to the top because of autoscrollToTopThreshold since item 0 is deleted and added to the recycled view pool and the newly to render item(all the way at the bottom) dequeues that same view resulting in _firstVisibleView which was pointing to the item of index 0 now points at item of index ~203.

This was not happening in old arch(Paper) because old arch doesnt recycle views.

My fix is to validate whether _firstVisibleView is the same view we stored in _prepareForMaintainVisibleScrollPosition and to do that I thought of storing the view's tag as the id. If the tag changed then we do not need to adjust the scrolling depending on that view since that view pointer is pointing to a different view and I reset the identifiers related to it.

Changelog:

[IOS] [FIXED] - Fix maintainVisibleContentPosition on iOS when item with index equal to minIndexForVisible is removed

Test Plan:

I am open to the maintainers' suggestion on what is the best way to test this.

But here is a recording from the repro of #52757 with a patch including the changes in this pr implemented in this branch.

Screen Recordings

Without Patch With Patch
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-07-22.at.12.27.19.mp4
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-07-23.at.13.00.01.mp4

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 23, 2025
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jul 23, 2025

@cipolleschi cipolleschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this!

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this in D78804475.

@itsramiel

Copy link
Copy Markdown
Author

@cipolleschi should I be changing anything regarding the failed internal linter?

@cipolleschi

Copy link
Copy Markdown
Contributor

@itsramiel no, don't worry... I just have to run the internal linter, which is different from the one in OSS, unfortunately, and we couldn't find a way to unify the two because internally we use a modified version of Clang... 😓

Anyway, just to set expectation, I have been asked to test the change on a few internal apps, so it will take me a little of time before landing this as I hve to manually run those tests.. 😅

@itsramiel

Copy link
Copy Markdown
Author

Hi @cipolleschi. Sorry to annoy you, but any chance this moves on?

@mhyassin

Copy link
Copy Markdown

Would love to see this merged 💯

@greg-schrammel

Copy link
Copy Markdown

this fix seems to be in, only checked 0.83.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants