fix: stop the whole page scrolling away during playback - #9
Merged
Merged
Conversation
Reported from production: open the demo, press Play, and the entire application
could be scrolled up off the screen, revealing a second screen of blank page
underneath. It got worse the longer playback ran.
Measured before the fix, on a production build: the document was 528px taller
than the viewport at 1440x900, and a real wheel event scrolled it. Reproduced at
1440x780, 1280x720 and 1512x830 too — every laptop-shaped window.
The cause is the `visually-hidden` utility. It is the usual `sr-only` recipe:
`position: absolute`, one pixel, clipped to nothing. What the recipe leaves out
is `top` and `left`, so the box sits at its *static* position. That is harmless
on a page that scrolls anyway, and wrong in a fixed-height shell, because the
tree emits one of these labels per row ("added in this commit") inside a
virtualised list — so a row a thousand pixels down the list put its box a
thousand pixels down the page. Playback made it grow because the tree grows.
`.shell` has `overflow: hidden` and is exactly `100dvh`, so it looks like it
should have contained them. It did not: an absolutely positioned box is only
clipped by an ancestor's overflow when that ancestor is itself a containing
block, and `.shell` was `position: static`.
Fixed at both ends:
- `.visually-hidden` is pinned to its containing block's origin, so its box can
never be below the fold whatever the flow around it does. Nothing using the
class is focusable or visible, so position has no other consequence.
- `.shell` is `position: relative`, so the `overflow: hidden` it already carries
actually clips — the right thing for an element whose job is to clip.
Verified: 0px of vertical overflow through a full sixteen-commit playback, in
every sub-view, in Compare and Insights, at 1440x900, 1280x720 and 1024x768, and
a wheel event over the right-hand edge moves nothing. The hidden labels are
still in the accessibility tree — tree rows still read "modified in this commit"
and the speed buttons are still named through their hidden suffix.
Short and narrow windows still scroll, which is deliberate: below 900px wide or
620px tall the pinned frame is given up rather than forcing the application into
a viewport it does not fit. There is a test for that too, so the fix cannot
quietly turn into a different bug.
The two new tests fail on the old CSS and pass on the new, which is the only
reason to trust them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This branch was successfully 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.
Reported from production: open the demo, press
Play, and the entireapplication could be scrolled up off the screen, revealing a second screen of
blank page underneath. It got worse the longer playback ran.
Reproduced
Against a production build, measuring
document.scrollHeightagainst theviewport through a playback:
Every laptop-shaped window, not an edge case.
Cause
The
visually-hiddenutility — the usualsr-onlyrecipe:With no
top/leftthe box sits at its static position. That is harmless ona page that scrolls anyway, and wrong in a fixed-height shell, because the tree
emits one of these labels per row (
"added in this commit") inside a virtualisedlist — so a row a thousand pixels down the list put its 1px box a thousand
pixels down the page. Playback made it grow because the tree grows.
.shellhasoverflow: hiddenand is exactly100dvh, so it looks like itshould have contained them. It did not, and this is the part worth remembering:
an absolutely positioned box is only clipped by an ancestor's
overflowwhenthat ancestor is itself a containing block, and
.shellwasposition: static. So the labels' containing block was the initial containing block — thepage — and they extended it.
A bisection confirmed it: hiding each candidate and re-measuring pointed at the
visually-hiddenspans, whose deepest bottom edge was 1428px in a 900px window.Fix
Both ends, because either alone would leave the trap set for the next person:
.visually-hiddenis pinned to its containing block's origin (top: 0; left: 0), so its box can never be below the fold whatever the flow around itdoes. Nothing using the class is focusable or visible — an
h3, acaptionand two
spans — so position has no other consequence..shellisposition: relative, so theoverflow: hiddenit alreadycarried actually clips. That is the right thing for an element whose job is to
clip, independently of this bug.
Verified
sub-views, with a diff open, in Compare and in Insights, at 1440×900, 1280×720
and 1024×768.
moves the page 0px.
"~ index.ts 167 B modified in this commit", and the five speed buttons arestill named through their hidden
speedsuffix.wide or 620px tall the pinned frame is given up rather than forcing the
application into a viewport it does not fit. 390×844 still scrolls 242px,
360×800 still scrolls 286px.
Three tests added. The two that assert the page cannot scroll fail on the old
CSS and pass on the new — checked by reverting the fix and re-running, which is
the only reason to trust them. The third asserts the narrow layout still scrolls,
so the fix cannot quietly turn into the opposite bug.
npm run typechecknpm run lintnpm testnpm run buildnpx playwright test🤖 Generated with Claude Code