Repository navigation
A11y: Surface required args and keyboard-reachable Setup controls in ArgsTable - #35306
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThe docs addon threads a new ChangesRequired state propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/addons/docs/src/blocks/controls/Boolean.tsx (1)
136-147: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the required cue in the unset-state fallback.
When
value === undefined, this returns the"Set boolean"button and never usesrequired, so a required arg that starts unset is still silent to assistive tech. The same setter-button pattern exists in the other controls that gate rendering on an initial value, so this accessibility gap remains on the most important path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/docs/src/blocks/controls/Boolean.tsx` around lines 136 - 147, The unset-state fallback in Boolean’s control renderer drops the required cue, so a required arg that starts undefined is announced like a normal optional setter. Update the `value === undefined` branch in `Boolean` to carry through the `required` state to the fallback `Button` (and any similar setter-button branches in related controls), preserving the accessibility cue while keeping the existing `getControlSetterButtonId` and `onSetFalse` behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/addons/docs/src/blocks/components/ArgsTable/ArgControl.tsx`:
- Around line 30-37: The default `display: none` on `SetupControlsLink` makes
the `Link` unreachable for keyboard focus, so `StyledTr` in `ArgRow` can’t ever
trigger `:focus-within` from the keyboard. Change `SetupControlsLink` to use a
visually-hidden-but-focusable style instead of removing it from the
accessibility tree, and update the hover/focus rules in `ArgRow`’s `StyledTr` to
restore the link to a visible state without relying on `display`. Keep
`NoControlPlaceholder` as the fallback for rows without controls.
In `@code/addons/docs/src/blocks/components/ArgsTable/ArgRow.tsx`:
- Line 151: The ArgControl render in ArgRow is passing a possibly undefined
required value through isRequired, and the cast to ArgControlProps is hiding the
type mismatch. Update the ArgRow/ArgControlProps usage so isRequired is always a
boolean, coercing row.type?.required to a boolean before passing it into
ArgControl and avoiding the unsafe cast where possible.
In `@code/addons/docs/src/blocks/controls/options/Radio.tsx`:
- Around line 77-82: The radio group is only announcing “required” in the legend
text, but the group itself is not marked required for assistive tech. Update the
Radio component around Wrapper and the legend so the actual radio group gets a
programmatic required state, using the existing required prop rather than
relying on the visible label text alone.
---
Outside diff comments:
In `@code/addons/docs/src/blocks/controls/Boolean.tsx`:
- Around line 136-147: The unset-state fallback in Boolean’s control renderer
drops the required cue, so a required arg that starts undefined is announced
like a normal optional setter. Update the `value === undefined` branch in
`Boolean` to carry through the `required` state to the fallback `Button` (and
any similar setter-button branches in related controls), preserving the
accessibility cue while keeping the existing `getControlSetterButtonId` and
`onSetFalse` behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 166a50be-e116-4533-a6f2-3f89d7ac48dc
📒 Files selected for processing (14)
code/addons/docs/src/blocks/components/ArgsTable/ArgControl.tsxcode/addons/docs/src/blocks/components/ArgsTable/ArgRow.tsxcode/addons/docs/src/blocks/controls/Boolean.tsxcode/addons/docs/src/blocks/controls/Color.tsxcode/addons/docs/src/blocks/controls/Date.tsxcode/addons/docs/src/blocks/controls/Files.tsxcode/addons/docs/src/blocks/controls/Number.tsxcode/addons/docs/src/blocks/controls/Object.tsxcode/addons/docs/src/blocks/controls/Range.tsxcode/addons/docs/src/blocks/controls/Text.tsxcode/addons/docs/src/blocks/controls/options/Checkbox.tsxcode/addons/docs/src/blocks/controls/options/Radio.tsxcode/addons/docs/src/blocks/controls/options/Select.tsxcode/addons/docs/src/blocks/controls/types.ts
461b9ba to
630bdcf
Compare
|
@Sidnioulz, I am seeing changes to the hovering mechanic, but that's not covered by the manual testing steps. |
It's step 4:
I did verify it. There is a story in ArgsTable where you can see it: https://635781f3500dd2c49e189caf-elyriepama.chromatic.com/?path=/story/addons-docs-components-argstable-argstable--in-addon-panel-no-controls |
What I did
These two changes were originally developed alongside the
lang-attribute work in #35207 but are unrelated, so they have been split out into this dedicated PR (and reverted on #35207).Both are accessibility improvements to the docs
ArgsTablecontrols:requiredargs to assistive tech. Therequiredflag from an arg's type is now threaded fromArgRow→ArgControl→ each control, which rendersaria-requiredon its underlying input (Boolean,Color,Date,Files,Number,Object,Range,Text,Selectsingle/multi, and theCheckbox/Radiofieldsets). Therequiredprop is added to the sharedControlPropstype.:hover(driven by JSonMouseEnter/isHovered). It now renders both the link and a placeholder dash at all times, and the row toggles their visibility purely via CSS on:hover, :focus-within(StyledTr), so keyboard focus reveals the link too. Thearia-hiddenattribute is added to the decorative required*marker.Note
This was mostly hand-coded, but split from another branch by AI. PR description is AI.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn task --task sandbox --start-from auto --template react-vite/default-tsaria-required="true"is present on the input/fieldset.Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
aria-requiredand clearer screen-reader labeling (including required markers in option legends).