Repository navigation
fix(AppShell): with fill height, keyboard users can scroll an overflowing main region - #8111
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
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.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/AppShell | +2.5KB |
+29B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 35.6KB | 35.7KB | +18B |
Accessibility Audit
Status: The accessibility audit did not run on this pull request. It runs after merges to main and before each release; pull requests that touch components run it with the ci:heavy-lanes label.
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.
The keyboard fix works: with height="fill", Tab now reaches an overflowing main region and Page Down scrolls it in Chromium, WebKit and Firefox, while a main that fits stays out of the tab order. Approval waits on an engineering owner settling two spec points: whether main may be a Tab stop named only by its main landmark role, with the inset focus ring (AST-025 FR12 asks for an accessible name, so either FR12 is updated or main gets one), and whether useScrollableArea may publicly accept one element as its own content box (the option AST-025 OQ1 leaves open). 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. Separately, please fix:
- The changeset and the AppShell
childrendoc say pages with interactive content keep their tab order, but a main that starts with a form field or a menu button gains one stop. AppShell.tsxkeeps main's DOM node in React state, which react-component-runtime INV1 rules out; a ref is enough.- At
height="auto"main is still observed although it can never become a stop, so attach the ref only forfill.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] With `height="fill"`, AppShell's main region can be scrolled from the keyboard when its content overflows and has nothing focusable (WCAG 2.1.1 Keyboard). Before, main was only the skip link's target (`tabIndex={-1}`): Tab passed it by, so long text, a report or a read-only page could not be scrolled without a pointer. Now, while main scrolls overflowing content, it is a stop in the tab order and keyboard focus on it draws the standard focus ring, inset. Tab entry moves on to its first link or button when there is one, so pages with interactive content keep their tab order. A main region that fits, and `height="auto"` (where the page scrolls), are unchanged. `useScrollableArea` also accepts one element as both viewport and content box, and then observes that element's direct children. |
There was a problem hiding this comment.
Tab entry moves on only when the first focusable element inside main is a plain link or button (AST-025 FR12); with a form field or a menu button first, main is one extra stop before it, so "pages with interactive content keep their tab order" overstates it. Suggest: "Tab entry moves on when the first focusable element inside main is a plain link or button; otherwise main is one stop before it."
There was a problem hiding this comment.
Fixed in cbe2a00: the changeset now says Tab entry moves on when the first focusable element inside main is a plain link or button, and that otherwise main is one stop before it.
| required: true, | ||
| description: | ||
| 'children, rendered in the main landmark. Scrolls internally when height is fill, and with the page when it is auto.', | ||
| 'children, rendered in the main landmark. Scrolls internally when height is fill, and with the page when it is auto. While it scrolls content that overflows, it is a stop in the tab order, and Tab entry moves on to its first link or button when it has one.', |
There was a problem hiding this comment.
Same wording issue as the changeset: delegation happens only when the first focusable element in main is a plain link or button, not whenever main contains one.
There was a problem hiding this comment.
Fixed in cbe2a00: the children doc now says the same: delegation happens only when main's first focusable element is a plain link or button, and otherwise main is one stop before it.
| keyboardAccess: {owner: 'content'}, | ||
| stickyContainment: 'always', | ||
| }); | ||
| const [mainElement, setMainElement] = useState<HTMLDivElement | null>(null); |
There was a problem hiding this comment.
This keeps main's DOM node in React state, which architecture:react-component-runtime INV1 rules out ("DOM nodes and whole DOM collections MUST NOT be stored in React state"). A ref that re-attaches the delegation when the node is replaced (INV6), or letting the hook attach it, avoids the state.
There was a problem hiding this comment.
Fixed in cbe2a00: main's node now lives in a callback ref, not React state. The keyboard delegation is attached while main is a scroll region and released when the node is detached or replaced, before a new node takes over.
|
|
||
| const mainInner = ( | ||
| <LayoutContent | ||
| ref={mainRef} |
There was a problem hiding this comment.
At height="auto" this ref still starts the hook's observers on main (a resize observer per direct child, a subtree mutation observer, a measurement after each mutation frame) although isMainScrollRegion can never be true there; passing the ref only when isFill avoids that (react-component-runtime INV5).
There was a problem hiding this comment.
Fixed in cbe2a00: only height="fill" passes the scroll hook's ref, so at "auto" nothing observes main. A unit test counts the observed targets inside main: none at auto, some with fill. It fails on the previous head.
There was a problem hiding this comment.
The keyboard fix works: with height="fill", Tab now reaches an overflowing main region with nothing focusable in it and Page Down scrolls it in Chromium and WebKit, while a main that fits keeps its old behavior. Approval waits on an engineering-owner decision on two spec points: whether main may be a Tab stop named only by its main landmark role, with the inset focus ring (AST-025 FR12 asks for an accessible name, so either FR12 is updated or main gets one), and whether useScrollableArea may publicly accept one element as its own content box (the option AST-025 OQ1 leaves open). 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. Independently, please:
- Reword the changeset and the AppShell
childrendoc, because a main whose first focusable element is a form field or a menu button now takes one extra Tab stop. - Own main's DOM node through a ref instead of React state in
AppShell.tsx, as react-component-runtime INV1 requires. - Attach the hook's ref only for
height="fill", since atautomain is observed although it can never become a stop. - Rebase onto
main, becausepr-a11ycurrently stops before the new AppShell step on a spec this branch predates.
[Automated review]
…n the rounded corner)
There was a problem hiding this comment.
The keyboard fix works: with height="fill", Tab now reaches an overflowing main region with nothing focusable in it and Page Down scrolls it in Chromium, Firefox and WebKit, while a main that fits keeps its old tab order. Approval waits on an engineering-owner decision on three spec points: whether main may be a Tab stop named only by its main landmark role (AST-025 FR12 asks for an accessible name), whether the inset focus ring and the new rounding of main's top-start corner, which also cuts content on fitting and auto mains, are the intended treatment, and whether useScrollableArea may publicly accept one element as its own content box (the option AST-025 OQ1 leaves open). 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. Independently, please:
- Reword the changeset and the AppShell
childrendoc, because a main whose first focusable element is a form field now takes one extra Tab stop, and fitting andautomains change at their top-start corner. - Own main's DOM node through a ref instead of React state in
AppShell.tsx, as react-component-runtime INV1 requires. - Attach the hook's ref only for
height="fill", since atautomain is observed although it can never become a stop.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] With `height="fill"`, AppShell's main region can be scrolled from the keyboard when its content overflows and has nothing focusable (WCAG 2.1.1 Keyboard). Before, main was only the skip link's target (`tabIndex={-1}`): Tab passed it by, so long text, a report or a read-only page could not be scrolled without a pointer. Now, while main scrolls overflowing content, it is a stop in the tab order and keyboard focus on it draws the standard focus ring, inset and following the rounded corner of the elevated surface. Tab entry moves on to its first link or button when there is one, so pages with interactive content keep their tab order. A main region that fits, and `height="auto"` (where the page scrolls), are unchanged. `useScrollableArea` also accepts one element as both viewport and content box, and then observes that element's direct children. |
There was a problem hiding this comment.
Two parts of this note don't match the code. Tab entry moves on only when the first focusable element inside main is a plain link or button (AST-025 FR12); with a form field or a menu button first, main is one extra stop before it. And since the corner commit, a fitting main and height="auto" in an elevated shell with a top nav and an inline side nav round off content at their top-start corner, so they are not unchanged. Suggest: "Tab entry moves on when the first focusable element inside main is a plain link or button; otherwise main is one stop before it", plus a line for the corner if it stays.
There was a problem hiding this comment.
Fixed in cbe2a00: the Tab-entry wording is as suggested. The corner now applies only while main is a keyboard stop, so fitting and auto mains are unchanged, and the changeset's corner line says that. The ring treatment stays part of the owner decision.
| xstyle={[contentAreaStyle, styles.mainFocusTarget]}> | ||
| xstyle={[ | ||
| contentAreaStyle, | ||
| shouldElevateWithCorner && styles.mainElevatedCorner, |
There was a problem hiding this comment.
This rounding applies whenever the shell elevates with a corner, at both heights and whether or not main overflows, so content touching main's top-start corner (a full-bleed header or image) is now cut round on pages where main is never a stop and no ring is drawn. If the goal is only the ring's shape, limiting it to isMainScrollRegion keeps fitting and auto mains as they were; either way it is part of the owner decision on the ring.
There was a problem hiding this comment.
Fixed in cbe2a00: the rounding now applies only while main is a scroll region (a keyboard stop), where it shapes the focus ring. Fitting and auto mains keep their corner, and the fitting-main browser test now asserts a square corner; it fails on the previous build with 28px.
457d6f8 to
cbe2a00
Compare
cbe2a00 to
8910dfe
Compare
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
Agreed, the observers belong to the shared owner. In b7aa07f, |
There was a problem hiding this comment.
The keyboard fix works: with height="fill", Tab now reaches an overflowing main region and Page Down scrolls it, while a main that fits, the skip link and height="auto" behave as before. Approval waits on an engineering-owner decision on four spec points: updating AST-025 FR12 for a Tab stop named only by its main landmark role, updating AppShell PR1 for observing main in fill, and providing or identifying the governing spec for the inset focus ring with its rounded corner and for useScrollableArea accepting one element as its own content box (the option AST-025 OQ1 leaves open). 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. One change is needed either way:
- The
[fix]changeset also announcesuseScrollableArea's new one-element shape, a compatible capability that AST-017 FR7 declares as[feat], so move that sentence into its own[feat]changeset if the shape stays, or drop it if the shape goes.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] With `height="fill"`, AppShell's main region can be scrolled from the keyboard when its content overflows and has nothing focusable (WCAG 2.1.1 Keyboard). Before, main was only the skip link's target (`tabIndex={-1}`): Tab passed it by, so long text, a report or a read-only page could not be scrolled without a pointer. Now, while main scrolls overflowing content, it is a stop in the tab order and keyboard focus on it draws the standard focus ring, inset; in an elevated shell with a top nav and an inline side nav, main then takes the surface's rounded top-start corner so the ring follows it. Tab entry moves on when the first focusable element inside main is a plain link or button; otherwise main is one stop before it. A main region that fits, and `height="auto"` (where the page scrolls), are unchanged. `useScrollableArea` also accepts one element as both viewport and content box, and then observes that element's direct children. |
There was a problem hiding this comment.
This [fix] note also announces a new public capability: useScrollableArea accepting one element as both viewport and content box. AST-017 FR7 declares compatible capabilities as [feat] (still a patch bump on 0.x), as main's CodeBlock endContent changeset does. If the one-element shape stays after the owner decision on AST-025 OQ1, move this sentence into its own [feat] changeset; if it goes, drop the sentence.
There was a problem hiding this comment.
Moved in d7317a2: the [fix] note no longer mentions useScrollableArea's one-element shape, which now has its own [feat] changeset, scrollable-area-single-element.md (AST-017 FR7). If the owner decision on AST-025 OQ1 drops the shape, that changeset goes with it.
There was a problem hiding this comment.
The keyboard fix works: with height="fill", Tab now reaches an overflowing main region and Page Down scrolls it in Chromium, Firefox and WebKit, while a main that fits, the skip link and height="auto" behave as before, and no further code change is needed. Approval waits on an engineering-owner decision on four spec points: updating AST-025 FR12 for a Tab stop named only by its main landmark role, updating AppShell PR1 for observing main in fill, and providing or identifying the governing spec for the inset focus ring with its rounded corner and for useScrollableArea accepting one element as its own content box (the option AST-025 OQ1 leaves open). 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; a point an owner declines instead needs its matching code change, such as a catalog-backed name for main or an internal content box.
[Automated review]
…names Byte-identical copies of each clip and still, named by pull request only.
There was a problem hiding this comment.
The keyboard fix works: with height="fill", Tab now reaches an overflowing main region and Page Down scrolls it, while a main that fits, the skip link and height="auto" behave as before, and no further code change is needed. Approval waits on an engineering-owner decision on four spec points: updating AST-025 FR12 for a Tab stop named only by its main landmark role, updating AppShell PR1 for observing main in fill, and providing or identifying the governing spec for the inset focus ring with its rounded corner and for useScrollableArea accepting one element as its own content box (the option AST-025 OQ1 leaves open). 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; a point an owner declines instead needs its matching code change, such as a catalog-backed name for main or an internal content box.
[Automated review]
d2bb6ef to
39ab04f
Compare
There was a problem hiding this comment.
The keyboard fix works: with height="fill", Tab now reaches an overflowing main region and Page Down scrolls it to the end in Chromium and WebKit, and no code fix is outstanding. Approval waits on an engineering-owner decision on four points the current specs do not settle: main's stop is named only by its main landmark role where AST-025 FR12 asks for an accessible name, AppShell now observes main in fill where AppShell PR1 says it observes nothing, no spec covers the inset focus ring with its rounded corner, and AST-025 OQ1 still leaves the public one-element useScrollableArea shape open. Any engineering owner, the author included, can record that decision by commenting /approve-spec <full head SHA> on this pull request, or say which point needs a code change instead (a catalog-backed name, another indicator, or an internal content box), with the records on main updated to match.
[Automated review]
39ab04f to
7d439ae
Compare
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
The keyboard fix works: with height="fill", Tab now reaches an overflowing main region and Page Down scrolls it to the end in Chromium and WebKit, and its role-only name is now covered by AST-025 FR12. Approval waits on an engineering-owner decision on the two AppShell record amendments this pull request makes: PR1 now lets AppShell observe main, its direct children and its subtree in fill, and FR4 now adds an inset Tab focus ring with a rounded corner that no current spec covers. Any engineering owner, the author included, can record that decision by commenting /approve-spec <full head SHA> on this pull request, or say which point needs a code change instead. Separately, the FR6/PR1 verification row in AppShell.spec.md still says measuring in fill fails; it should say that measuring the header there, or not measuring main, fails.
[Automated review]
| | FR1–FR4, AR1 | `AppShell.test.tsx` slot, landmark, banner, and ref suites; `AppShellMainKeyboard.a11y.browser.spec.ts` for main as a tab stop and its focus ring | Content only; each slot; mobile top bar with banner; Tab, Shift+Tab, click and skip link onto an overflowing main | A missing or duplicate landmark, or a nested banner, fails role queries. | `audit:AppShell/anatomy` | | ||
| | FR5, AR2 | `AppShell.test.tsx` skip-link and keyboard suites | First Tab; Enter; click; translated text | Losing the href, the focus move, or tab order fails. | `audit:AppShell/accessibility` | | ||
| | FR6, PR1 | `AppShell.test.tsx` height-mode suites | `fill`, `auto`, section header in `auto` | Dropping the header's sticky placement or measuring in `fill` fails. | `audit:AppShell/behavior` | | ||
| | FR6, PR1 | `AppShell.test.tsx` height-mode and main keyboard access suites; `observeMainOverflow.test.ts` | `fill`, `auto`, section header in `auto`; main fitting and overflowing in `fill` | Dropping the header's sticky placement or measuring in `fill` fails. | `audit:AppShell/behavior` | |
There was a problem hiding this comment.
This row now lists the main-overflow tests, but its failure expectation still reads "measuring in fill fails", while the rewritten PR1 above requires measuring main in fill. Suggest: "Dropping the header's sticky placement, measuring the header in fill, or not measuring main there fails."
…wing main region With `height="fill"`, main becomes a sequential tab stop while its content overflows (spec:AST-025 FR12), and Tab focus on it draws the standard ring, inset (component:AppShell FR4). AppShell measures main through an internal observer that watches main's direct children, since main has no inner content box; the public useScrollableArea is unchanged. The AppShell record states the observation in PR1 and the ring in FR4.
7d439ae to
399ea1a
Compare
|
/approve-spec 399ea1a |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
The keyboard fix works: with height="fill", Tab now reaches an overflowing main region and Page Down scrolls it to the end in Chromium and WebKit, while the skip link and a click stay ringless. The AppShell record changes (PR1 and FR4) carry an engineering owner's spec approval at this head, and the verification row now matches the rewritten PR1. No further changes are needed from this review; CI's build and test jobs still have to run once the pull request leaves draft.
[Automated review]
cixzhang
added
the
ci:run
…e it overflows (#8155) ## User impact A keyboard user who tabs onto a Carousel's scroll area lands on an element with no role and no name, so a screen reader announces nothing. The same unnamed stop appears even when everything fits. The edge buttons hand focus here when their direction runs out (component:Carousel FR2). ## Evidence Chromium, keys captioned: Tab rings the › button, a second Tab focuses the scroll area, → → → ← scroll it. Its ring is the browser's default outline.  - Before: the binding's `scroller-keyboard` state failed `carousel.scroller.role-and-name`, recorded as a known failure. After: it passes 8/8 with no known failures, and axe reports 0 violations on every Carousel story. - New unit tests fail without the change: the named group, the tab stop following overflow, and the tab stop following slides added, removed or grown with the box unchanged. Another pins the translated default name. - In Chromium and WebKit, before the re-measure, a fixed-width carousel's tab stop went stale as its content changed, and Scroll right stayed disabled after a slide was added at the end; both follow now. ## Problem and solution fit - The scroll area is `role="group"`, named like the carousel (the caller's `aria-label`, else the translated "Carousel"), so landing on it says where the user is. - It is a tab stop (`tabIndex={0}`) only while its content overflows; while it fits it is `tabIndex={-1}`, which keeps the edge buttons' programmatic hand-off (FR2) working. - The overflow is measured again when the content changes inside an unchanged box: `useScrollOverflow` now observes each child's box, children added or removed, and the container's class and style (`spec:AST-025` FR5). The edge buttons and fades, and TabList's fades, follow the same measure. ## Expected behavior and authority `spec:AST-025` FR12: a viewport that takes keyboard scrolling joins the tab order only while it overflows, with a role and an accessible name; as #9088 states it, a `group` named by the component that owns it, the caller's label, else its translated default. `component:Carousel` FR2 names it the hand-off target, out of the tab order when the content fits. ## Smallest restoration - Three attributes on the scroll area; `useScrollOverflow` also observes the content, with its API unchanged. - `Carousel.doc.mjs` and the Carousel record's review table describe the new behavior; the binding's known failure is removed. - Tab entry does not delegate to a first link or button (FR12's optional path); that needs #8111's shared delegation. ## Writing rubric results R1 narration: a projection row called the gap pending; it now states the behavior. R2–R6: none. ## Scope - [x] One defect is restored. - [x] Separable contradictory or unsettled tagalongs were removed or split. - [x] No new public API, visual representation, default, or interaction model is hidden under the bug-fix label. - [x] Public text and artifacts contain no internal Meta context. ## Testing Local gates at a2184a1: - `pnpm exec vitest run packages/core/src/Carousel packages/core/src/hooks/useScrollOverflow.test.tsx packages/core/src/TabList`: 5 files, 158 tests passed. - `pnpm exec playwright test packages/core/src/Carousel/__tests__/Carousel.a11y.chromium.spec.ts`: 8 passed. - `pnpm a11y:audit -- --components core/Carousel`: 0 violations, gate passed. - `pnpm test:ui`: 16,462 passed. The node project: 14,563 passed; its only failures are `packages/cli/foundation/release/cache.test.mjs` (fixed by #8520) and `apps/storybook/.storybook/story-tree.test.ts` (#8509), unrelated to this change. - Root build and typecheck, `CI=true pnpm lint`, `pnpm check:knowledge`, `pnpm check:changesets` and its coverage, the Storybook build, the interaction config (136 passed), `pnpm check:repo`, Prettier: clean. - Size: 10 files, 321 lines added, 43 removed. Sibling: #7906 and #8153 also edit Carousel.tsx; whichever lands second rebases.
Siblings: #8066, #6314.
Why
With
height="fill", a keyboard user can't scroll AppShell's main region when it holds nothing focusable: Tab skips it, so Page Down and the arrow keys do nothing. axe reports it asscrollable-region-focusable, a serious violation.What led to it
Part of the touch-presentations program's work on AppShell scrolling (
spec:AST-094), alongside #8066, which changes which element scrolls. Main was only the skip link'stabIndex={-1}target, which Tab never stops on.Options and tradeoffs
spec:AST-025FR12 asks; it costs measuring main, which happens only withfill.Why this choice
The second follows FR12 and AppShell FR4: a viewport joins the tab order only while it overflows, and Shift+Tab skips it. A fitting main, and
height="auto", keeptabIndex={-1}. Only Tab focus draws main's ring, inset and following the surface's rounded corner; a click, the skip link or a script's focus draws none.Main has no inner content box, so an observer internal to AppShell watches main's direct children; the public
useScrollableAreais unchanged, and its one-element form is a separate draft that waits on AST-025 OQ1. The AppShell record states the observation (PR1) and the ring (FR4), and drops the gap this closes. Main's stop is named by itsmainrole alone, which the FR12 ruling allows for the single main.How to check it
In Chromium, on the AppShell Playground story with
height="fill"and 40 rows:Evidence
Before (main) and after (this PR):
Tab, then Shift+Tab, with a link inside; the ring:
AppShellMainKeyboard.a11y.browser.spec.tscovers each step: 16 of 16 in Chromium, and 15 of 15 in WebKit, where the window-switch case is skipped.main, not ignored and unnamed, so its role is announced; WebKit computes the same role. The test fails on main (Tab never reaches main) and when main is given a name.fill/autoswitch doesn't restart it; each of the observer's 5 tests fails when its behavior is removed.cache.test.mjsandstory-tree.test.ts, which fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520 and test(storybook): run the story-tree check where Storybook is built, and bring the tree back to its shape #8509 fix.Writing rubric results