Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
ac9bd4c to
93aa9f9
Compare
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsAppShell (@astryxdesign/core) · 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: The accessibility audit did not run on this pull request. It runs after merges to 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 needs an engineering owner's decision before it can be approved: no current spec defines how AppShell handles the device's safe-area insets, and the description itself lists the landscape content insets and the LayerProvider block-start inset as pending owner decisions. An engineering owner can record that decision by commenting /approve-spec <full head SHA> on this pull request (you may do so yourself if you are an engineering owner), or point to the spec that already governs this behavior. Whatever the decision, these fixes are needed:
- With a
bannerover a sidenav-only layout on a phone, both header wrappers pad by the top inset, leaving a 62px blank band above the phone top bar (AppShell.tsx:875-879). - The bottom
::afterspacer makes a child that fills main in fill mode, such as theshell-navtemplate'sLayout height="fill", overflow by the inset, so main gains an outer scroll while the child's footer still sits on the home indicator. - The docs best practice and the Changeset should say that in landscape the SideNav and the content sides are not inset, and the header of
AppShellSafeArea.test.tsxpoints to a spec file that does not exist.
[Automated review]
| stylex.props( | ||
| headerAreaStyle, | ||
| isAuto && styles.headerSticky, | ||
| styles.headerSafeArea, |
There was a problem hiding this comment.
With a banner and no TopNav, this wrapper renders under the banner's header wrapper, which already pads by the top inset, so on a phone with a 62px inset the top bar gets a second 62px blank band. Apply the block-start inset here only when headerContent == null, the condition the role above already uses.
There was a problem hiding this comment.
Fixed in e7320d9: under a banner, the sidenav-only top bar now takes the side insets only (headerContent == null ? headerSafeArea : headerSafeAreaSides). New jsdom test “pads a sidenav-only phone top bar under a banner by the sides only” fails on the previous head.
| mainSafeAreaEnd: { | ||
| '::after': { | ||
| content: '""', | ||
| display: 'block', | ||
| height: 'env(safe-area-inset-bottom, 0px)', |
There was a problem hiding this comment.
In fill mode a child that fills main (for example Layout height="fill" under contentPadding={0}, as the shell-nav template composes it) now overflows by the bottom inset: main scrolls 34px while the child's footer still sits on the home indicator. Carrying the inset in main's block-end padding (LayoutContent is border-box) would let a percentage-height child end above it instead.
There was a problem hiding this comment.
Fixed in e7320d9: the inset now lives in main's block-end padding (LayoutContent is border-box), alone or added to the content padding, instead of an ::after spacer. New Chromium case: a height: 100% child in main ends 34 px above the bottom with no outer scroll (fails on the previous head); jsdom checks both padding forms.
| { | ||
| guidance: true, | ||
| description: | ||
| 'On phones, opt the page into the full screen with `viewport-fit=cover` in the viewport meta tag (`width=device-width, initial-scale=1, viewport-fit=cover`). AppShell then extends its header under the status bar and out to the screen sides, and keeps the header\'s items and the end of the content inside the device\'s safe area. An installed web app can also draw under the status bar with `<meta name="apple-mobile-web-app-status-bar-style" content="black-translucent">`.', |
There was a problem hiding this comment.
In landscape AppShell insets only the header: once the page opts into viewport-fit=cover, the inline SideNav and the content sides sit under the notch. This guidance (and the Changeset) should say so until that case is decided.
There was a problem hiding this comment.
Fixed in e7320d9: the best practice, its dense form and the changeset now say that in landscape only the header is inset (not an inline SideNav or the content sides).
| * @position Testing; validates AppShell.tsx's safe-area styles. jsdom's CSS | ||
| * parser drops `env()` values, so the declarations are read from the rules | ||
| * StyleX hands to `insertRule`; the geometry under real insets is proven by | ||
| * __tests__/AppShellSafeArea.interaction.browser.spec.ts |
There was a problem hiding this comment.
This file does not exist; the browser proof is __tests__/AppShellSafeArea.a11y.chromium.spec.ts.
There was a problem hiding this comment.
Fixed in e7320d9: the header now points to __tests__/AppShellSafeArea.a11y.chromium.spec.ts.
93aa9f9 to
e7320d9
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved. No current spec defines AppShell's safe-area behavior, and component:AppShell FR4 says main's padding is contentPadding, so the bottom inset also needs that claim amended. An engineering owner can approve this direction by commenting /approve-spec <full head SHA> on this pull request (you may do so yourself if you are an engineering owner), or reject it. Whatever the decision, AppShell.tsx:463-465 needs a fix: it adds the inset to --layout-padding-own-outer-y, which is 0px inside AppShell, so main loses its bottom contentPadding on every page (24px to 0px in Top nav only with no inset); composing with var(--container-padding-block-end, 0px) as Drawer does keeps it, and the Chromium spec should assert contentPadding plus the inset rather than a base measured on the same head.
[Automated review]
| paddingBlockEnd: 'env(safe-area-inset-bottom, 0px)', | ||
| }, | ||
| mainSafeAreaEndPadded: { | ||
| paddingBlockEnd: `calc(var(--layout-padding-own-outer-y, var(--layout-padding-outer-y, ${spacingVars['--spacing-4']})) + env(safe-area-inset-bottom, 0px))`, |
There was a problem hiding this comment.
This adds the inset to --layout-padding-own-outer-y, which is 0px here: AppShell renders its own <Layout padding={0}> and main inherits that value, while LayoutContent applies contentPadding through paddingStyles[padding] and --container-padding-block-end. So main's block-end padding drops contentPadding on every page: Top nav only goes from 24px to 0px with no inset, and gets 34px instead of 58px with a 34px inset. calc(var(--container-padding-block-end, 0px) + env(safe-area-inset-bottom, 0px)), as Drawer.tsx composes it, resolves to 24px and 58px here and also covers the flush branch, since the variable is 0px when contentPadding is 0.
| expect(g.padding.top).toBe(PORTRAIT.top); | ||
| expect(g.item.top - g.bannerTop).toBeGreaterThanOrEqual(PORTRAIT.top - 0.5); | ||
| // Main's end padding grows by exactly the bottom inset. | ||
| expect(g.spacer - base).toBe(PORTRAIT.bottom); |
There was a problem hiding this comment.
base is measured on this same head, where main's block-end padding is already 0, so this checks only the delta and passes with contentPadding lost. Comparing against contentPadding plus the inset (24 + 34 on Top nav only) would catch it; the same applies at line 208, and line 182 should expect 58 rather than the bare inset.
e7320d9 to
02e3234
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec defines AppShell's safe-area behavior, and component:AppShell FR4 says main's padding is contentPadding, so the bottom inset also needs that claim amended. An engineering owner can approve the direction by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or reject it. Whatever the decision, AppShell.tsx:475-477 still drops contentPadding: it adds the inset to --layout-padding-own-outer-y, which is 0px inside AppShell, so Top nav only loses its 24px bottom padding with no inset and gets 34px instead of 58px with a 34px inset. Composing with var(--container-padding-block-end, 0px), as Drawer.tsx does, keeps it, and the Chromium spec should assert contentPadding plus the inset rather than a base measured on the same head.
[Automated review]
| paddingBlockEnd: 'env(safe-area-inset-bottom, 0px)', | ||
| }, | ||
| mainSafeAreaEndPadded: { | ||
| paddingBlockEnd: `calc(var(--layout-padding-own-outer-y, var(--layout-padding-outer-y, ${spacingVars['--spacing-4']})) + env(safe-area-inset-bottom, 0px))`, |
There was a problem hiding this comment.
--layout-padding-own-outer-y is 0px here: AppShell renders its own <Layout padding={0}> and main inherits that value, while LayoutContent applies contentPadding through paddingStyles[padding], which this overrides. So main's block-end padding drops contentPadding on every page: Top nav only goes from 24px to 0px with no inset, and gets 34px instead of 58px with a 34px inset. calc(var(--container-padding-block-end, 0px) + env(safe-area-inset-bottom, 0px)), as Drawer.tsx composes it, resolves to 24px and 58px here and also covers the flush branch.
| expect(g.padding.top).toBe(PORTRAIT.top); | ||
| expect(g.item.top - g.bannerTop).toBeGreaterThanOrEqual(PORTRAIT.top - 0.5); | ||
| // Main's end padding grows by exactly the bottom inset. | ||
| expect(g.spacer - base).toBe(PORTRAIT.bottom); |
There was a problem hiding this comment.
base is measured on this same head, where main's block-end padding is already 0, so this checks only the delta and passes with contentPadding lost. Comparing against contentPadding plus the inset (24 + 34 on Top nav only) would catch it; the same applies at line 209, and line 183 should expect 58 rather than the bare inset.
cixzhang
added
the
ci:run
…he device's safe area On a phone page that opts into the full screen (`viewport-fit=cover`), an installed web app above all, AppShell's header items sat under the status bar and the Dynamic Island, and the last content sat on the home indicator. The header now pads its items by the top and side safe-area insets, so its background reaches under the status bar while its items stay clear, as a UIKit bar does. Under a banner, the sidenav-only phone top bar takes the side insets only, since the banner's wrapper already pads the top. Main's block-end padding grows by the bottom inset, in the page or in its own scroller; main is border-box, so a child that fills it ends above the inset instead of overflowing. In landscape only the header is inset. Without `viewport-fit=cover` every inset is zero and nothing moves. The docs say how a page and an installed web app opt in. Its browser proof, `AppShellSafeArea.a11y.chromium.spec.ts`, runs in pr-a11y through its `@pr-a11y scope` line when a pull request changes AppShell or its stories.
02e3234 to
71d5e63
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec defines AppShell's safe-area behavior, and component:AppShell FR4 says main's padding is contentPadding, so the bottom inset also needs that claim amended and the header behavior a claim beside FR3. An engineering owner can approve the direction by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or reject it. Whatever the decision, AppShell.tsx:499-501 still drops contentPadding: it adds the inset to --layout-padding-own-outer-y, which is 0px inside AppShell, so Top nav only loses its 24px bottom padding with no inset and gets 34px instead of 58px with a 34px inset. Composing with var(--container-padding-block-end, 0px), as Drawer.tsx does, keeps it, and the Chromium spec and jsdom test should assert contentPadding plus the inset rather than a base measured on the same head.
[Automated review]
User impact
On a phone page with
viewport-fit=cover(an installed web app with a translucent status bar), AppShell's header title and buttons sit under the status bar and Dynamic Island, and the last line on the home indicator. On iOS 26 every Home Screen icon opens as a web app.Problem and solution fit
env(safe-area-inset-*).Expected behavior and authority
UIKit bars attached to the top extend their background behind the status bar while their items stay in the safe area; WebKit's guidance for
viewport-fit=coveris to pad content withenv(safe-area-inset-*). Astryx does this for layers (spec:AST-059) and the fullscreen Dialog, Drawer and BottomSheet, but not AppShell (family:layout-regionsDEC-2).Smallest restoration
Two AppShell styles and a docs best practice. Without
viewport-fit=cover, or in a portrait Safari tab, every inset is 0 and nothing moves.Owner decisions (proposed; pending owner review):
--container-padding-*(architecture:container-padding), whose owners decide.--astryx-layer-inset-block-start(AST-059): the toast viewport sits outside AppShell's subtree.Evidence
The end padding is proven in Chromium with emulated insets; the simulator's installed app stops its viewport 62 pt above the screen bottom, so nothing reaches the home indicator to film.
Same installed web app, main (top) and this branch (bottom):
Siblings: #8111 (merged, kept here) and #8142 edit these AppShell lines.
Scope
Testing
Local gates at
71d5e63e67, one commit on mainbcb16db0e8:vitestover AppShell, Layer and the keyboard-scroll helpers: 17 files, 348 tests pass. The newAppShellSafeArea.test.tsxreads the rules StyleX injects, because jsdom dropsenv(): the header's top and side insets (swapped in RTL), the sidenav-only phone top bar (sides only under a banner), and main's end padding, alone or added to the content padding.AppShellSafeArea.a11y.chromium.spec.tsagainst a freshpnpm storybook:build: 6 pass. Insets are emulated over the Chromium protocol: portrait 62/34 on "Top nav only" and "Auto height", landscape 62 on each side with 21 at the bottom, the sidenav-only top bar, no insets, and a child that fills main ending 34 px above the bottom with no outer scroll. Against main's build atc5383172d4: 5 of 6 fail; only "with no insets nothing moves" passes.AppShellMainKeyboard.a11y.browser.spec.ts(fix(AppShell): with fill height, keyboard users can scroll an overflowing main region #8111, which shares main's style list): 16 pass in Chromium, 15 and 1 skip in WebKit.viewport-fit=coverandblack-translucent; the same page in a Safari tab reports zero insets.layerViewportInset.test.ts: AppShell joins itsGUTTER_OWNERS, the surfaces that read the device's insets outside the layer gutter (AST-059 non-goals); AppShell pads the page shell and is not an anchor-mode layer.pnpm test:ui: 681 files, 17,012 tests pass;pnpm test:node: everything passes exceptstory-tree.test.ts(test(storybook): run the story-tree check where Storybook is built, and bring the tree back to its shape #8509) and twocache.test.mjscases (fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520), which fail the same way on main.