Repository navigation
fix(a11y): name a scrolling LayoutContent, LayoutPanel or StackItem - #9985
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:spec-owner-review
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| LayoutFooter | 5.9 kB (0) · 1.6 kB CSS (0) | — | — · ✓ · ✗ |
| LayoutHeader | 6.0 kB (0) · 1.6 kB CSS (0) | — | — · ✓ · ✗ |
| StackItem | 🔴 16.1 kB (+11.5 kB) · 219 B CSS (0) | — | — · ✓ · ✓ |
13 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| LayoutPanel | 🔴 19.1 kB (+11.4 kB) · 4.9 kB CSS (0) | — | — · ✓ · ✓ |
| LayoutContent | 🔴 17.4 kB (+11.4 kB) · 1.7 kB CSS (0) | — | — · ✓ · ✓ |
| CommandPalette | 54.7 kB (+1.2 kB) · 12.3 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 229.8 kB (+1.1 kB) · 454 B CSS (0) | — | — · — · — |
| AlertDialog | 87.8 kB (+1.0 kB) · 13.1 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 260.7 kB (+796 B) · 2.4 kB CSS (0) | — | — · — · — |
| lab/Comments | 266.0 kB (+763 B) · 2.5 kB CSS (0) | — | — · — · — |
| lab/CommentComposer | 105.6 kB (+717 B) · 1.9 kB CSS (0) | — | — · — · — |
| PowerSearch | 244.8 kB (+21 B) · 22.2 kB CSS (0) | — | — · — · — |
| Layout | 6.8 kB (0) · 2.3 kB CSS (0) | — | — · ✓ · ✓ |
| Stack | 5.7 kB (0) · 1.6 kB CSS (0) | — | — · ✓ · ✓ |
| Tokenizer | 70.0 kB (0) · 17.1 kB CSS (0) | — | — · — · — |
| AppShell · Storybook | 73.2 kB (−125 B) · 12.2 kB CSS (0) | 1,096 (0) | — · ✓ · ✓ |
Package size: @astryxdesign/core 6.43 MB packed (+3.9 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 8 rows, core 2 rows; run pnpm api:report.
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
|
/approve-spec 18c7636 |
|
AI review status for this pull request.
|
github-actions
Bot
removed
the
needs:spec-owner-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: the scroll-region naming works as the approved records describe for LayoutContent, LayoutPanel and a default StackItem, but the StackItem path and the records need fixes first.
- A scrollable
StackItemrendered asnav,mainorulgetsrole="group"while it overflows (StackItemScrollRegion.tsx:46), so screen readers lose its landmark or list role, which AST-025 FR12 rules out ("group unless the viewport is itself a landmark"). - A scrollable
StackItemrendered from a Server Component no longer accepts a server-componentas(StackItem.tsx:121), which narrows existing usage in a patch[fix]against AST-017 FR53. - The LayoutContent, LayoutPanel and Stack records still say in their compatibility and at-a-glance rows that the DOM output does not change, so they should name the new
roleandaria-labeloutput and its patch[fix]class under AST-017 FR53. - Putting the
'use client'child inStack/makesserverSafeComponents.test.tsskip the./Stackentry, so a client import added toStackItem.tsxno longer fails it; moving the child beside the hook and listing it inCLIENT_ISLANDSrestores the guard.
Taking the client path only when as is a plain div or span would fix the first two together. The record edits need a fresh spec approval after the new commit.
[Automated review]
| }: StackItemScrollRegionProps) { | ||
| const region = useScrollRegionProps({ | ||
| isScrollable: true, | ||
| role: elementProps.role, |
There was a problem hiding this comment.
Only an explicit role reaches the hook here, so an as with its own role is re-roled while it overflows: in Chromium a scrolling as="nav" goes from navigation to group, as="main" from main to group, and as="ul" from list to group. AST-025 FR12 says "The role is group unless the viewport is itself a landmark". Treat an as other than div or span like a caller role: no default role or name, and no observation.
| ...props, | ||
| }; | ||
|
|
||
| if (isScrollable) { |
There was a problem hiding this comment.
From a Server Component, as now has to cross into the 'use client' StackItemScrollRegion, so a server-component as fails, as the JSDoc above notes. AST-017 FR53 does not let a patch [fix] narrow existing usage. Taking this path only for a div or span string keeps the static render for every other as (and fixes the role override too).
| - **AR2 — Keyboard scrolling.** A scrolling region is a browser scroll | ||
| container, so its overflow scrolls with the keyboard once focus is inside | ||
| it or on it. | ||
| - **AR3 — A scrolling region is named.** While the region scrolls, its |
There was a problem hiding this comment.
AR3 adds role and aria-label output, but this record's at-a-glance rows (Builder impact, Compatibility/readiness) and its Compatibility and migration list still say the DOM output does not change. Name the change and its patch [fix] class under AST-017 FR53 there, as LayoutPanel already does for its AR2. LayoutPanel.spec.md (for AR3) and Stack.spec.md need the same edit.
| @@ -0,0 +1,60 @@ | |||
| // Copyright (c) Meta Platforms, Inc. and affiliates. | |||
|
|
|||
| 'use client'; | |||
There was a problem hiding this comment.
A 'use client' file in Stack/ makes serverSafeComponents.test.ts skip ./Stack in its directive-free check, so a client import added to StackItem.tsx now passes it (the same import fails on the base branch). Moving this file beside the hook and adding it to CLIENT_ISLANDS keeps the guard that Stack's AR1/PR1 verification row relies on.
18c7636 to
71941b4
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 71941b4 |
github-actions
Bot
removed
the
needs:spec-owner-review
71941b4 to
e1722e4
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec e1722e4 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: the scroll-region naming matches the LayoutContent, LayoutPanel and Stack records an engineering owner approved for this head, and the earlier findings are fixed, but the changeset needs two corrections.
- Regression: the changeset says "a scrollable StackItem" is named, but only a
divorspanitem is (StackItem.tsx:120), so it should say "a scrollabledivorspanStackItem". - Miss: the changeset names neither the old behavior nor the accessibility target that AST-017 FR53 asks an accessibility
[fix]changeset to name, so it needs a "Before, …" sentence and the target (the records' AR3 and AR2).
A new commit voids the current /approve-spec, so after the fix an engineering owner, the author included, comments /approve-spec with the new head SHA.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] LayoutContent, LayoutPanel and a scrollable StackItem name their region while it scrolls. It is then a group named by the caller's label, or by a translated "Content", so a screen reader that reaches it announces it. A caller's role stays with only the caller's name, and Tab behavior stays the browser's. |
There was a problem hiding this comment.
Two fixes here. Only a div or span StackItem takes the naming path (StackItem.tsx:120), so "a scrollable StackItem" overstates it: say "a scrollable div or span StackItem". AST-017 FR53 also asks an accessibility [fix] changeset to name the old behavior and the accessibility target, for example: "Before, a scrolling region was a nameless element that Chromium could make a Tab stop" and the records' AR3 and AR2.
| * @position Internal hook for LayoutContent, LayoutPanel and StackItem. | ||
| * A browser can make a scrolling region a Tab stop of its own, so a screen | ||
| * reader can reach it. While it scrolls, the region is a named group, so | ||
| * the screen reader announces it. A caller's role, such as a landmark, | ||
| * stays with the caller's name. Tab behavior stays the browser's. |
There was a problem hiding this comment.
Non-blocking (INV29): this @position prose repeats the hook's JSDoc below. "Internal hook for LayoutContent, LayoutPanel and StackItem." is enough here.
| render( | ||
| <StackItem | ||
| aria-label="Results" | ||
| as="section" |
There was a problem hiding this comment.
Non-blocking: with as="section" this StackItem never reaches StackItemScrollRegion (only a div or span does), so the caller-role pass-through that Stack AR2's verification row cites is not exercised. Dropping as="section" (and the tag assertion) covers it.
While the region scrolls, it is a group named by the caller's label, or by a translated "Content". A screen reader that reaches it, including as a browser-made Tab stop, announces it. A caller's role stays, with only the caller's name, and Tab behavior stays the browser's. The three regions reuse AppShell's overflow observer, which measures a region that holds its content directly. Only a scrollable div or span StackItem measures, through a client island; any other element keeps its own role, and StackItem otherwise stays a static render. Replaces meta#9095.
e1722e4 to
175b388
Compare
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet.
The scroll-region naming for LayoutContent, LayoutPanel and a scrollable div or span StackItem matches the LayoutContent, LayoutPanel and Stack records an engineering owner approved, and Chromium's accessibility tree shows each overflowing region as a named group. The two changeset corrections from the last review are in: it now names the div or span scope, the old behavior and the AR3/AR2 target.
Next step: mark the pull request ready for review, and a maintainer adds the ci:run label so CI runs, since the branch lives in a fork.
[Automated review]
cixzhang
added
the
ci:run
Replaces #9095, which first explored this.
User impact
A screen reader reaching a scrolling LayoutContent, LayoutPanel or StackItem announces nothing about it. Chromium makes it a Tab stop when it holds nothing focusable.
Problem and solution fit
divwith no role or name unless the caller adds them, and most callers don't.While the region scrolls, it is now a group named by the caller's
label(StackItem:aria-label), or by a translated "Content". A region that fits, or cannot scroll, is unchanged.Expected behavior and authority
The spec owner decided this on Nov 4: name the scrolling region only. The records carry her line verbatim: LayoutContent AR3, LayoutPanel AR3, Stack AR2.
Flagged: I added one sentence to each: a caller's
roletakes only the caller'slabel.spec:AST-025FR12 lets AppShell's single main rely on its role, so it stays unnamed.Smallest restoration
useScrollRegionProps(internal): the region's ref, and its role and name while it overflows.divorspanStackItem renders a client island; other elements keep their role.@astryx.scrollRegion.label. No new prop and no tab index.Evidence
group "Content",group "Messages", a panel and a StackItem.ulStackItem and amainregion.useScrollRegionProps.test.tsx, 12 cases.Scope
Testing
Gates at this head, on main a0c48b8: typecheck, lint, 2,182 unit tests and the i18n, use-client and knowledge checks pass. In Chromium the new spec passes ×3, and the Layout, AppShell and Stack accessibility specs pass (44).
Writing rubric results