fix(addon-docs): improve object control JSON editor accessibility#34108
Open
anchmelev wants to merge 2 commits intostorybookjs:nextfrom
Open
fix(addon-docs): improve object control JSON editor accessibility#34108anchmelev wants to merge 2 commits intostorybookjs:nextfrom
anchmelev wants to merge 2 commits intostorybookjs:nextfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds accessibility and error UI for the ObjectControl raw JSON editor, includes tests for ARIA behavior and error handling, and updates interaction/interaction-panel components to improve accessible labeling, status announcements, and navigation state handling. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
Contributor
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
Contributor
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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