refactor(scroll): content anchor as the single source of truth for restore#774
Merged
Conversation
…for restore Remove the exact-saved-scrollTop fast-path from restoreSavedPosition so the content anchor (message id + fraction, re-derived from each row's CURRENT measured height) governs every restore. Font-size, view-density and viewport -width changes are now handled by the same correct path instead of being caught only incidentally by a total-height delta; the saved scrollTop is demoted to a last-resort fallback used only when there is no usable anchor. - Drop the now-dead stored scrollHeight/clientWidth from ScrollState and remove getSavedScrollHeight/getSavedClientWidth; saveScrollPosition keeps scrollHeight/clientHeight only as transient inputs for the wasAtBottom decision. - Rewrite the affected virtualized restore unit tests to assert the restore CONSULTS the anchor (getOffsetForMessageId / scrollToIndex) rather than a pixel. - Add scroll-invariants invariant-12: a width + density change while away holds the reading anchor on return (chromium).
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
1 participant
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.
Makes the message-list scroll restore rely on the rendering-independent content anchor (
{ messageId, fraction }, re-derived from each row's current measured height) as the single source of truth, and removes the exact-saved-scrollTop "fast-path".Why
The old fast-path restored the saved
scrollTopdirectly when the savedscrollHeight(±4px) andclientWidthwere unchanged. That pixel proxy only stood in for "layout byte-identical" — fragile. Font-size and view-density changes were handled only incidentally (caught by the total-height delta), and a relayout that left the height coincidentally unchanged could mis-fire onto a stale pixel. The fraction anchor is correct in every case, including identical layout.Changes
restoreSavedPosition: remove the fast-path; the anchor is tried first and unconditionally. SavedscrollTopis demoted to a last-resort fallback used only when there is no usable anchor. (Integrates with main'spinVirtualizedAnchor/requestAnchorAroundLoad.)scrollStateManager: drop the now-dead storedscrollHeight/clientWidthand thegetSavedScrollHeight/getSavedClientWidthgetters;scrollHeight/clientHeightremain only as transient inputs for thewasAtBottomdecision.getOffsetForMessageId/scrollToIndex) rather than a saved pixel — the fraction can't be unit-tested in jsdom (no layout).scroll-invariantsinvariant-12: a viewport-width + view-density change while away holds the reading anchor on return (does not snap to bottom; same message stays at the fold).Verification
npm run typecheck✅FloatingDateHeaderflake, confirmed on clean main)npm run test:scroll(chromium): 22 passed