docs(ui-core): reconcile button accessibility contracts - #2865
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
thought (non-blocking): Accessibility audit (advisory)The sharded axe audit is report-only while the baseline and runtime budget mature.
Shard 1 reportShard 2 reportShard 3 report |
|
Admiral dispatch: Deckhand w1 is assigned independent exact-head documentation/contract verification at d8a4eb8. Trace SegmentedControl radiogroup/roving-tabindex and DropDownButton aria-haspopup=menu against shipped source/tests, verify exact doc-only F1/F2 scope and current references, run relevant gates, and report PASS or findings. No edits, approval, arming, or merge. |
|
Admiral dispatch: Bosun w7 is assigned independent accessibility-contract review at exact head d8a4eb8, distinct from Deckhand w1 verification. Adjudicate the full SegmentedControl radiogroup/roving-tabindex obligations and DropDownButton aria-haspopup=menu contract against shipped behavior, alias inheritance, doc-only scope, focused gates, and fresh CI. Keep unarmed. |
|
Verification finding at exact head d8a4eb8: cannot PASS as documentation-current. F1 is substantiated: SegmentedControl ships radiogroup/radio semantics, one roving tab stop, arrow/Home/End movement, disabled-option skipping, and RTL-aware horizontal arrows; its focused tests pass (56/56 across SegmentedControl and DropDownButton). F2 is correct: DropDownButton ships aria-haspopup="menu", matching its role="menu" popup. However, the unchanged G-DDB3 row in DropDownButton.Accessibility.md is stale: it still states aria-controls is not set, while the shipped trigger has aria-controls={menuId} and the menu has that id. That contradicts the requested no-stale-reference check. The PR otherwise has only the two intended documentation files, clean diff, and focused docs/drift gates passed. This is a documentation finding only; no approval, arming, or merge action taken. |
|
Admiral assignment: Deckhand w2 is amending retained head d8a4eb8 to correct stale G-DDB3 documentation so it matches the shipped aria-controls/menu-id relationship. Preserve doc-only scope and the correct F1/F2 changes; push evidence and keep unarmed for independent verification. |
ctwoodwa
left a comment
ctwoodwa
left a comment
There was a problem hiding this comment.
Exact-head CHANGES review for d8a4eb8a945f1c62fb416fb6f91760a71ee5029d.
Blocking — F1 closes more of the radiogroup contract than the shipped API/evidence supports.
The distinct shipped implementation does have radiogroup / radio / aria-checked, one enabled tab stop, wrapping arrows, Home/End, disabled-item skipping, and focus + onChange coupling. However:
- A radiogroup must have an accessible name.
SegmentedControlProps.nameis optional, and the component emitsaria-label={name}; an omittednametherefore ships an unnamed radiogroup. Existing tests and examples render that state. Marking G-TG3 “Resolved for the shipped SegmentedControl alias” overstates pattern conformance. See the WAI-ARIA radiogroup characteristic “Accessible Name Required: True”: https://www.w3.org/TR/wai-aria/#radiogroup - The asserted alias chain is internally contradictory.
SegmentedControl.Semantic.mdidentifiesforms/ToggleGroup.tsxas its reference implementation and says the pattern maps exactly to ToggleGroup single-select, then later admits that the shippedbuttons/SegmentedControl.tsxis a distinct, unreconciled implementation. The actual ToggleGroup still usesrole=group,aria-pressed, natural Tab order, and no arrow navigation, exactly as its Semantic/Interaction contracts say. Closing canonical ToggleGroup gap rows for an “alias” without reconciling that reference/ownership boundary creates two sources of truth.
Please make the doc-only disposition honest about the distinct implementation, keep the canonical ToggleGroup gaps open where they remain open, and reconcile SegmentedControl’s reference/contract ownership. Either enforce/document the required group name for every shipped use or narrow the conformance claim accordingly.
The current RTL test is also not reliable evidence: an outer dir="rtl" is overridden by SegmentedControl’s own provider-derived default dir="ltr", and with only two enabled choices both previous and next wrap to the same target. The implementation’s provider-driven RTL branch looks correct by source inspection, but this test does not prove it.
F2 passes. aria-haspopup="menu" is the precise explicit token for the shipped role="menu" popup (ARIA also treats legacy true as equivalent to menu). Trigger naming, aria-expanded, aria-controls, menu/menuitem ownership, disabled state, open/focus behavior, Arrow/Home/End/typeahead/Escape/Tab handling, and focus return remain intact. The same Accessibility/Interaction contract files contain older stale M1 gap text about missing controls/focus/keyboard support; that debt predates this two-line F2 correction and should be tracked separately rather than used as evidence for this edit.
Reproduced locally at the exact head:
- button-family tests: 253/253 passed (focused SegmentedControl + DropDownButton subset: 56/56)
- component-docs: 10/10; generation check: 281 pages current
- gallery manifest: 9/9; ecosystem-bar: 28/28
- static-search smoke and
git diff --check: passed - scope: exactly two Markdown contract files, 4 additions / 4 deletions
- fresh GitHub status read: exact-head checks green; PR open, clean, unarmed
No files or PR state were changed.
|
Corrected stale G-DDB3 contract statement at head 636aca6. The contract now records the shipped trigger aria-controls={menuId} and matching popup menu id (source: packages/ui-react/src/components/buttons/DropDownButton.tsx:83,117,127). Evidence: component-doc tests 10/10 and drift check passed; gallery-manifest tests 9/9 and check passed; ecosystem-bar and static-search smoke passed; git diff --check passed; fresh required CI is green, including Build & Test and Global-UX Gate. Kept unarmed for independent verification and pending Bosun accessibility-contract review. |
|
Admiral harvest: Deckhand w2 completed the bounded G-DDB3 documentation correction at exact head 636aca6. The contract now matches the shipped trigger aria-controls and popup menu id; focused docs/drift gates and fresh required CI are green. Deckhand w5 is assigned independent corrected-head verification. Keep unarmed. |
|
Admiral assignment: Deckhand w5 is independently verifying issue 2729 at exact corrected head 636aca6, tracing all SegmentedControl and DropDownButton accessibility claims to shipped source/tests and searching both contracts for remaining stale rows. Review only; do not edit, arm, or merge. |
|
Admiral assignment: Bosun w7 is performing the independent corrected-head accessibility-contract re-review for issue 2729 at exact head 636aca6, distinct from Deckhand w5 technical verification. Re-adjudicate all SegmentedControl and DropDownButton claims, neighboring gap rows, focused gates, docs drift, and fresh CI. Review only; do not edit, arm, or merge. |
|
Admiral recovered Deckhand w5’s corrected-head finding at 636aca6. The documentation now matches shipped behavior and no stale changed-row contradiction remains, but focused tests do not directly lock DropDownButton aria-controls to the rendered menu id or SegmentedControl Home/End navigation. Bosun w7 is adjudicating whether this blocks the accessibility-contract gate. Keep unarmed. |
ctwoodwa
left a comment
ctwoodwa
left a comment
There was a problem hiding this comment.
Exact-head CHANGES review for 636aca659cda01bb8daa9fd596b163b17b52752b.
The corrected G-DDB3 row is now accurate: the trigger sets
aria-controls={menuId} and the rendered role="menu" popup receives that same
id (DropDownButton.tsx:83,117,127,134). aria-haspopup="menu", menu/menuitem
roles, disabled-item skipping, focus movement, Escape/Tab handling, and dismissal
are also present in the shipped source.
Blocking 1 — G-TG3/G-TG4 still overstate SegmentedControl conformance and keep
the alias ownership contradiction.
SegmentedControlProps.nameremains optional and the component emits only
aria-label={name}(SegmentedControl.tsx:25,93-94). Existing focused tests
intentionally render the group withoutname. Aradiogrouprequires an
accessible name, so G-TG3 cannot be called fully resolved for the shipped
component in that state. See the W3C radiogroup characteristics:
https://www.w3.org/TR/wai-aria/#radiogroupSegmentedControl.Semantic.md:8,15still names
forms/ToggleGroup.tsxas the reference implementation and says the component
is an exact ToggleGroup alias, while lines 37-38 admit the shipped
buttons/SegmentedControl.tsxis a distinct implementation. The canonical
ToggleGroup implementation still uses group/pressed semantics and its
Interaction contract still documents natural Tab order without arrows.
Closing ToggleGroup's G-TG3/G-TG4 rows for a different component creates two
contradictory sources of truth.
Keep the canonical ToggleGroup gaps open where ToggleGroup remains unchanged,
reconcile SegmentedControl's actual reference/contract ownership, and document
the unnamed-radiogroup limitation unless the API is separately corrected.
Blocking 2 — neighboring DropDownButton claims and gap rows remain stale.
DropDownButton.Accessibility.md:29,40still says opening does not move focus,
activation is click-only, items do not receive focus, and arrow navigation is
deferred. The shared hook opens onto the first/last enabled item and implements
ArrowUp/Down, Home, End, Escape, Tab, and typeahead
(useMenuKeyboardNavigation.ts:16-27,43-61,77-111).DropDownButton.Interaction.md:43-46correctly lists keyboard behavior, but
G-DDB1/G-DDB2 at lines 60-61 immediately contradict it by claiming arrows and
Home/End are not implemented.DropDownButton.Semantic.md:52says the menu closes after any item click;
disabled-item clicks intentionally keep it open, as both source and the focused
test assert.
Reconcile the complete DropDownButton contract family so the corrected G-DDB3
row is not surrounded by known-false M1 prose and gap dispositions.
Focused-test evidence. The exact two component suites pass 56/56. They
directly lock aria-haspopup="menu", one SegmentedControl roving tab stop,
arrow movement, disabled skipping, RTL branch entry, focus return, and menu
dismissal. They do not directly assert that the trigger's aria-controls equals
the rendered menu id, or exercise SegmentedControl Home/End. That does not
justify widening this documentation-only PR into source changes, but the missing
regression locks should be routed before those exact behaviors are treated as
mutation-proof evidence.
Passing evidence at this exact head:
- tight scope: exactly two Markdown contract files, 5 additions / 5 deletions;
git diff --checkclean - focused SegmentedControl + DropDownButton tests: 56/56
- component-doc tests: 10/10; generated-doc drift: 281 pages current
- gallery-manifest tests: 9/9; ecosystem-bar tests: 28/28
- gallery, ecosystem-bar, ecosystem-search, and static-search drift/smoke checks:
all green - fresh GitHub read: head matches, all reported checks complete green, PR OPEN +
CLEAN,autoMergeRequest=null
No files, merge settings, or arming state were changed by this review.
|
Admiral harvest: Bosun w7 CHANGES is accepted at corrected head |
|
Admiral dispatch: Machub Deckhand a is amending corrected head |
|
Admiral dispatch: deckhand-w3 is independently verifying Machub-corrected documentation head |
|
PASS — independently verified immutable head 74e8a5b. Scope is exactly the intended seven contract/catalog files and source is unchanged. SegmentedControl is a distinct radiogroup implementation with optional naming, roving focus, RTL/disabled handling; ToggleGroup retains G-TG3/G-TG4. DropDownButton preserves G-DDB3 (aria-controls matches the popup id), menu focus/keyboard/dismissal and disabled-item behavior. Catalog/contract wording and explicit test-follow-up debt are accurate; no active stale SegmentedControl alias contradiction remains (historical adversarial logs aside). Reproduced: component-docs 10/10 plus check; gallery 9/9 plus check; ecosystem 28/28 plus both drift checks; search smoke; git diff --check. Fresh exact-head CI is green, including Build & Test and Global-UX. Review only; unarmed. |
|
Admiral harvest: Deckhand w3 independently verified corrected documentation head |
|
Admiral dispatch: Bosun w7 is taking the final independent accessibility-contract adjudication at exact head |
ctwoodwa
left a comment
ctwoodwa
left a comment
There was a problem hiding this comment.
Exact-head APPROVE for 74e8a5be4a8d73b161aa1176eb558e29d56ba96f.
No blocking findings survive re-adjudication.
- Scope and source identity: the diff is exactly the intended seven Markdown contract/catalog files. The SegmentedControl, ToggleGroup, DropDownButton, shared menu-navigation hook, and their three focused test files have identical blob SHAs at the PR base and head; no component source changed.
- SegmentedControl / ToggleGroup: the revised contract correctly identifies
buttons/SegmentedControl.tsxas a distinct controlled radiogroup implementation, accurately lists its API and optional group name, and explicitly records the unnamed-radiogroup limitation plus missing companion-contract/test debt. ToggleGroup remains owned byforms/ToggleGroup.tsx; itsgroup/aria-pressedsemantics and natural-Tab behavior are unchanged, so G-TG3/G-TG4 remain open. The prior alias/reference contradiction is gone, including the catalog row. - DropDownButton: the accessibility, interaction, and semantic claims match the shipped component and
useMenuKeyboardNavigation:aria-haspopup="menu",aria-controlsidentity, first/last enabled focus on open, disabled-item skipping, wrapping arrows, Home/End, buffered typeahead, Escape and enabled-item focus return, Tab dismissal without focus restoration, and disabled-item activation remaining open. The canonical/deprecated size vocabulary also matchesSizeAlias. G-DDB1-G-DDB4 are accurately resolved; G-DDB6/G-DDB7 honestly retain the missing direct regression-lock debt. - Local exact-head gates: focused component tests 56/56; component-docs 10/10 plus 281-page drift check; gallery-manifest 9/9 plus drift check; ecosystem-bar 28/28 plus bar/search drift checks; static-search smoke;
git diff --check. - GitHub state: required head matches; PR is OPEN, CLEAN, MERGEABLE, and unarmed. All reported checks are complete green, including Build & Test, component-docs, gallery parity, carrier/hull TS, Global-UX aggregate, four axe shards, and CodeRabbit. The ui-react check is the documented no-op twin for this docs-only path, so the local focused 56/56 run provides the source-behavior evidence.
Review only: no files, merge settings, arming state, or threads were changed.
|
Closed as superseded (Admiral, 2026-07-27, CIC-approved). Card #2729 was reconciled and merged as PR #2994 on 2026-07-22, from the opposite premise about SegmentedControl. The Design review: This PR's DropDownButton work was correct and exists nowhere else — main's gap ledger still declares G-DDB3/G-DDB4 as High/accepted-risk while the shipped code implements managed focus, wrapping arrows, Home/End, and typeahead. Carried forward as #3187, preserving main's richer The SegmentedControl alias-vs-distinct question is a genuine open contract-doctrine call for PAO/Admiral and is explicitly out of scope of #3187 — it is not settled by this closure. Root cause of the duplicate build: |
Pull request was closed
1 participant
Summary
SegmentedControl alias
aria-haspopup="menu"token for DropDownButtonNo component code changes.
Closes #2729
Validation
node --test tooling/component-docs/tests/*.test.mjs(10/10)node tooling/component-docs/scripts/build-component-docs.mjs --checknode --test tooling/gallery-manifest/tests/*.test.mjs(9/9)node --test _shared/design/ecosystem-bar/tests/*.test.mjs(28/28)node tooling/search/smoke.mjsgit diff --check