Repository navigation
Conversation
… the padding
The grab bar floats over the content: 24 px tall, opaque behind the pill
and fading out across its lower half. At the default padding (16 px), a
sheet's first line started inside that fade, and a Popover sheet's (12 px)
under its opaque part, so the top of the first line read washed out or cut.
At rest, the content box's block-start inset is now never less than the
bar's height (`max(padding, --spacing-6)`), so the first line starts below
the bar for any `padding`, `padding={0}` included. The inset is the content
box's own padding, so it scrolls away with the content, which still passes
beneath the bar. The published inset stays the padding, so a child that
bleeds into it also starts below the bar. component:BottomSheet's Content
area inset records the rule.
Chromium proof: BottomSheetGrabBar.a11y.chromium.spec.ts (first line below
the bar at rest; fitting sheets don't scroll; scrolled content passes
under the bar), adapted from the measurements in the alternative fix that
shortened the fade instead.
Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
|
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:spec-owner-review
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsContextMenu (@astryxdesign/core) · View in Storybook
DateInput (@astryxdesign/core) · View in Storybook
DateTimeInput (@astryxdesign/core) · View in Storybook
TimeInput (@astryxdesign/core) · View in Storybook
MobilePowerSearch (@astryxdesign/lab) · View in Storybook
MobileTokenizer (@astryxdesign/lab) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression57 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 37 more. Generated by PR Enrichment workflow | Storybook | Sandbox | 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.
This change is waiting on an engineering owner to decide the new BottomSheet rule it adds: at rest, a sheet's block-start inset is never less than the grab bar's 24px. That rule replaces the current inset (the padding prop, then theme padding, then --spacing-4) and the unpadded padding={0} contract, so an engineering owner records the decision by commenting /approve-spec <full head SHA> here; the author may do so as an engineering owner. Whatever the decision, a few fixes are needed:
- The DateInput, DateTimeInput and TimeInput touch pickers and the DropdownMenu and ContextMenu action sheets pass
padding={0}and already reserve the band, so they now get a second 24px strip under the handle and grow 24px taller. --container-padding-block-startnow publishes less than the padding actually applied, but FR4 and the container-padding architecture require the two to match: publish the applied value or amend those records in this PR.BottomSheet.doc.mjs, thepaddingJSDoc and the record's compatibility line still describe the old inset, and under AST-017 FR3 this change is a[feat], not a[fix].
[Automated review]
| // Content area inset). The inset is the content box's own padding, so it | ||
| // scrolls away with the content, which then passes under the bar. | ||
| clearsGrabBar: { | ||
| paddingBlockStart: `max(var(--container-padding-block-start, 0px), ${HANDLE_BAR_HEIGHT})`, |
There was a problem hiding this comment.
This floor also applies to padding={0} sheets that already reserve the band themselves: TouchDateField, TouchDateTimeField and TouchTimeField pad --spacing-6 in their sheet bodies, and MenuBottomSheet (DropdownMenu, ContextMenu) pads its menu content. In Chromium their first row moves down 24px and each hug sheet grows 24px. Either drop those callers' own band in this PR or leave padding={0} unfloored. Separately, --container-padding-block-start stays below the value applied here, while FR4 and the container-padding architecture require the published inset to match the applied one.
| on `bottom-sheet`, then `--spacing-4` on every logical edge. At rest the | ||
| block-start inset is never less than the grab bar's height (`--spacing-6`), | ||
| so the first row starts below the bar; scrolled content still passes under it. |
There was a problem hiding this comment.
This adds a normative floor, but the record still says padding={0} keeps an unpadded Content area (Compatibility and migration, lines 63-65), and FR4 still says the Content area publishes the applied inset. Please reconcile those in this PR. An engineering owner decides the floor itself (/approve-spec <full head SHA>).
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] BottomSheet: at rest, a sheet's content starts below the grab bar whatever its `padding`, so the first line is no longer drawn through the bar's fade. Scrolled content still passes beneath the bar. A sheet at the default padding starts its content 8 px lower; one with `padding={0}` keeps its content clear of the bar. |
There was a problem hiding this comment.
AST-017 FR3 uses [feat] for a layout change that intentionally changes visual direction and [fix] for one that restores current visual authority. No current claim reserves the band, so this reads as [feat], still a patch. It would also help to name the old padding={0} appearance (edge-to-edge) next to the new 24px inset.
…heets stop reserving the band From CodeBunny's review at a6642d3: - The block-start inset is now floored where it is published, so the Content area applies and publishes the same value (bleed children escape exactly what they sit in): the theme default's chain in Layout/container.stylex.ts, and each explicit padding step in BottomSheetPanel. - Core's own padding={0} sheets reserved the band under the grab bar themselves, so they grew a second 24px strip. The touch date, date-time and time pickers drop their 24px top padding (they look the same), and the DropdownMenu and ContextMenu sheet content drops its 16px top (their first row sits 8px lower, below the bar). - BottomSheet's doc, `padding` JSDoc and the record's compatibility line describe the floor; the changeset is a [feat] (spec:AST-017 FR7).
github-actions
Bot
added
the
needs:design-review
meta-cla
Bot
added
the
CLA Signed
There was a problem hiding this comment.
This change is waiting on an engineering owner to decide the new BottomSheet rule it writes into the component record: at rest the Content area's block-start inset is never less than the grab bar's 24px, replacing the current --spacing-4 inset and the unpadded padding={0} contract. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever the decision, a few fixes are needed:
- Because the floored inset is also published, a lone
Sectionescapes the whole band, so the Showcase, Tall Sheet, Hug Height and Text Only sheets render exactly as before, with the first line 12-17px under the bar, and the new Chromium spec's at-rest tests fail for them. - Lab's MobilePowerSearch and MobileTokenizer sheets pass
padding={0}with their own top band and now get a second 24px strip. docs/architecture/container-padding.mdand Popover's DEC-8 still state the unfloored inset, andBottomSheet.doc.mjs(line 98), the Container Padding story and a few comments still describe the old float.
[Automated review]
| }, | ||
| containerPaddingBlockStart: { | ||
| '--container-padding-block-start': bottomSheetBlockStart, | ||
| '--container-padding-block-start': bottomSheetBlockStartClear, |
There was a problem hiding this comment.
Publishing the floored value keeps FR4's published-equals-applied rule, but it is also what a first-child Section subtracts (Section.tsx:69-72): a lone Section escapes all 24px and pads only its own 16px, so Showcase, Tall Sheet and Hug Height still start their first line 16-17px below the bar's top in Chromium, pixel-identical to main (Text Only: 12px). Keeping the band out of what bleed children escape, for example by reserving it on the scrolling body, would let the content box still apply and publish one value.
| description: JSON.stringify(reading), | ||
| }); | ||
| expect(reading.firstLineTop).not.toBeNull(); | ||
| expect(reading.firstLineTop!).toBeGreaterThanOrEqual( |
There was a problem hiding this comment.
At this head this assertion cannot pass for showcase, tall-sheet or text-only: each puts a lone Section first, which escapes the whole published inset, and a Chromium probe at 390x844 reads the first line 17px, 16px and 12px below the bar's top against a 24px fadeEnd.
| on `bottom-sheet`, then `--spacing-4` on every logical edge. At rest the | ||
| block-start inset is never less than the grab bar's height (`--spacing-6`), | ||
| so the first row starts below the bar; scrolled content still passes under it. | ||
| The Content area applies and publishes the same inset, so a child that bleeds | ||
| into it escapes exactly what it sits in. |
There was a problem hiding this comment.
These sentences disagree for bleed content: a lone Section 'escapes exactly what it sits in', all 24px, so its first row does not start below the bar. docs/architecture/container-padding.md (lines 117-122) and Popover's DEC-8 ('inset by Popover's padding') also restate the old inset and need the same amendment in this PR.
| type: '0 | 0.5 | 1 | 1.5 | 2 | 3 | 4 | 5 | 6 | 8 | 10', | ||
| description: | ||
| 'Internal padding of the sheet content using the spacing scale, matching Dialog. When omitted, uses the theme default for bottom sheets (--spacing-4 unless the theme sets padding on bottom-sheet). A lone Section child escapes the padding, and bleed children such as Table and Divider compensate against it. Use 0 for edge-to-edge content that supplies its own inset.', | ||
| "Internal padding of the sheet content using the spacing scale, matching Dialog. When omitted, uses the theme default for bottom sheets (--spacing-4 unless the theme sets padding on bottom-sheet). A lone Section child escapes the padding, and bleed children such as Table and Divider compensate against it. Use 0 for edge-to-edge content that supplies its own inset. Whatever the padding, the block-start inset is never less than the grab bar's height (--spacing-6), so resting content starts below the bar.", |
There was a problem hiding this comment.
The component description at line 98 still says 'content starts at the theme's bottom-sheet padding (--spacing-4 by default)', and the Container Padding story still says 'padding={0} restores the unpadded content box' (apps/storybook/stories/BottomSheet.stories.tsx:130); both need the floor too.
| // content, which then passes under the bar. | ||
| const clearsGrabBarStyles = stylex.create({ | ||
| 0: { | ||
| '--container-padding-block-start': `max(${spacingVars['--spacing-0']}, ${HANDLE_BAR_HEIGHT})`, |
There was a problem hiding this comment.
padding={0} now gets a 24px top inset, so callers that still pad their own band double it: lab's MobilePowerSearch header (paddingBlockStart: --spacing-6, MobilePowerSearch.tsx:323-324, three sheets) and MobileTokenizer's sheet body (--spacing-8, MobileTokenizer.tsx:160). In Chromium the Filters heading moves from 33px to 57px below the bar's top.
…ection escapes From CodeBunny's review at c65c74a: publishing the floored inset let a lone Section (Showcase, Tall Sheet, Hug Height, Text Only) escape the whole band, so its first line still started 12-17px under the bar. The band under the bar is no longer container padding. The Content area publishes only the block-start padding past it (`max(padding - --spacing-6, 0)`) and applies the band on top (`calc(published + --spacing-6)`), so its applied inset is still max(padding, the bar) and what a lone Section or Layout escapes ends at the band: a lone Section's first line now starts 37px below the bar's top (13px before). Every other edge applies and publishes the same inset. - Lab's MobileTokenizer and MobilePowerSearch sheets (`padding={0}`) no longer double the band: the tokenizer's body keeps its 32px start, and the search sheets' sticky header sits over the band at rest, clearing the bar only once stuck. - component:BottomSheet's inset and compatibility lines, architecture:container-padding, Popover's DEC-8 wording, BottomSheet's doc, the Container Padding story and comments describe the band. - The Chromium spec's at-rest check now includes Hug Height.
|
/approve-spec 812b064 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
The grab-bar rule is approved for this head and the core change works as described, but a few fixes are needed before it can merge. Please address these; the new head then needs a fresh /approve-spec from an engineering owner:
- The
table-filterandtable-inboxpage templates inpackages/clistill add their own 24px top band inside their bottom sheets (SHEET_TOP_INSETand the composer'spaddingBlockStart={3}), so they now open with a second empty band; drop it as Core and Lab do here, and name the CLI update in the changeset. component:BottomSheetFR4 and its verification row still say the published inset equals the applied padding and that a lone padded Section keeps its geometry, so they need the same band exception as the new Content area inset.- The changeset says default-padding content moves 8 px, but a sheet whose only child is a
Sectionmoves 24 px and grows by the same amount, and the changeset should say so.
[Automated review]
| on `bottom-sheet`, then `--spacing-4` on every logical edge. At rest the | ||
| block-start inset is never less than the grab bar's height (`--spacing-6`), | ||
| so the first row starts below the bar; scrolled content still passes under it. | ||
| That band under the bar is not container padding: the Content area publishes |
There was a problem hiding this comment.
This carve-out needs FR4 to match. FR4 (line 154) still says the Content area 'publishes the applied inset', and its verification row (line 385) fails when 'The published inset differs from the applied padding, or a lone padded Section changes geometry'. This head now does both by design. Could you amend FR4 and its row to name the band here as well?
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] BottomSheet: at rest, a sheet's content starts below the grab bar whatever its `padding`, so the first line is no longer drawn through the bar's fade. The block-start inset is never less than the bar's height (`--spacing-6`): the band under the bar is not padding a child escapes, so a lone `Section` starts below the bar too; scrolled content still passes beneath it. A sheet at the default padding starts its content 8 px lower, and one with `padding={0}` starts it below the bar. Core's own `padding={0}` sheets (the touch date and time pickers, DropdownMenu and ContextMenu sheets) no longer reserve that band themselves, so the pickers look the same and a menu's first row sits 8 px lower. |
There was a problem hiding this comment.
The 8 px holds for content placed directly in the sheet. A sheet whose only child is a Section or Layout (Showcase, Tall Sheet, Hug Height, Text Only) moves 24 px, and its hug height grows by the same amount. The CLI's table-filter and table-inbox page templates also need their own top band removed, which makes this a @astryxdesign/cli update to classify here too.
| ] as const; | ||
| // The block-start inset never drops below the grab bar's height, so | ||
| // resting content starts below the bar (component:BottomSheet, Content | ||
| // area inset); it is applied and published alike. |
There was a problem hiding this comment.
At the block start this no longer holds: the tests below expect max(calc(… - var(--spacing-6)),0px) to be published and calc(var(--container-padding-block-start,0px)+var(--spacing-6)) to be applied. The same stale claim is in the content-box comment at BottomSheetPanel.tsx:304-305.
… FR4 states the band exception From CodeBunny's review at 812b064: - The CLI's table-filter and table-inbox page templates reserved their own 24px band inside their sheets (`SHEET_TOP_INSET`, the composer's `paddingBlockStart={3}`); the sheet keeps that band now, so they drop it and table-filter's sheets open on the same line as before. - component:BottomSheet FR4 and its verification row state the block-start exception: the Content area publishes only the padding past the band, so a child escapes the padding but never the band. - The changeset says a sheet whose only child is a Section moves that Section 24px and grows by the same amount, and names the CLI update.
|
/approve-spec 1c54a3a |
There was a problem hiding this comment.
Thanks: the grab-bar rule is approved for this head and works as described, with resting content now starting below the bar and scrolled content still passing beneath it. One record fix is needed before merge: the INV2 verification row in docs/architecture/container-padding.md still fails the Content area when it publishes a different block-start inset than it applies, which this change now does by design, so it needs the same band exception you added to FR4's row. Any new commit also needs a fresh /approve-spec <full head SHA> from an engineering owner, which you can record yourself.
[Automated review]
| container lowering. At the block start a sheet keeps a band under its floating | ||
| grab bar (`--spacing-6`) that is not container padding: it publishes only the | ||
| padding past that band and applies the band on top, so its applied block-start | ||
| inset is `max(padding, --spacing-6)` and a lone Section or Layout that escapes | ||
| the published padding still starts below the bar (`component:BottomSheet`, | ||
| Content area inset). |
There was a problem hiding this comment.
This paragraph now says the block start publishes only the padding past the band, but the INV2 verification row further down (line 376) still fails BottomSheet when "The Content area publishes an inset that differs from its applied padding". Could it get the same band exception FR4's row now has, e.g. "…on an edge other than the block start, its block start publishes more than the padding past the grab bar's band, or…"?
| ] as const; | ||
| // The block-start inset never drops below the grab bar's height, so | ||
| // resting content starts below the bar (component:BottomSheet, Content | ||
| // area inset); it is applied and published alike. |
There was a problem hiding this comment.
Nit: at the block start the published and applied values now differ by design (the assertions below check exactly that), so "it is applied and published alike" no longer holds. The content-box comment at BottomSheetPanel.tsx:304-305 has the same stale wording.
…w state the block-start band exception From CodeBunny's review at 1c54a3a: architecture:container-padding's host-side rule and its INV2 verification row for BottomSheetPanel still required the published block-start inset to equal the applied one; both now name the exception the Content area inset decides (the block start publishes only the padding past the grab bar's band).
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
This change is waiting on an engineering owner to approve the BottomSheet grab-bar rule as this head writes it: the Content area's block-start inset is never less than the bar's 24px and only the padding past that band is published, which replaces the current inset in component:BottomSheet and the published-equals-applied rule in architecture:container-padding. The earlier /approve-spec comments named earlier commits, so an engineering owner records the decision for this head by commenting /approve-spec <full head SHA> on the pull request, and the author may do so as an engineering owner. Two optional touch-ups before that: the comments at BottomSheetPanel.test.tsx:417-419 and BottomSheetPanel.tsx:304-305 still say the block start is applied and published alike, and INV11 in container-padding.md still calls any published/applied mismatch a conformance gap.
[Automated review]
| currently available at each logical edge through the existing private | ||
| `--container-padding-*` variables. Those values must match that state's | ||
| applied padding and follow nesting and overlay reset; missing values mean zero. | ||
| applied padding, except BottomSheet's block start, which publishes only the |
There was a problem hiding this comment.
With this exception the host-side rule treats BottomSheet's block start as an intended split, but INV11 (line 289) still says a host state "whose published inset does not match its applied padding is ineligible until that conformance gap is fixed". Nothing compensates at a sheet's block start today, so behavior is unaffected; naming the exception in INV11 too, or saying the block start stays ineligible by design, would keep the record consistent for whoever approves it.
|
/approve-spec 18aee67 |
github-actions
Bot
removed
the
needs:spec-owner-review
…to the grab-bar inset
|
/approve-spec 1eb521c |
There was a problem hiding this comment.
Changes are needed before merge. The grab-bar rule has an engineering owner's approval for this head and works as described, but merging main replaced the FR4 row in packages/core/src/BottomSheet/BottomSheet.spec.md (line 157) with a copy of FR4's verification row, so the record no longer states FR4's invariant. Please restore that row with the band-exception wording it had before the merge, then record a fresh /approve-spec <full head SHA> for the new commit, which you can do yourself as an engineering owner.
[Automated review]
| | FR2 | The Sheet panel carries `bottom-sheet`; Content area, Handle, and Scrim carry no BottomSheet public target. | Current source, docs, and tests | | ||
| | FR3 | Content renders in one real observed content box. Effective overflow, containment, and keyboard access come from `useScrollableArea`; the named overflowing body delegates forward Tab entry to a safe first sequential native link/button. | `spec:AST-025`; tests | | ||
| | FR4 | The Content area pads caller content by the sheet's container inset and publishes the applied inset to descendants as a container publisher. | `architecture:container-padding` | | ||
| | FR4 | `BottomSheetPanel.test.tsx` container padding tests; `derivedVarRegistry.test.ts` | default, `padding` prop, theme `padding`, lone padded Section child | The published inset differs from the applied padding on an edge other than the block start, the block start publishes more than the padding past the grab bar's band, or a lone padded Section escapes into the band. | `audit:BottomSheet/anatomy` | |
There was a problem hiding this comment.
This row now holds the five cells of FR4's verification row (line 387) under the three-column ID | Invariant | Basis header, so FR4's invariant is gone. It looks like the merge of main kept the wrong line here. Here is the wording from before the merge:
| FR4 | The Content area pads caller content by the sheet's container inset and publishes the applied inset to descendants as a container publisher, except at the block start: there it publishes only the padding past the band it keeps under the grab bar (Content area inset), so a child escapes the padding but never the band. | `architecture:container-padding` |
User impact
A person opening a bottom sheet on a phone saw the top of its first line drawn through the grab bar. The bar floats over the content (24 px, opaque behind the pill, fading out below it). At the default padding the first line started inside the fade, and in a Popover's sheet (12 px padding) it started under the opaque part, so the line's top was cut.
Now, at rest, the content starts below the bar whatever the sheet's
padding. Scrolled content still passes beneath the bar.Problem and solution fit
max(padding, --spacing-6), the bar's height. The band under the bar isn't container padding: the sheet publishes only the padding past it (max(padding - --spacing-6, 0)) and applies the band on top. A loneSectionorLayoutthat escapes its padding therefore still starts below the bar. Every other edge applies and publishes the same inset. It's the content box's own padding, so it scrolls away with the content.table-filterandtable-inboxtemplates drop theirs, so the band isn't doubled. Lab's MobileTokenizer keeps its 32 px start, and MobilePowerSearch's sticky header sits over the band at rest and clears the bar once stuck.padding={0}sheets: The touch date, date-time and time pickers drop their 24 px top padding, so they look the same. The DropdownMenu and ContextMenu sheet content drops its 16 px top, so the first row sits 8 px lower, below the bar.component:BottomSheet's Content area inset and compatibility line record the rule; the doc andpaddingJSDoc describe it. Changeset:[feat](spec:AST-017FR7), a visible layout change.Credit: vjeux's #8514 found and measured the problem and proposed shortening the fade instead. This change takes the other direction its body lists (reserving the band), and its Chromium spec is adapted from #8514's measurements (co-authored).
Before and after
Chromium, 390×844 at 3×, the sheet's top 120 px. Left: before. Right: after.
A sheet whose only child is a
Section(Showcase, Tall Sheet, Hug Height and Text Only in Storybook). The Section escapes the sheet's padding but not the band:Core's own
padding={0}sheets, which no longer reserve the band themselves:Measured (first line's top, from the bar's top; the bar is 24 px):
Evidence
BottomSheetPanel.test.tsx's padding tests pin the applied block-start inset tomax(<published padding>, --spacing-6)for the default chain,padding={0}andpadding={2}. All three fail on main.BottomSheetGrabBar.a11y.chromium.spec.ts. At rest the first line starts below the whole bar (showcase, tall-sheet, text-only). Sheets whose content fits still don't scroll. Scrolled content passes under the bar.