Repository navigation
Conversation
The grab bar floats over the body with no reserve, so the content starts at the sheet's top edge -- a deliberate choice its test keeps. Its backdrop, though, stayed opaque to 60% of its 24 px band and faded out across the rest, which reaches past a content wrapper's default 16 px top padding: at rest the first line sat 18-19 px down, inside the fade, and read washed out. The backdrop is now opaque to the pill's lower edge and transparent by the default top padding (`--spacing-4`), so the first line is clear at rest while scrolled content still fades in beneath the pill. Layout is unchanged: the content keeps its place, fitting sheets do not scroll and the peek detent stays where it was. Tests: a Chromium spec reads the painted gradient's stops against the first line in two default-padding stories (it fails on the previous head), checks the opaque part still backs the pill, and guards that five fitting sheets do not scroll and that scrolled content still passes beneath the bar; a unit test pins the stops on the style.
|
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
…eir content CI's accessibility run found Snap Points With Peek overflowing by 10 px. That sheet's height is its detent's, not its content's, so whether the content fits depends on the machine's font metrics: it fitted on the machine the spec was written on and not on CI's. It was never a fitting sheet, and the change under test paints only, so it cannot move it. The guard keeps the four sheets whose height follows their content.
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.
This visual fix is on hold for a design decision: no current spec says where BottomSheet's grab-bar fade should end, and the description asks the design owner to settle it. Approval needs that direction decided (the backdrop opaque to the pill's lower edge at 14 px and transparent by --spacing-4 at 16 px, accepting that scrolled content then passes through a 2 px fade instead of about 10 px), and an engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, which the author may do as an engineering owner. Separately, please drop the new source-text case in BottomSheetPanel.test.tsx: it restates the style declaration, which the repository's test-sufficiency rules (INV5, INV6) don't count as evidence, and the new Chromium spec already checks the rendered stops.
[Automated review]
| // before the first line does, or that line reads washed out at rest; and | ||
| // the opaque part must still cover the pill. Asserted on the style | ||
| // definition for the same reason as the test above. | ||
| it('ends the bar\u2019s fade where the content\u2019s default top padding ends', async () => { |
There was a problem hiding this comment.
This case asserts the style's source text, which architecture:component-test-sufficiency INV6 says does not prove the promise (and INV5 says a test must not rely on alone when the outcome can be observed more directly). It also stays green if HANDLE_BAR_HEIGHT moves to --spacing-8, which pushes the opaque stop past the 16 px fade end. Consider deleting it: BottomSheetGrabBar.a11y.chromium.spec.ts already checks the resolved stops against the first line in a real browser.
It restated the style declaration, which the test-sufficiency rules do not count as evidence, and the Chromium spec already reads the stops the browser paints. BottomSheetPanel.test.tsx is back to main's, the no-reserve test included.
PR Analysis Report
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 | View full report |
There was a problem hiding this comment.
This visual fix is on hold for a design decision: no current spec says where BottomSheet's grab-bar fade should end, and the description asks the design owner to settle it. Approval needs that direction decided (the backdrop opaque to the pill's lower edge at 14 px and transparent by --spacing-4 at 16 px, accepting that scrolled content then passes through a 2 px fade instead of about 10 px), and an engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, which the author may do as an engineering owner. The source-text unit test is gone, so nothing else is required; if an owner prefers an alternative (reserving the band in layout, or keeping the current fade), the fade needs rework.
[Automated review]
| barTop: barBox.top, | ||
| barBottom: barBox.bottom, | ||
| opaqueEnd: at(stops[0] ?? '', 0), | ||
| fadeEnd: at(stops[stops.length - 1] ?? '', barBox.height), |
There was a problem hiding this comment.
Optional hardening: getComputedStyle keeps a stop's declared position even after the browser clamps it, so if the opaque stop ever passes this one (a 32 px bar puts it at 18 px), the surface is painted to 18 px while this still reads 16 px and the first-line check passes. Reading the fade end as the larger of the two stops, for example Math.max(opaqueEnd, lastStop), would catch that.
|
Why this matters: a bottom sheet at the default padding opens with its first line drawn through the grab bar's backdrop, so the first thing a reader sees is washed out — and this pull request cannot settle that alone, because no current spec says where the backdrop's fade ends. The question for the design and engineering owners: should the grab bar's backdrop end its fade at 16 px ( Our pick is yes, as built here. Measured in Chromium at 390×844:
The before and after crops are in the description, with the two alternatives: reserve the band with padding on the scroll body (the peek detent then has to count it, since it overflows by 14 px otherwise, and the no-reserve test goes), or leave it as it is. |
|
Posted by Cindy's agent on her behalf. Thanks for picking this up. Cindy chose a different fix for the grab-bar overlap: BottomSheet keeps its content's resting top inset at 24 px or more, whatever the padding. The first row never starts under the grab bar's fade, and scrolled content still passes under the bar as it does today. A shorter fade (this PR) leaves a sheet with smaller padding, such as Popover's sheet presentation, with its first row right at the bar's opaque edge, and scrolled content meets a near-hard edge. The inset fix clears every sheet in one place. It will be one line in BottomSheet's inset rule plus the layout change, with before-and-after captures at 390 px. This PR will be superseded by that fix. We'll close it once the fix lands, crediting your report of the issue. |
kentonquatman
left a comment
kentonquatman
left a comment
There was a problem hiding this comment.
The new gradient is a bit too short. Visually, it looks closer to a hard cutoff than a gradient when scrolling. Would it be possible to keep the previous gradient and only have it appear once the user has started scrolling?
|
A 2 px fade does read as a cut line under scrolled content, so the previous gradient stays. Cindy's fix for the overlap (above) keeps a sheet's content 24 px or more below its top at rest, which puts the whole previous fade over empty surface until something scrolls under it: it appears only once the user starts scrolling, as you describe, with no scroll listener. That fix supersedes this pull request. The previous fade with that inset, at rest and scrolled 40 px, in Chromium and WebKit:
|
|
Agreed: an inset of at least 24 px clears every sheet in one place and keeps the previous fade, which the review above asks for as well. This pull request can close when your fix lands. If it helps, its Chromium check that reads the first line against the fade's resolved stops can come with your fix, with the fade's end at 24 px. At
|
|
Posted by Cindy's agent on her behalf. Closing in favor of #8558, which landed the BottomSheet resting inset Cindy chose (at least 24 px above the content, so the grab bar never overlaps it). Thank you for the shorter-fade exploration; it helped settle the direction. |
User impact
A bottom sheet at the default padding opens with its first line drawn through the grab bar's backdrop. The bar floats over the content with no reserve, so the content starts at the sheet's top edge, and the bar's surface-coloured fade reached 24 px down, past a content wrapper's default 16 px top padding: the top of the first line read washed out at rest. The review that approved #8229 lists it as a follow-up: "its first row starts 14 px down, inside the bar's 24 px fade".
The backdrop is now opaque to the pill's lower edge and transparent by the default top padding (
--spacing-4). Nothing moves.Visual authority
DDrow reused or amended.Design decisionstable by a team design owner.docs/design/.None yet: the no-reserve bar is recorded only in
BottomSheetPanel.tsx's comment and its test, so this asks the design owner where the fade ends. Alternatives:Before and after
Chromium, 390×844 at 3×, light theme, the sheet's top 64 px. Showcase, at the default padding:
Text Only Fitting,
padding={0}:State matrix
--color-background-surfaceeverywhere, unchanged; only the two stops move.Scope
Testing
BottomSheetGrabBar.a11y.chromium.spec.ts(new), against the built stories: the painted gradient's stops against the first line in Showcase and Tall Sheet, failing on main (the lines at 19 and 18 px, the fade ending at 24); the opaque part still backs the pill; four sheets sized by their content don't scroll; scrolled content still passes beneath the bar.BottomSheetPanel.test.tsx: unchanged, the no-reserve test included (283 BottomSheet unit tests pass).pnpm exec playwright test packages/core/src/BottomSheet/__tests__/ --config playwright.config.ts --project=chromium): all pass.pnpm build, rootpnpm typecheck,pnpm vitest run packages/core/src/BottomSheet packages/core/src/Popover packages/core/src/Layer packages/core/src/__tests__(743), the CLI tests (309),pnpm check:knowledge,pnpm check:changesets, changeset coverage,CI=true pnpm lint(no error) andnode scripts/check-sync.js.