Repository navigation
fix(BottomSheet): a set-height sheet keeps its content padding in WebKit's scroll range - #8557
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the min-height: fit-content change works in WebKit (a capped sheet with content placed directly in it now ends 16 px below its last line instead of 0 px, and Chromium is unchanged), but two points need addressing before approval.
- The flex-row dialog host carried from #8534 contradicts the current
component:BottomSheetrecord, whose element jobs say the dialog holds the panel "with its own grid"; please amend that line through the spec gate (an engineering owner can approve it with/approve-spec <head SHA>) or keep the grid. - Both changesets claim Safari fixes, but the readings come from Playwright WebKit, and
spec:AST-013FR10 asks for evidence from Safari itself; please add iOS Safari readings (including one behind the "about 40 ms" relayout figure) or narrow the wording to what was measured.
[Automated review]
| display: 'grid', | ||
| alignContent: 'end', | ||
| justifyItems: 'center', | ||
| display: 'flex', |
There was a problem hiding this comment.
This makes the open dialog a flex row, but the current component:BottomSheet record (Performance and resources, element jobs) says the dialog "holds the panel at the block end with its own grid". Please amend that line through the spec gate or keep the grid host; the layout itself is unchanged in Chromium and WebKit.
There was a problem hiding this comment.
The flex row is no longer part of this pull request: it now sits on main's grid host, where the fix and its guard pass in both engines. Whether the row stays is #8534's own owner question.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] BottomSheet: in Safari, a sheet at a set height whose content sits directly in it keeps its bottom padding at the end of its scroll, instead of ending flush with the last line. |
There was a problem hiding this comment.
The fix holds in Playwright WebKit (0 px to 16 px at the scroll end of a capped direct-content sheet), but spec:AST-013 FR10 says Playwright WebKit does not prove Safari behavior. Please add an iOS Safari reading before and after, or describe the fix in terms of what was measured.
There was a problem hiding this comment.
Narrowed in 9bfb74a to what was measured, Playwright's WebKit. iOS Safari readings before and after are being taken, and the wording will name Safari once they land.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] BottomSheet: in Safari, an update inside an open sheet no longer holds up the page about 40 ms longer than it should. The open sheet's dialog places its panel with a flex row instead of a grid; the layout is the same. |
There was a problem hiding this comment.
The "about 40 ms" Safari figure needs a measurement from Safari itself under spec:AST-013 FR10; the commit messages report it from a phone-sized WebKit page.
github-actions
Bot
added
the
needs:spec-owner-review
9bfb74a to
982a837
Compare
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Recommending approval: switching the BottomSheet content box to min-height: fit-content keeps its bottom padding inside the scroll range of a set-height sheet with direct content in WebKit (0 px to 16 px at the scroll end), with no change in Chromium. The new scroll-end guard fails on the base in WebKit and passes on this head, and the dialog-host and Safari-wording points raised on the earlier revision are resolved. Firefox 151 still ends such a sheet flush, unchanged from main and outside this fix.
[Automated review]
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| lab/ChatEmojiPicker | 65.1 kB (+1 B) · 773 B CSS (0) | — | — · — · — |
| lab/Comments | 232.6 kB (+1 B) · 2.5 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 222.3 kB (+1 B) · 454 B CSS (0) | — | — · — · — |
24 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| BottomSheetSwitcher | 15.0 kB (0) · 3.4 kB CSS (0) | — | ✓ · ✓ · — |
| MultiSelector | 72.8 kB (0) · 15.2 kB CSS (0) | — | — · — · — |
| Pagination | 86.2 kB (0) · 16.0 kB CSS (0) | — | — · — · — |
| Selector | 77.2 kB (0) · 15.6 kB CSS (0) | — | — · — · — |
| TableHeaderCell | 75.9 kB (+1 B) · 15.5 kB CSS (−1 B) | — | — · — · — |
| lab/CommentComposer | 102.5 kB (0) · 1.9 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 227.2 kB (0) · 2.4 kB CSS (0) | — | — · — · — |
| lab/MobileTokenizer | 101.1 kB (0) · 880 B CSS (0) | — | — · — · — |
| lab/TourStep | 55.5 kB (0) · 590 B CSS (0) | — | — · — · — |
| TableCell | 76.3 kB (0) · 15.8 kB CSS (−1 B) | — | — · — · — |
| lab/ChatReactionBar | 65.7 kB (−1 B) · 1.1 kB CSS (0) | — | — · — · — |
| lab/MobilePowerSearch | 214.0 kB (−1 B) · 1.2 kB CSS (0) | — | — · — · — |
| BottomSheet | 35.0 kB (0) · 7.4 kB CSS (−2 B) | — | ✓ · ✓ · — |
| DropdownMenu | 73.1 kB (0) · 15.4 kB CSS (−2 B) | — | — · — · — |
| Markdown | 145.2 kB (0) · 18.3 kB CSS (−2 B) | — | — · — · — |
| MoreMenu | 73.4 kB (0) · 15.4 kB CSS (−2 B) | — | — · — · — |
| Popover | 50.1 kB (0) · 12.4 kB CSS (−2 B) | — | — · — · — |
| PowerSearch | 184.5 kB (−1 B) · 20.6 kB CSS (−1 B) | — | — · — · — |
| Table | 84.4 kB (−1 B) · 16.0 kB CSS (−1 B) | — | — · — · — |
| Tokenizer | 70.3 kB (0) · 17.1 kB CSS (−2 B) | — | — · — · — |
| ContextMenu | 75.1 kB (−1 B) · 15.3 kB CSS (−2 B) | — | — · — · — |
| DateInput | 103.3 kB (0) · 15.3 kB CSS (−3 B) | — | — · — · — |
| TimeInput | 90.1 kB (−1 B) · 14.3 kB CSS (−3 B) | — | — · — · — |
| DateTimeInput | 158.8 kB (−2 B) · 17.0 kB CSS (−4 B) | — | — · — · — |
Package size: @astryxdesign/core 6.09 MB packed (+119 B); no new dependencies.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
982a837 to
614dacc
Compare
|
Moved to 614dacc: this pull request now sits on #8534, which adds the scroll-end guard and its hug-height stories, so it holds only its own change: the content box's |
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
This needs an engineering owner's spec decision before it can be approved, and two changeset fixes either way.
This pull request carries an earlier revision of #8534 that rewrites the current component:BottomSheet element-jobs line ("holds the panel at the block end with its own grid" to "… with its own flex row") and ships the flex-row dialog host, which also changes how a percentage height lays out; the approval on #8534 covers that pull request's later head, whose line reads differently. An engineering owner records the decision for this head by commenting /approve-spec 614dacc9f769f29bbe7040743f6c64fa7844aa90 here (the author can, as an engineering owner), or the pull request can rebase once #8534 merges so only the min-height fix remains.
.changeset/bottom-sheet-flex-host.mdsays "the layout is the same", but withheight="50%"a long sheet moves from the top of the screen and a short one from a 112 px strip to the bottom half of the screen in Chromium and WebKit, so the note should say so, as #8534's current changeset does..changeset/bottom-sheet-content-min-height.mdclaims the fix in iOS Safari, but the only iOS readings were taken with main's grid host rather than the flex-row host this head ships, so add an iOS reading of the capped direct-content story at this head or narrow the claim to WebKit.
The min-height: fit-content change itself works: in WebKit the capped direct-content sheet now ends 16 px above its bottom edge instead of flush, and Chromium is unchanged.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] BottomSheet: in Playwright's WebKit on a phone-sized page, an update inside an open sheet no longer holds up the page about 40 ms longer than it should; the iOS 26.2 simulator showed no difference. The open sheet's dialog places its panel with a flex row instead of a grid; the layout is the same. |
There was a problem hiding this comment.
This says the layout is the same, but the flex row changes a percentage height: with height="50%" on a 390x844 page, main puts a long sheet at the top of the screen (0 to 470 px) and cuts a short one to 112 px, while this head puts both in the bottom half, in Chromium and WebKit. #8534's current changeset already names this change; please carry that wording.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] BottomSheet: in WebKit, including iOS Safari, a sheet at a set height whose content sits directly in it keeps its bottom padding at the end of its scroll (0 → 16 px), instead of ending flush with the last line. |
There was a problem hiding this comment.
The iOS Safari part of this claim rests on simulator readings from a revision that used main's grid host. This head ships the flex-row host, and its own comment notes that the host's layout mode decides whether WebKit keeps this padding. An iOS reading of A11y/BottomSheet Capped Direct Blocks at this head, or wording limited to WebKit, would settle it.
| - the dialog carries the sheet's dialog role, modality and the top layer | ||
| when the sheet is modal, and holds the panel at the block end with its | ||
| own grid; a covered sheet's recede (FR6) is this element's transform; | ||
| own flex row; a covered sheet's recede (FR6) is this element's transform; |
There was a problem hiding this comment.
This rewrites a current claim, so it needs an engineering owner's /approve-spec for this head. The approval on #8534 covers a later wording of this line ("hosts the panel at the bottom edge, so a percentage height is a share of the viewport").
| // block-end padding from the scroll range. In a row the panel's height is | ||
| // its own cross size, so a sheet taller than the viewport runs past the top | ||
| // edge, as in a grid. The stack's recede applies to this dialog too, so a |
There was a problem hiding this comment.
Non-blocking (comment style): this sentence runs 27 words, past the 25-word limit for a description. Suggested: "In a row the panel keeps its own height, so a sheet taller than the viewport runs past the top edge."
…Kit's scroll range The panel's content box takes min-height: min-content so it grows with its content past a fixed-height viewport. WebKit ignores min-content on a block's min-height, so on a sheet at a set height the box stayed its body's height: content placed directly in it overflowed the box, and its block-end padding fell out of the scroll range, ending the scroll flush with the last line. fit-content is the same size in the block axis and WebKit honours it. The scroll-end guard gains a capped sheet with direct content: it reads 0 px in WebKit before the fix and 16 px, the content padding, with it. Chromium was already right.
614dacc to
9903519
Compare
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Recommending approval: the BottomSheet content box's min-height: fit-content keeps the 16 px content padding at the end of a set-height sheet's scroll in WebKit, where main ends flush with the last line, and the captured Chromium and Firefox frames are unchanged. The new capped-direct-blocks guard case fails in WebKit against main and passes here.
The earlier points about the carried dialog host and the iOS readings no longer apply: the flex-row host now comes from main, and in WebKit the fix gives the same 0 → 16 px result under grid, flex-row and flex-column hosts, so the earlier simulator readings still cover it.
[Automated review]
cixzhang
added
the
ci:run
User impact
In Safari on iPhone, a bottom sheet at a set height (
capped,tall, snap points) with overflowing content directly in it, without a Section, scrolls to an end flush with the last line, under the home indicator. Chrome keeps the 16 px padding.Problem and solution fit
height: 100%; min-height: min-content, and WebKit ignoresmin-contenton a block's min-height. On a set height the box stays the body's height (522 px against 1,760 px of content), so its bottom padding falls out of the scroll range.min-height: fit-content: the same size in the block axis, and WebKit honours it.height: 100%(content sized in percentages needs it).Expected behavior and authority
component:BottomSheetFR4: "The Content area pads caller content by the sheet's container inset", so the inset stays below the content at its scroll end.Smallest restoration
One value: the content box's
min-heightgoes frommin-contenttofit-content, and the guard gains the capped case and its story. No API, visual default or interaction changes.Evidence
min-contentrestored), WebKit's capped case reads 0 px and fails "Expected: >= 15.5, Received: 0". The iOS 26.2 simulator read 0 px on the superseded 982a837.capped-direct-blocksfails in WebKit on the red arm and passes here.Scope
Testing
On this head, on
main1821fb3:pnpm install --frozen-lockfile, build, type check, BottomSheet's unit tests (461), strict lint on the changed files,check:changesetsandpnpm review:prepass. On a fresh Storybook build, BottomSheet's accessibility contracts and browser specs pass in Chromium (47 of 47, the scroll-end guard 3 of 3) and its browser specs under the CI WebKit job's command (37 of 37, the scroll-end guard 3 of 3). The same change on #8534's head passed the full suites:test:ui(18,983), the node suites (16,062, with 2 expected failures),check:repoandASTRYX_STRICT_LINT=1 pnpm lint(0 errors).