Repository navigation
fix(Carousel): the scroll area is a named group, a tab stop only while it overflows - #8155
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report
Modified ComponentsCarousel (@astryxdesign/core)
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 | 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.
Approval waits on one engineering-owner decision and one fix. No current spec chooses the scroll area's accessible name, so an engineering owner needs to accept the carousel's own label, or name the source the scroll area should use, by commenting /approve-spec <full head SHA> on this pull request; the author can record that decision as an engineering owner. Separately, tabIndex={hasOverflow ? 0 : -1} relies on useScrollOverflow, which re-measures only when the scroll area itself scrolls or resizes, so slides added after mount inside an unchanged box (for example a one-slide-per-view carousel fed by async data) leave an overflowing scroll area out of the tab order with both edge buttons hidden: please re-measure when the content changes, keep -1 while it fits for the edge-button hand-off, and add a test that adds slides without a scroll or resize. The accessibility guidance in Carousel.doc.mjs should also stop describing the scroll area as an unconditional tab stop.
[Automated review]
| // programmatically, for the edge buttons' hand-off (FR2). | ||
| role="group" | ||
| aria-label={ariaLabel} | ||
| tabIndex={hasOverflow ? 0 : -1} |
There was a problem hiding this comment.
hasOverflow comes from useScrollOverflow, which re-measures only on this element's own scroll events and own-box resize. Slides appended after mount inside an unchanged box (a one-slide-per-view carousel fed by async data) never flip it: in Chromium, 1280px of content in a 320px area stays at tabindex="-1" with both edge buttons hidden, so Tab skips the carousel and the arrow keys never reach it, where the base kept a reachable stop. spec:AST-025 FR12 requires overflowing content to stay keyboard reachable and leaves geometry to FR5 ("A viewport-only ResizeObserver is insufficient because content may grow while the viewport's own box remains unchanged"). Re-measuring when the slides change (or reading the shared useScrollableArea measurement) while keeping -1 for the fitting case would close it; a test that rerenders with more slides and a wider scrollWidth without firing scroll would pin it.
cbf248b to
dc7c557
Compare
There was a problem hiding this comment.
Approval waits on one engineering-owner decision and one fix. No current spec chooses the scroll area's accessible name, so an engineering owner needs to accept the carousel's own label, or name the source the scroll area should use, by commenting /approve-spec <full head SHA> on this pull request; the author can record that decision as an engineering owner. Separately, tabIndex={hasOverflow ? 0 : -1} relies on useScrollOverflow, which re-measures only when the scroll area itself scrolls or resizes, so slides added after mount inside an unchanged box (for example a one-slide-per-view carousel fed by async data) leave an overflowing scroll area out of the tab order with both edge buttons hidden: please re-measure when the content changes, keep -1 while it fits for the edge-button hand-off, and add a test that adds slides without a scroll or resize. The accessibility guidance in Carousel.doc.mjs should also stop describing the scroll area as an unconditional tab stop.
[Automated review]
| // programmatically, for the edge buttons' hand-off (FR2). | ||
| role="group" | ||
| aria-label={ariaLabel} | ||
| tabIndex={hasOverflow ? 0 : -1} |
There was a problem hiding this comment.
hasOverflow comes from useScrollOverflow, which re-measures only on this element's own scroll events and own-box resize. Slides appended after mount inside an unchanged box (a one-slide-per-view carousel fed by async data) never flip it: in Chromium, 1280px of content in a 320px area stays at tabindex="-1" with both edge buttons hidden, so Tab skips the carousel and the arrow keys never reach it, where the base kept a reachable stop. spec:AST-025 FR12 requires overflowing content to stay keyboard reachable and leaves geometry to FR5 ("A viewport-only ResizeObserver is insufficient because content may grow while the viewport's own box remains unchanged"). Re-measuring when the slides change (or reading the shared useScrollableArea measurement), while keeping -1 for the fitting case, would close it; a test that rerenders with more slides and a wider scrollWidth without firing scroll would pin it.
There was a problem hiding this comment.
Fixed in a2184a1: useScrollOverflow now also observes each slide's box, slides added or removed, and the scroll area's class and style (spec:AST-025 FR5), so slides appended inside an unchanged box make it a tab stop and show the edge buttons, and it stays -1 while everything fits. The new tests add and grow slides without a scroll or resize, and the doc's guidance is updated. Naming follows FR12 as #9088 states it, with a test for the translated default.
… area's own size The scroll area is a tab stop only while its content overflows, and its edge buttons and fades follow the same measure. The content can come to overflow, or stop, with the scroll area keeping its size. These fail on `useScrollOverflow` as it stands, which measures only on the container's own scroll and resize: - a child added or removed, a child that resizes, and a new class or style on the container (a gap) each measure again; - once the container detaches, nothing observes its content; - Carousel's tab stop follows slides added or removed and a slide that grows, with the scroll area keeping its size.
…nly the container `useScrollOverflow` measured on the container's own scroll and resize, so content that came to overflow, or stopped, with the container keeping its size left the state stale: a fixed-width Carousel kept or lost its tab stop wrongly, and a Carousel scrolled to its end kept Scroll right disabled after a slide was added. The hook now also observes each child's size (a slide's content growing), the children added or removed, and the container's class and style (a new gap or padding), and measures again on each, still only while the container is rendered. Every observation ends when the container detaches.
…overflows The accessibility guidance said the scroll container is a tab stop. It is one while its content overflows, so keyboard users can pan it with the arrow keys; while everything fits it stays out of the tab order.
…lt name A Tab-able scroll area is a group named by the component that owns it: the caller's label, else that component's translated default. The caller's label was tested; this pins the default, so a carousel with no `aria-label` still names its scroll area "Carousel".
b85821e to
a2184a1
Compare
cixzhang
added
the
ci:run
|
/approve-spec a2184a1 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
This looks ready to merge: the Carousel scroll area is now a named group that joins the tab order only while it overflows, and slides added or grown inside an unchanged box now make it a tab stop and bring back the edge buttons, which the new tests and a Chromium check confirm. The naming follows the current scrollable-container spec (spec:AST-025 FR12), and the Carousel record edit has an engineering owner's approval on this head. No changes are needed.
[Automated review]
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.
scroller-keyboardstate failedcarousel.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.Problem and solution fit
role="group", named like the carousel (the caller'saria-label, else the translated "Carousel"), so landing on it says where the user is.tabIndex={0}) only while its content overflows; while it fits it istabIndex={-1}, which keeps the edge buttons' programmatic hand-off (FR2) working.useScrollOverflownow observes each child's box, children added or removed, and the container's class and style (spec:AST-025FR5). The edge buttons and fades, and TabList's fades, follow the same measure.Expected behavior and authority
spec:AST-025FR12: 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, agroupnamed by the component that owns it, the caller's label, else its translated default.component:CarouselFR2 names it the hand-off target, out of the tab order when the content fits.Smallest restoration
useScrollOverflowalso observes the content, with its API unchanged.Carousel.doc.mjsand the Carousel record's review table describe the new behavior; the binding's known failure is removed.Writing rubric results
R1 narration: a projection row called the gap pending; it now states the behavior. R2–R6: none.
Scope
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 arepackages/cli/foundation/release/cache.test.mjs(fixed by fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520) andapps/storybook/.storybook/story-tree.test.ts(test(storybook): run the story-tree check where Storybook is built, and bring the tree back to its shape #8509), unrelated to this change.CI=true pnpm lint,pnpm check:knowledge,pnpm check:changesetsand its coverage, the Storybook build, the interaction config (136 passed),pnpm check:repo, Prettier: clean.Sibling: #7906 and #8153 also edit Carousel.tsx; whichever lands second rebases.