UI: Improve interactions panel accessibility#34110
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:
📝 WalkthroughWalkthroughReworks Interaction and InteractionsPanel markup/ARIA and label/status logic: semantic list markup, new live-region announcements, updated toggle icons/aria-expanded, computed interaction labels/status, disabled navigation handling, new tests, and an updated e2e selector. Changes
Sequence Diagram(s)(Skipped — changes are accessibility/markup and localized UI logic updates that do not introduce a new multi-component sequential flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/component-testing/components/InteractionsPanel.test.tsx (1)
98-122: Consider adding test coverage for remaining status values.The test covers 'playing' and 'errored' statuses but the
StatusAnnouncementMappingalso includes 'rendering', 'completed' (without errors), and 'aborted'. Consider adding assertions for these states to ensure comprehensive coverage of the live region announcements.Example additional assertions
// Test 'aborted' status announcement rerender( <ThemeProvider theme={convert(themes.light)}> <InteractionsPanel {...createProps({ status: 'aborted', interactions: getInteractions(CallStates.DONE), })} /> </ThemeProvider> ); expect(screen.getByRole('status')).toHaveTextContent('Component test was aborted.');🤖 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 98 - 122, Add assertions for the remaining statuses defined in StatusAnnouncementMapping so the live-region announcements are fully covered: after the initial 'playing' and 'errored' assertions, call rerender with InteractionsPanel (using createProps) for 'rendering', 'completed' (successful), and 'aborted' statuses using appropriate getInteractions(CallStates.*) values, then assert the role="status" or role="alert" contains the expected messages for each status; update the test in InteractionsPanel.test.tsx near the existing rerender blocks so assertions for 'rendering', 'completed', and 'aborted' are included and use the ThemeProvider/convert(themes.light) wrapper as done currently.
🤖 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/InteractionsPanel.test.tsx`:
- Around line 98-122: Add assertions for the remaining statuses defined in
StatusAnnouncementMapping so the live-region announcements are fully covered:
after the initial 'playing' and 'errored' assertions, call rerender with
InteractionsPanel (using createProps) for 'rendering', 'completed' (successful),
and 'aborted' statuses using appropriate getInteractions(CallStates.*) values,
then assert the role="status" or role="alert" contains the expected messages for
each status; update the test in InteractionsPanel.test.tsx near the existing
rerender blocks so assertions for 'rendering', 'completed', and 'aborted' are
included and use the ThemeProvider/convert(themes.light) wrapper as done
currently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4782b256-22fd-457a-8a55-016b4fdf2478
📒 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
|
Hi @Sidnioulz, gentle follow-up on this PR since it closes #31701. |
Hi there! Thank you for the PR. We're gearing up for 10.3 right now, so there is a feature freeze in effect. It might take a bit longer than usual to review but it's now on my todo list :) |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
Sidnioulz
left a comment
There was a problem hiding this comment.
This is a fantastic improvement overall! Kudos! 🎉
There's a bunch of PR feedback for you to look at, and I'll get in touch with colleagues to check the parts I'm not sure about.
Thanks a lot! I’ve now addressed the review feedback on my side and gave everything another pass as well. |
|
FYI the CI errors appear unrelated to your code. We're gonna fix that upstream and then rebase your branch. I'm also waiting to hear from Michael for the copy check. |
…for styled component
…er stepStatusTextMap for clarity
… for undefined status
fae5d01 to
e1b9dab
Compare
Sidnioulz
left a comment
There was a problem hiding this comment.
Excellent work @anchmelev! Thank you for your work and your responsiveness.
Closes #31701
What I did
Improved accessibility in the Interactions panel by making the steps list semantic and improving screen reader support.
This PR:
olandli)aria-expandedaria-busywhile interactions are rendering or runningReact.useId()so the component remains compatible with React 17Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
cd code && yarn task compile && yarn storybook:uihttp://localhost:6006/?path=/story/core-component-test-basics--steparia-expandedDocumentation
MIGRATION.MD
Summary by CodeRabbit
New Features
Improvements
Tests