-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5107
#5107 · @HelloOjasMutreja (contributor, fork) · merged 405f1eed31a
Verdict approve + squash-merged. Head reviewed: e7f170607e3945cb7e5dec2fc9ad16983eb608f1
Chosen over #5344, which fixes the same two lines
by teaching Divider a role prop. Closes #4994.
role="listbox" only permits option/group children. Both selectors pushed a
Divider — role="separator" + aria-orientation — straight into the listbox for
a {type: 'divider'} option, so every screen-reader user of an app with a divider
in a Selector got a listbox whose children a browser may drop or mis-report.
axe: aria-required-children, critical.
No repo story opens a listbox holding a standalone divider, which is why CI never
saw it and why .github/a11y-baseline.json has no entry for it.
aria-hidden="true" on the two Divider call sites (Selector.tsx:1224,
MultiSelector.tsx:1399). The element keeps its role, its geometry and its theme
target; it just leaves the accessibility tree. This is the pattern already used a
few lines below in both files for the section heading (Selector.tsx:1252,
MultiSelector.tsx:1428) and in CommandPaletteGroup.tsx:94. Divider spreads
{...props} before its own role, so nothing in the shared component changed.
The {type: 'divider'} option type carries no label by construction
(Selector/types.ts:26-30), so nothing informative is hidden — a labeled divider
is unreachable from this path.
One worktree at the PR head merged with today's main, storybook dev, six
review-only stories; measured patched, then git checkout origin/main -- the two
source files, HMR, measured again. Probe:
{review-root}/probe-kit/probe-listbox-a11y.cjs (new, reusable).
| story | before | after |
|---|---|---|
| Selector + divider | aria-required-children critical ×1 | 0 |
| Selector + divider + section | critical ×1 | 0 |
| Selector + search + divider | critical ×1 | 0 |
| MultiSelector + divider | critical ×1 | 0 |
| MultiSelector + selectAll + divider | critical ×1 | 0 |
| MultiSelector + search + divider | critical ×1 | 0 |
Chromium a11y tree, Selector: listbox → option Apple, option Banana, separator, option Orange, option Lemon → listbox → option ×4. The search-row divider (outside
the listbox, Selector.tsx:1449 / MultiSelector.tsx:1630) is untouched and still
exposed as separator, which is correct there.
For contrast, #5344's
role="presentation" leaves aria-orientation behind and trades one critical for
two (aria-allowed-attr).
No API change. No new theme targets. themeProps('divider') and the divider
theme target still resolve; a theme selecting [role="separator"] still matches.
API no · Visual no — all six before/after frames are byte-identical (same md5), same divider rect and color · Theme no.
Nothing. One static attribute, no effect, no listener, no dependency, no bundle delta.
approve — no blocking findings. 266/266 tests pass at head-merged-with-main;
tsc --noEmit clean; the 6-day-old head merges into main with no conflict and
the merged tree differs from main by exactly this PR's five files.
R3c/R3d were asked and came back clean: the root cause is Divider always emitting
role="separator", and the two options below the call site are new Divider API
(what #5344 does — more surface, worse audit) or not using Divider at all (same
a11y tree, loses the shared theming). The same-shape probe — every role="listbox"
in core and lab — finds no other listbox that contains a Divider; menus legitimately
permit separator.
Nit not raised, because the PR was merged: both new tests assert
document.querySelector('[role="separator"]') carries aria-hidden, which pins the
attribute rather than the exposed children, and would pick the search-row divider
first in a hasSearch fixture. The behavior-pinning form is
within(getByRole('listbox')).queryByRole('separator') → null.
Thanks for this, and sorry it sat. Nice call reusing the section-heading pattern instead of touching Divider — the open listbox measures clean in Chromium. Merging.
[Reviewed by Robohands]
Nothing — posted as drafted, and merged in the same turn on her instruction.
apps/storybook/stories/MultiSelector.stories.tsx:590 still carries
"No hasSelectAll: its divider is an unallowed listbox child". That comment was
already stale on main (select-all has had no divider for a while — see the
"no divider under it" comment at MultiSelector.tsx:1370), and it is now doubly
stale. A story with hasSelectAll + isDefaultOpen + a divider would also give CI
its first look inside an open listbox, which is the gap that hid this bug.