Repository navigation
fix(virtual-list): keep the focus snap when a scroll request echoes it - #152
Merged
Merged
Conversation
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.
Home rows rested at different tops going Up than going Down in the Plex app (row 8 at 496, row 6 at 532, row 2 at 496, all 520 on the way down). 24 px of drift, 12 px where the app's preview expansion is partial.
What happens: on every Lightning focus move the app calls scrollToIndex for the row that VirtualList just scrolled to itself, 15-30 ms later with the same target (ScrollIntoViewHelper in the app runs on Lightning because IS_WEB is true there). scrollToOffset drops the focus anchor before the duplicate-target guard turns the call into a no-op, so keepFocusedItemSnapped switches off, and scrollToIndex also stores a fixed index intent that re-applies a stale offset on the next maxScroll change. When the scroll animation ends the batched sizes drain, a row above the focused one resizes (928 to 904 px for the preview row), the focused cell's offset moves and nothing resnaps.
Fix: scrollToOffset keeps the anchor when the requested offset is within 1 px of where the anchor already snaps the list, and scrollToIndex doesn't set the stale index intent in that case. Any other target still releases the anchor. The app doesn't need to change.
Verified: unit test (useScrollHandler.spec.tsx, uses createTestRenderer) fails without the fix and also without only the anchor half, plus a test that a scrollToIndex to another row still releases the anchor. In the app (headless Chrome, patched dist on top of the stable ref handle fix): a settled Down/Up sweep rests all rows at 520 both ways, baseline had 8@496 6@532 3@496 1@532 on the way up. One animated 24 px resnap after the drain, no fight with the intent. Haven't re-run the timed 700/150 ms repro with the fix, only the settled sweep.