Repository navigation
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Same kind of problem as in #58985.
ViewabilityHelper.dispose()clears the pendingminimumViewTimetimers but keeps_viewableIndices.onUpdatestores the new indices before it starts the timer, so after adispose()the helper thinks those items were already handled. Every followingonUpdatewith the same indices runs into the "nothing changed" early return andonViewableItemsChangednever fires for them. It only gets unstuck once the set of viewable items changes, e.g. by scrolling far enough.That didn't matter as long as
dispose()was only called on an instance that was about to go away. With StrictMode (in dev) and<Activity>the sameVirtualizedListinstance getscomponentWillUnmountand thencomponentDidMountagain, and it keeps its helpers. So if a list gets hidden while aminimumViewTimetimer is running, the items that just became viewable are never reported after it's shown again.The fix is one line,
dispose()now also callsresetViewableIndices()so the next update looks at the items again. That can't lead to duplicate callbacks because_onUpdateSyncstill diffs against_viewableItemsand only reports real changes. There's a test for that as well.One thing this doesn't do: the list doesn't re-run viewability by itself when it comes back. The report happens on the next scroll, cell layout or render batch. I didn't want to change mount behavior in the same PR.
Changelog:
[GENERAL] [FIXED] - Fix onViewableItemsChanged not firing for items whose minimumViewTime timer was cancelled by a StrictMode/Activity remount
Test Plan:
Two new tests in
ViewabilityHelper-test.js.minimumViewTime reports items again after disposefails on main and passes with the fix:does not report already viewable items again after disposepasses before and after. It's there to make sure the reset doesn't cause duplicate callbacks.I also tried it with a real
VirtualizedList(not part of the PR):viewabilityConfig={{minimumViewTime: 1000, itemVisiblePercentThreshold: 50}}, hide it with<Activity mode="hidden">while the timer is pending, show it again, send another scroll event. On mainonViewableItemsChangedis never called, with the fix it's called once. Same result with the StrictMode double mount when the first update lands before the simulated unmount.eslintandprettier --checkare clean for both files.