Repository navigation
fix(BottomSheet): a render reads the stored viewport height - #10363
Conversation
BottomSheetPanel read window.innerHeight in an inline useSyncExternalStore snapshot. Its identity changed on every render, so React re-read it in the commit after the DOM writes, and the browser recalculated style and layout in the tap's handler. The panel now keeps the layout viewport's height in a module-level store. One resize listener updates it: the first open sheet adds it and the last removes it. React gets stable module functions for the snapshot and the server snapshot, so a render reads the stored value and the commit reads nothing. The value and its updates are today's: innerHeight, at the first render and on each resize. In Chromium and on the iOS Simulator the tap's first frame is unchanged: the frame then does the same recalculation.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| DateTimeInput | 196.3 kB (+122 B) · 18.1 kB CSS (0) | — | — · — · — |
| PowerSearch | 272.3 kB (+105 B) · 23.1 kB CSS (0) | — | — · — · — |
| DateInput | 173.8 kB (+88 B) · 16.4 kB CSS (0) | — | — · — · — |
22 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| TimeInput | 150.6 kB (+85 B) · 15.5 kB CSS (0) | — | — · — · — |
| BottomSheet | 59.5 kB (+77 B) · 7.5 kB CSS (0) | — | ✓ · ✓ · — |
| ContextMenu | 90.6 kB (+1 B) · 16.6 kB CSS (0) | — | — · — · — |
| lab/ChatEmojiPicker | 96.7 kB (+1 B) · 773 B CSS (0) | — | — · — · — |
| lab/CommentThread | 302.0 kB (+1 B) · 2.4 kB CSS (0) | — | — · — · — |
| BottomSheetSwitcher | 17.5 kB (0) · 3.4 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenu | 86.3 kB (0) · 16.7 kB CSS (0) | — | — · — · — |
| Markdown | 162.4 kB (0) · 19.5 kB CSS (0) | — | — · — · — |
| MoreMenu | 86.6 kB (0) · 16.7 kB CSS (0) | — | — · — · — |
| Pagination | 103.4 kB (0) · 17.3 kB CSS (0) | — | — · — · — |
| Popover | 78.7 kB (0) · 13.4 kB CSS (0) | — | — · — · — |
| Selector | 93.2 kB (0) · 16.9 kB CSS (0) | — | — · — · — |
| Table | 100.0 kB (0) · 17.3 kB CSS (0) | — | — · — · — |
| TableCell | 91.8 kB (0) · 17.0 kB CSS (0) | — | — · — · — |
| TableHeaderCell | 91.4 kB (0) · 16.8 kB CSS (0) | — | — · — · — |
| Tokenizer | 84.9 kB (0) · 18.1 kB CSS (0) | — | — · — · — |
| lab/ChatReactionBar | 97.9 kB (0) · 1.1 kB CSS (0) | — | — · — · — |
| lab/CommentComposer | 138.0 kB (0) · 1.9 kB CSS (0) | — | — · — · — |
| lab/Comments | 307.2 kB (0) · 2.5 kB CSS (0) | — | — · — · — |
| lab/TourStep | 84.9 kB (0) · 590 B CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 244.4 kB (0) · 454 B CSS (0) | — | — · — · — |
| MultiSelector | 88.8 kB (−1 B) · 16.3 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.79 MB packed (+998 B); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 19 rows, core 24 rows; run pnpm api:report.
Touched components checked; add ci:run-visual or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. It runs once the pull request leaves draft and a maintainer adds the ci:run label.
The stored viewport height removes the commit-time innerHeight read without changing the sheet: a tap in an open sheet reads it 3 times on main and 0 times here in Chromium, the new re-render test fails on main, and the reserve tests pass unchanged.
Optional: readViewportHeight stores a height during a render that may never commit (a suspending sibling, say), so the next sheet's first commit can use a stale reserve until its subscription corrects it; returning layoutViewportHeight() uncached while no listener is attached keeps the comment's "null with no sheet open" true.
[Automated review]
A render that never commits, such as one beside a suspending sibling, stored a height that no listener kept current, and the next sheet's first commit used it. A render now reads the viewport while no sheet listens, and the first subscription stores the height and lets React compare it with the one it rendered.
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. It runs once the pull request leaves draft and a maintainer adds the ci:run label.
Storing the viewport height only while the shared resize listener runs fixes the stale-reserve case from the last review: the new discarded-render test fails on the previous revision (a 360 px reserve instead of 260 px) and passes here, and the re-render test still fails on main with 3 reads.
Optional, inherited from main: handleViewportResize also runs for element resize events, such as a <video>'s, and reads innerHeight each time; an isNode(event.target) guard, as the keyboard module uses, would skip them.
[Automated review]
cixzhang
added
the
ci:run
A @Pr-a11y scope token names a component with its package, so the accessibility contracts job rejected the bare DateInput token before it ran any spec.
|
/approve-spec d8fc02e |
There was a problem hiding this comment.
Changes requested: the viewport-height store is sound, but the release note claims more than the change proves.
- Miss: the changeset's first sentence says "an open sheet that renders again reads no layout", but the change and its tests cover only the
innerHeightread and the sheet's scrolling body still measures scroll geometry after such a render (18 to 36 reads per toggle in Chromium), so please reword it to "an open sheet that renders again no longer reads the viewport height" (.changeset/bottom-sheet-viewport-height-store.md:5).
The code needs no change: the re-render test fails on main with 3 reads, a Chromium tap in an open sheet drops from 3 innerHeight reads to 0 with pixel-identical frames, and the reserve tests pass unchanged.
[Automated review]
The scrolling body still measures its scroll geometry after an open sheet renders again, as before. So the changeset says the sheet no longer reads the viewport height, not that it reads no layout.
|
/approve-spec d8c2447 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The pull request already carries ci:run, so CI runs once it is marked ready for review.
The changeset now claims only what the change proves: an open sheet that renders again no longer reads the viewport height. The re-render test fails against the base component with 3 reads and passes here, a Chromium tap in an open sheet drops from 3 innerHeight reads to 0 with pixel-identical frames and an unchanged reserve, and the BottomSheet and DateInput unit suites pass.
[Automated review]
cixzhang
added
the
ci:run-a11y
User impact
A tap that renders an open BottomSheet again, such as DateInput's month and year toggle or a checkbox in the sheet, made the sheet read
window.innerHeightin React's commit, after the commit's DOM writes. So the browser recalculated style and layout inside the tap's handler. The first frame after the tap is unchanged by this fix, because the frame then does the same recalculation (Evidence).Problem and solution fit
useSyncExternalStoresnapshot was an inline function that readinnerHeight. Its identity changed on every render, so React re-read it in the commit.resizelistener updates it: the first open sheet adds it, and the last one removes it. React gets stable module functions for the snapshot and the server snapshot. While the listener runs, a render reads the stored value and the commit reads no viewport height. With no listener, a render readsinnerHeightand stores nothing, so a render that never commits leaves no stale height.innerHeightat the first render and on eachresize. Sheet height, reserve and detents are unchanged;snapHeightsstill reads the viewport when a gesture asks.Expected behavior and authority
architecture:react-component-runtimeINV18: layout is read as little as possible, once per change. INV5: the listener has one owner and goes with the last sheet. INV9: the snapshot keeps its identity.Smallest restoration
One store in
BottomSheetPanel.tsx. No public API.Evidence
innerHeight, through React's store check. The tap readsinnerHeight2 times (the toggle) and 3 times (a checkbox).innerHeight0 times, and the click's task forces no style or layout. The click handler drops from 109–110 ms to 65–71 ms. The scrolling body still measures its scroll geometry in the next frame, as onmain.mainand on mutants that keep the listener, skip the resize refresh, readinnerHeightthrough a stable function, or store the height with no listener (a render beside a suspending sibling).Scope
Testing
Local (Linux), on
main39f2441: the fullpnpm test:node(16,962 tests, including the routing-lines check of every browser spec); coretsc; units for BottomSheet, DateInput, hooks and utils (3,321); ESLint on the changed files; Prettier;check:knowledge; the Changeset coverage check;pnpm review:pre. On a Storybook built from this head: the new read-count spec 3 of 3 in Chromium, twice (onmain's build its two tap cases fail with 2 and 3 reads); BottomSheet's a11y specs in Chromium 84 of 84 and its interaction specs 16 of 16. BottomSheet's and DateInput's specs in WebKit under the CI profile (4 CPUs,CI=1): a11y 54 passed, 2 skipped; interaction 25 passed, 15 skipped. Opening a sheet still reads the height once after its commit, in the store's first subscription, as today's open does.bottom-sheet-wide-rangetoggle (3 taps each)mainmainbottom-sheet-wide-range(n=4)default, bottom sheet (n=3)Overlap:
git merge-treeis clean with #10233 and #9776. #9777 conflicts withmainin 15 files, and this adds none. The store needs none of #10233's realm-shared values: each copy of Core reads the sameinnerHeightthrough its own listener.