Addon-Docs: Improve object control JSON editor accessibility#34108
Addon-Docs: Improve object control JSON editor accessibility#34108anchmelev wants to merge 25 commits intostorybookjs:nextfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds structured raw-JSON editing UI to the Object control with inline parse-error display and improved ARIA attributes; resets parse errors when switching modes or when the JSON string changes. Also adds Storybook play tests covering validation and error-reset flows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/addons/docs/src/blocks/controls/Object.tsx`:
- Around line 233-257: parseError persists across remounts of the uncontrolled
RawInput, so when the textarea is rebuilt from jsonString the old error state
stays shown; fix this by clearing the parseError when the raw editor mounts or
when jsonString/forceVisible changes. Add a useEffect in the component that
contains rawJSONForm (or in the same component using parseError state) that
calls setParseError(undefined) with a dependency on jsonString and/or
forceVisible (or runs once on mount) so that when RawInput (key={jsonString}) is
remounted the aria-invalid and ErrorMessage are cleared until new validation
runs via updateRaw.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b967165c-5819-42ca-af3c-495ca6cf461b
📒 Files selected for processing (2)
code/addons/docs/src/blocks/controls/Object.test.tsxcode/addons/docs/src/blocks/controls/Object.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (3)
code/core/src/component-testing/components/InteractionsPanel.test.tsx (1)
16-39: Keepcallsin sync with the overridden interaction state.
createProps()hardcodescallsfromCallStates.DONE, but the running/errored path only swapsinteractions. That leavescallsByIdout of sync with the rendered rows and can mask regressions inMethodCallor exception rendering.♻️ One way to keep the fixture coherent
-const createProps = (overrides: Partial<InteractionsPanelProps> = {}): InteractionsPanelProps => ({ +const createProps = ( + callState: CallStates = CallStates.DONE, + overrides: Partial<InteractionsPanelProps> = {} +): InteractionsPanelProps => ({ storyUrl: 'http://localhost:6006/?path=/story/core-component-test-basics--step', status: 'completed', controls: { start: vi.fn(), back: vi.fn(), goto: vi.fn(), next: vi.fn(), end: vi.fn(), rerun: vi.fn(), }, controlStates: { detached: false, start: true, back: true, goto: true, next: true, end: true, }, - interactions: getInteractions(CallStates.DONE), - calls: new Map(getCalls(CallStates.DONE).map((call) => [call.id, call])), + interactions: getInteractions(callState), + calls: new Map(getCalls(callState).map((call) => [call.id, call])), api: { openInEditor: vi.fn() } as unknown as API, ...overrides, });- createProps({ - status: 'playing', - interactions: getInteractions(CallStates.ACTIVE), - }) + createProps(CallStates.ACTIVE, { status: 'playing' })- {...createProps({ - status: 'errored', - interactions: getInteractions(CallStates.ERROR), - })} + {...createProps(CallStates.ERROR, { status: 'errored' })}Also applies to: 83-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/component-testing/components/InteractionsPanel.test.tsx` around lines 16 - 39, createProps currently builds interactions from a possibly-overridden CallStates but always constructs calls from CallStates.DONE, causing calls (the Map passed as calls/callsById) to diverge from interactions; update createProps so calls are derived from the same state as interactions (e.g., compute const state = overrides.state ?? CallStates.DONE or infer state from overrides.interactions, then call getCalls(state) when building calls) so getCalls and getInteractions use the same CallStates and the MethodCall rows and exception rendering remain in sync.code/core/src/component-testing/components/InteractionsPanel.tsx (1)
145-156: Derive the spoken error state from the same sources as the visible error UI.
statusAnnouncementonly keys offhasException, but this component can also surface failures viahasResultMismatch,caughtException, andunhandledErrors. A sharedhasErrorsflag here would keep the live-region copy aligned with what the panel actually renders.♻️ Possible simplification
- const statusAnnouncement = + const hasErrors = + hasException || + hasResultMismatch || + !!caughtException || + (unhandledErrors?.length ?? 0) > 0; + const statusAnnouncement = status === 'rendering' ? 'Component test is rendering.' : status === 'playing' ? 'Component test is running.' : status === 'errored' ? 'Component test failed.' : status === 'aborted' ? 'Component test was aborted.' - : hasException + : hasErrors ? 'Component test completed with errors.' : 'Component test completed successfully.';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/component-testing/components/InteractionsPanel.tsx` around lines 145 - 156, Create a unified error flag (e.g., hasErrors) in the InteractionsPanel and set it to true when any of hasException, hasResultMismatch, caughtException, or unhandledErrors is truthy; then update statusAnnouncement to use hasErrors instead of only hasException so the spoken/live-region state matches the visible error UI (update the logic around statusAnnouncement and reference the existing symbols: statusAnnouncement, hasException, hasResultMismatch, caughtException, unhandledErrors, hasErrors).code/core/src/component-testing/components/Interaction.tsx (1)
142-151: Expand aria-label for non-step calls to include path and args context.
aria-labelcurrently falls back to onlycall.methodfor non-step calls, discarding thepathandargsthat<MethodCall />renders visually. For example, a call likeuserEvent.click(button Click)gets the aria-label "click", losing all context about the path and target. The same fallback is also used by the nested-toggle label at lines 258-260.Consider building the aria-label from
call.pathandcall.argsfor non-step calls to match the visual rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/component-testing/components/Interaction.tsx` around lines 142 - 151, getInteractionLabel currently returns only call.method for non-step calls, losing the path and args context that <MethodCall /> shows; update getInteractionLabel(Call) to build a descriptive aria-label by concatenating call.method with a representation of call.path (e.g., join path segments) and call.args (stringify/map args to readable tokens) so aria-label matches the visual "<MethodCall />" output, and apply the same construction for the nested-toggle label that uses getInteractionLabel so both places include path and args context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/component-testing/components/Interaction.tsx`:
- Around line 142-151: getInteractionLabel currently returns only call.method
for non-step calls, losing the path and args context that <MethodCall /> shows;
update getInteractionLabel(Call) to build a descriptive aria-label by
concatenating call.method with a representation of call.path (e.g., join path
segments) and call.args (stringify/map args to readable tokens) so aria-label
matches the visual "<MethodCall />" output, and apply the same construction for
the nested-toggle label that uses getInteractionLabel so both places include
path and args context.
In `@code/core/src/component-testing/components/InteractionsPanel.test.tsx`:
- Around line 16-39: createProps currently builds interactions from a
possibly-overridden CallStates but always constructs calls from CallStates.DONE,
causing calls (the Map passed as calls/callsById) to diverge from interactions;
update createProps so calls are derived from the same state as interactions
(e.g., compute const state = overrides.state ?? CallStates.DONE or infer state
from overrides.interactions, then call getCalls(state) when building calls) so
getCalls and getInteractions use the same CallStates and the MethodCall rows and
exception rendering remain in sync.
In `@code/core/src/component-testing/components/InteractionsPanel.tsx`:
- Around line 145-156: Create a unified error flag (e.g., hasErrors) in the
InteractionsPanel and set it to true when any of hasException,
hasResultMismatch, caughtException, or unhandledErrors is truthy; then update
statusAnnouncement to use hasErrors instead of only hasException so the
spoken/live-region state matches the visible error UI (update the logic around
statusAnnouncement and reference the existing symbols: statusAnnouncement,
hasException, hasResultMismatch, caughtException, unhandledErrors, hasErrors).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b47552d2-4bac-4599-a363-ef54da681c57
📒 Files selected for processing (3)
code/core/src/component-testing/components/Interaction.tsxcode/core/src/component-testing/components/InteractionsPanel.test.tsxcode/core/src/component-testing/components/InteractionsPanel.tsx
anchmelev
force-pushed
the
fix/issue-24150-object-control-json-a11y
branch
from
2e6bbc4 to
1fd2f95
Compare
|
Hi @Sidnioulz, gentle ping on this PR as part of #24150. |
Sidnioulz
changed the title
Sidnioulz
added
bug
accessibility
addon: interactions
ci:normal
addon: docs
and removed
addon: interactions
labels
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
Hey @Sidnioulz |
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for the PR! Great work with the ARIA attributes!
Let's discuss the best approach for the ToggleButton, and I'll then be able to approve.
|
|
||
| const ErrorMessage = styled.p(({ theme }) => ({ | ||
| margin: 0, | ||
| color: theme.color.negative, |
There was a problem hiding this comment.
nit: please make sure to test this on light and dark themes.
In other areas of the codebase, we have:
color: theme.base === 'dark' ? theme.color.negative : theme.color.negativeTextThere was a problem hiding this comment.
Good call, I aligned the error text color with the existing light/dark theme pattern and checked it in both themes.
anchmelev
force-pushed
the
fix/issue-24150-object-control-json-a11y
branch
from
63f3818 to
3890b50
Compare
…adjustments and state management
…nput and label for better alignment
# Conflicts: # code/addons/docs/src/blocks/controls/Object.tsx # code/core/src/components/components/Button/Button.stories.tsx # code/core/src/components/components/Button/Button.tsx
…t-control-json-a11y
…fix/issue-24150-object-control-json-a11y # Conflicts: # code/core/src/components/components/Button/Button.tsx # code/core/src/components/components/Button/helpers/InteractiveTooltipWrapper.tsx
…fix/issue-24150-object-control-json-a11y
|
Hi @Sidnioulz, gentle follow-up on this PR. |
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
I ended up finding a bug that shows up when using the control and switching from empty to filled.
Go to https://635781f3500dd2c49e189caf-qmqwwhnlky.chromatic.com/?path=/story/addons-docs-components-argstable-argstable--all-controls&globals=;sb_theme:dark, and click the object control to set it.
Besides that, we need to find a context where a filled Object control sits side by side with other filled controls to check if spacings look good (I'd prefer to have a new ArgsTable AllControlsFilled story for that, so we have visual regression testing).
…active JSON editing; enhance ObjectControl with JSON error handling
Thanks, I fixed the empty -> filled bug and added the requested story coverage. I tracked this back to Separate note: one of the current fork checks appears to be unrelated to this PR. The formatting job is currently running |
| <Wrapper> | ||
| {isObjectOrArray && ( | ||
| <RawButton | ||
| disabled={readonly} |
There was a problem hiding this comment.
@anchmelev we need to preserve the readonly bit. Sometimes we render ArgsTable in a non-editable way.
|
|
||
| - **Base branch**: `next` (all PRs should target `next`, not `main`) | ||
| - **Node.js**: `22.21.1` (see `.nvmrc`) | ||
| - **Node.js**: `22.22.1` (see `.nvmrc`) |
There was a problem hiding this comment.
Do you know why this changed? This shouldn't have happened on that PR.
| @@ -15,12 +13,6 @@ export * from '@storybook/react'; | |||
| export * from './portable-stories.ts'; | |||
| export * from './types.ts'; | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | |||
| // @ts-ignore | |||
| declare module '@storybook/nextjs-vite/vite-plugin' { | |||
| export const storybookNextJsPlugin: typeof vitePluginStorybookNextJs; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
We know we have some failing type checks in the monorepo that are annoying, and that can trip up LLMs. Sometimes, these only fail locally with specific IDEs, etc, but are fine in CI.
Those need to be handled in separate PRs. Sometimes they are false positives; and also, they're different from UI/a11y issues so I wouldn't be the right person to review NextJS changes.
Could you please remove it from this PR? Feel free to make a followup PR if you'd like.
| const vitePluginStorybookNextjs = require('vite-plugin-storybook-nextjs'); | ||
| const vitePluginStorybookNextjs = | ||
| require('vite-plugin-storybook-nextjs') as typeof vitePluginStorybookNextJs; | ||
|
|
||
| export const storybookNextJsPlugin = vitePluginStorybookNextjs; |
There was a problem hiding this comment.
Same here, for a separate PR please 🙏
| {/* We don't let react-aria set an aria-describedby attribute because it clashes with our intention to explicitly set an aria-label that can be different from the tooltip copy. Some screenreaders would announce the label AND description if we also allowed aria-describedby, which would decrease usability. When a component has its own explicit description, we preserve that ID instead of the tooltip's. */} | ||
| <Focusable> | ||
| {/* @ts-expect-error: We have to override aria-describedby and this is the only way we can do it (undefined won't work and an empty string will result in DOM pollution). */} | ||
| {React.cloneElement(child, { 'aria-describedby': overrideAriaDescribedby ?? null })} |
There was a problem hiding this comment.
Thanks for fixing that! Good catch!
| ariaDescription = undefined, | ||
| }) => { | ||
| const { ariaDescriptionAttrs, AriaDescription } = useAriaDescription(ariaDescription); | ||
| const childWithAriaDescription = ariaDescriptionAttrs['aria-describedby'] |
There was a problem hiding this comment.
Aren't you also cloning the element in TooltipProvider? Isn't it redundant to do it twice?
| const tooltipTriggerChild = childWithAriaDescription as ReactElement< | ||
| DOMAttributes<Element>, | ||
| string | ||
| >; |
There was a problem hiding this comment.
This type cast feels problematic to me. We should normally have a single element child so that TooltipProvider Focusable knows it has a single child. If that requirement gets forgotten in the future, a type cast would hide a newly introduced regression.
Maybe a type guard on cloneElement could help ensure childWithAriaDescription is already correctly typed?
3 participants
Part of #24150
What I did
Improved accessibility of the Object control JSON editor in docs controls:
aria-invalidand conditionalaria-describedbyfor parse errors.role="status",aria-live="polite") to announce JSON parse errors.ariaDescriptionto the “Edit as JSON” toggle button.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
cd code && yarn storybook:ui)..../blocks-controls-object--object).{"label":) and blur the textarea.aria-invalid=true) and references the error viaaria-describedby.{"label":"updated"}) and blur again.Documentation
MIGRATION.MD
Summary by CodeRabbit
New Features
Bug Fixes
Tests