fix(ButtonGroup): keep arrow keys inside a member's open menu, plus the ButtonGroup audit

[cixzhang]

ButtonGroup, full audit

First audit of this component. Graded end to end against Component Audit Rubric v1.7.1, in Chromium against a static Storybook build.

Corrections since the first push

A review of this PR found that its headline fix was a regression, plus three lesser defects. All four are corrected here.

1. The A4 guard was a regression. It bailed out whenever the key's target had any [popover] ancestor. Since useLayer wraps every layer's children in [popover], a ButtonGroup rendered inside a Popover, ContextMenu, HoverCard or Toast matched an ancestor popover and swallowed all of its own arrow keys. The group stopped working entirely in those contexts. It is now scoped through useListFocus's own boundarySelector, and both cases hold at once. Measured in Chromium, three ways, below.
2. The regression test was too weak to catch it. It asserted with two .not.toHaveFocus() calls, which pass when focus went nowhere at all, and in jsdom that is exactly what happened. It asserts positively now, and a second test covers the case the old guard broke.
3. §2 was over-scored at 5, and is 4. The 5/5 anchor asks for an extensible *VariantMap and MediaTheme verified. ButtonGroup/index.ts declares no variant map, elevation is a closed union over a closed style record, and MediaTheme was never rendered.
4. The capture count was wrong. 44 contact-sheet captures, not 34.

Before: 72.0 / C, 4 open BLOCKs. Ledger row recorded before any change: f5fac81.
After: 82.8 / B, 0 open BLOCKs. Re-derived, not patched: (4x16 + 4x14 + 4x14 + 4x12 + 5x6 + 4x4 + 4.5x8 + 4x8 + 4.5x8 + 4x5 + 4x5) / 5 = 82.8. Held until this lands, so its commit can name a real main commit.

Section	Before	After	What moved
§1 Accessibility	3	4	A4 closed, and closed both directions. A20 and A21 measured for the first time, both pass.
§2 Theming	3	4	T6 closed: a theme can reach elevation. Short of 5 on the variant map and MediaTheme.
§5b Design, rendered	3	4	The disabled state is captured now, so state coverage is complete.
§6 Testing	2.5	4.5	V6 and V10 closed, V11 closed.
§8 Docs	4	4.5	X10 at 100%, X12 and X16 closed.

§3, §4, §5a, §7, §9 and §10 are unchanged.

Fixed

A4 (BLOCK): arrow keys crossed a layer boundary in both directions

useLayer renders a DropdownMenu's menu inline inside the group rather than portaling it, which is the same fact IS_LAST_ITEM relies on. So an arrow key pressed inside the open menu bubbles to the group's useListFocus handler, getCurrentIndex() returns -1 for the focused menu item, and the group focuses one of its own buttons. The menu stays open with focus behind it.

The first fix here was a hand-rolled closest('[popover]') bail-out in a wrapping keydown handler. closest() walks up, and every layer carries [popover], so it also matched when the group itself was inside a layer. That killed arrow navigation for every ButtonGroup inside a Popover, ContextMenu, HoverCard or Toast.

The mechanism was already there, and useListFocus is where it belongs. boundarySelector scopes both halves of the problem in the hook that every other list consumer shares: an item counts as the group's own only when its nearest boundary ancestor is the group's container, and a key event is ignored when its nearest boundary ancestor is not. The boundary is '[role="group"], [popover]': the group's own root, or any layer, whichever is nearer to the event target. Naming the group's own root is the whole difference, because a group inside a Popover reaches its own role="group" before it reaches the enclosing [popover].

packages/core/src/ButtonGroup/ButtonGroup.tsx:173-183. The bespoke wrapper is gone; the group hands handleKeyDown straight to onKeyDown again.

Measured in Chromium, on one page carrying both scenarios, built three times from the same source tree with only ButtonGroup.tsx swapped:

Build	A: group inside a Popover. Focus on Copy, then ArrowRight, End, Home	B: ArrowRight inside a member's open DropdownMenu
parent b594d5c5fc4	Cut, Paste, Copy	button "Allow once", focus leaves the open menu
first fix (closest('[popover]'))	Copy, Copy, Copy, every key dead	menuitem "Always allow"
this PR (boundarySelector)	Cut, Paste, Copy	menuitem "Always allow"

Only the boundary passes both. The menu is still open in every B measurement, so B is about where focus went, not whether the layer survived.

Before	After

T6 (BLOCK): elevation selected between StyleX style objects but was missing from themeProps.
The group rendered data-size and data-orientation only, so a theme had no hook for the elevation levels. Verified by injecting the rules a theme would emit: .astryx-button-group[data-elevation="high"] matched nothing before and matches now. ButtonGroup.tsx:197, ButtonGroup.doc.mjs:18.

V6 (BLOCK): no keyboard test existed.
28 tests, none of them keyboard, on a component that wires arrow navigation, wrapping, Home and End, and RTL flipping. Eight added: arrows and wrap, Home and End, vertical orientation, RTL, disabled-member skip, a member's open menu, and a group inside a popover. The last two are the boundary, and each is red against one of the two broken builds above: the open-menu test fails on the parent commit, the in-popover test fails on the first fix, both pass here. Verified by swapping the source in place.

V10 (BLOCK): no story rendered a disabled member.
So disabled-cursor-audit and disabled-hover-audit both reported a pass over zero elements for this component (Checked 0 disabled element(s) across 0 of 12 stories), and A20 and A21 were unmeasurable. With the Disabled story they check 5 elements: no disabled member paints a hover state, and every one answers the pointer with default at all five hit-test points, which is v1.7.1's expected value.

X10, X12, X16, I15, I17 (FIX). isDisabled was the one documented prop no story exercised. Added Disabled, Elevation and LongLabels stories; added a curated targets.json entry, so rtl:audit --filter ButtonGroup now reports CUR RTL-ready ButtonGroup {"D2":"pass"} instead of nothing; added labelling guidance to usage.bestPractices.

Why §2 is 4 and not 5

T6 is genuinely closed, and everything else in the 5/5 anchor holds: every visual value is a token, themeProps sits on the painting element carrying every style-driving prop, targets[] moved in the same diff, both Vitest guards pass unmodified, and no new DOM node appeared. Two things in that anchor do not hold.

• No extensible variant map. The anchor wants *VariantMap in the component's public subpath barrel with the prop type derived from it. ButtonGroup/index.ts declares none, and elevation is Elevation, a closed union of four strings, indexed into a closed elevationStyles record. A theme cannot add a level.
• MediaTheme was never rendered. The anchor wants light, dark, a custom theme and MediaTheme verified with screenshots. The contact sheet has the first three.

Neither is a new BLOCK. T19 blocks a closed map on a themeable axis, and elevation resolves to whole shadowVars tiers, which makes it a closed system axis in the sense P18 carves out for size, not a theme-extensible one. So the honest read is 4: the 5 anchor minus two of its clauses.

Needs review

The roving-tabindex question, and which pattern this component claims (A2).
The group renders role="group", keeps every member in the tab order (three tab stops, measured), and also wires arrow-key navigation. That combination matches no APG pattern: Toolbar in this repo pairs arrow navigation with role="toolbar" and hasRovingTabIndex: true. Which model ButtonGroup should take is the open design question in #4238, so it is not settled here.

#4238 rests on a false premise, and its text is not corrected from this PR.
It states that ButtonGroup "already does BOTH the connected visual AND roving via useListFocus". It does not. ButtonGroup.tsx:173 calls useListFocus without hasRovingTabIndex, so the hook only moves focus and never touches tabindex; there is no single tab stop, and therefore no two roving layers to fight. Anyone picking that issue up would be working from a premise that does not hold. Left for @cixzhang to decide, because rewriting a design issue's framing is not an audit PR's call, but it should not sit there unflagged.

The same T6 shape sits in four sibling components, and stays there.
Card, Button, Banner and ChatComposer all apply elevationStyles[elevation] with elevation absent from their themeProps call, all from #4251. Not touched here on purpose. A four-component sweep is a different change with a different blast radius and a different reviewer, and folding it into an audit of one component would make both harder to review and harder to revert. Each will come up in its own audit, or in one sweep diff that owns the whole shape.

A member inside a group has no pressed state (B4, §5b).
Measured with the pointer down and :active matching: transform stays none because Button drops styles.pressable inside a group, and the background is identical to hover. A standalone Button gets scale(0.98). Whether a connected member should depress, and how, is a design call rather than a correction.

Long labels overflow their container (R1, I18).
The group is inline-flex and members do not wrap or truncate, so a group whose labels are wider than its container overflows it: 376px of members in a 280px container, and at a 320px viewport the document scrolls sideways (scrollWidth 392 against clientWidth 320). The new LongLabels story shows it. Wrapping, truncating or allowing members to shrink are three different answers, so this needs a decision.

ref={mergeRefs(ref, listRef)} is built during render (ButtonGroup.tsx:194).
Left alone deliberately: #5267 migrates this exact line to useMergedRefs and #5269 lints the pattern.

A coarse pointer gets no larger hit area. Icon members stay 32x32 under emulated any-pointer: coarse. Over the 24px floor, so not a finding against the group, and the sizing belongs to Button and IconButton.

Full contact sheet, 44 captures across 12 stories in light and dark, RTL, the y2k theme and forced colors: assets/pr-5355.

Checks

pnpm build and pnpm lint:strict clean. The ButtonGroup suite (36 tests) plus every other useListFocus consumer, since the fix moved onto the shared hook: AvatarGroup, Breadcrumbs, ContextMenu, DropdownMenu, NavMenu, Outline, Pagination, SegmentedControl, SideNav, TabList, TabMenu, Toolbar, TopNav, the whole hooks directory, and Popover, Toast and HoverCard for the layer side. 1198 tests over 45 files, all green. check:sync, check:changesets, check:use-client, a11y:audit --components ButtonGroup (0 violations, baseline unchanged), rtl:audit --filter ButtonGroup, and both disabled sweeps also pass locally.

Night Watch — Component Auditor

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
astryx	Ready	Preview	Aug 23, 2026 11:40am

[github-actions]

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

Modified Components

ButtonGroup _{(@astryxdesign/core)} · View in Storybook

Metric	Before	After	Delta
Bundle Size (ESM)	N/A	N/A	N/A
Lines of Code	N/A	122	-
Complexity	N/A	Low (3)	-

Bundle Size Summary

Package	Size (ESM)	Size (CJS)	Gzipped
@astryxdesign/core	N/A	4.8KB	1.2KB

Accessibility Audit

Status: No accessibility violations detected.

---

_{Generated by PR Enrichment workflow | Storybook | Sandbox | View full report}