fix: clear temporary web scroll padding after the browser re-serializes it - #538
Open
badcuban wants to merge 1 commit into
Open
fix: clear temporary web scroll padding after the browser re-serializes it#538badcuban wants to merge 1 commit into
badcuban wants to merge 1 commit into
Conversation
…es it
ScrollAdjust pads the content container so a scroll adjustment can move
past content that has not grown yet, then removes the padding on the next
animation frame only if the inline style still equals the string it wrote.
Browsers re-serialize CSS lengths to about six significant digits and drop
float noise ("607.46875px" reads back as "607.469px",
"258.15000000000003px" as "258.15px"), so the comparison never matched and
the padding stayed forever: permanent blank scrollable space below the
last item that getContentSize() knows nothing about. Fractional scroll
offsets on non-integer device pixel ratios make this common.
Record the value the browser actually stored after the write so the
frame-later comparison is like for like. The regression test covers the
six-digit truncation route; the same comparison also covers float noise.
Fixes LegendApp#537
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
badcuban
added a commit
to Threadlines/threadlines
that referenced
this pull request
Sep 1, 2026
The release smoke job deletes pnpm-lock.yaml and resolves the workspace from scratch. With "^3.3.9", that now picks 3.3.10 (published today), and pnpm fails with ERR_PNPM_UNUSED_PATCH because our patch is keyed to 3.3.9. Pin the exact version the patch targets; bump both together when upstream ships the fix (LegendApp/legend-list#538).
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.
Problem
On web,
ScrollAdjustpads the content container so a scroll adjustment can move past content that has not grown yet, then reverts the padding on the next animation frame only if the inline style still equals the string it wrote:Browsers re-serialize CSS lengths, keeping about six significant digits and dropping float noise:
So the guard never matches and the padding stays forever. Users see permanent blank scrollable space below the last item, and
scrollToEndlands above it becausegetContentSize()does not include the padding. Fractional scroll offsets on non-integer device pixel ratios (a 125% Windows laptop, for example) make this frequent. A chat-style list withmaintainScrollAtEndandmaintainVisibleContentPositionhits it within a few row updates.Fixes #537. Reported by @szado; @chrisnojima's patch-package workaround in that thread takes the same approach.
Fix
Record the value the browser actually kept after the write, so the frame-later comparison is like for like. This covers both routes to the mismatch (six-digit truncation and float noise) without guessing which values round-trip.
Repro
In a Chromium app at
devicePixelRatio1.25, pin a vertical list at the end withinitialScrollAtEnd,maintainScrollAtEndandmaintainVisibleContentPosition, then let rows near the end grow and shrink (tool-call rows in a chat transcript). Within a few updatesgetComputedStyle(contentContainer).paddingBottomis left at a few hundred px and the list scrolls into empty space. Captured in a real app: 140px and 406px of stuck padding while every row's position and size were correct.Tests
__tests__/components/ScrollAdjust.web.test.tsuses a style stand-in that re-serializes to six significant digits like a browser. It fails onmain(Expected: "607px",Received: "607.469px") and passes with the fix.bun test __tests__/components/ScrollAdjust.web.test.ts: 13 pass, 0 fail.bun run lint,bun run tsc:src,bun run build: pass.bun teston my Windows machine (bun 1.3.14) reports 247 failures on untouchedmain, allTypeError: undefined is not an object (evaluating 'listeners.get')acrossscrollToIndex,requestAdjust,findAvailableContainersand others, including the existingScrollAdjust (web)cases. With this branch it is 248: the only difference is the new case, which fails there the same way the existing ones do and passes when the file runs alone. I could not get a clean full run in this environment, so please run it in yours.No changelog edit:
prep-changeloglooks like the release-time script.