Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(multi-action-button, split-button): ensure screen reader commands can navigate menu popup #7074

Merged
merged 2 commits into from
Dec 2, 2024

Conversation

nuria1110
Copy link
Contributor

fix #7054

Proposed behaviour

  • First option in menu popup gains focus when menu is open.
  • Existing keyboard navigation Space, Enter and Arrow Up/Down should still work as expected.
  • Menu should close when no option has focus.
  • Menu popup is no longer in a portal.

Current behaviour

  • Screen reader users using VoiceOver commands cannot navigate into the menu popup.
  • Menu popup in in a portal.

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

Best tested in an example containing several focusable elements e.g.: "Sizes" story.

  • Navigate to toggle button and use VO commands Control+Option+Space to open menu.
  • Use Control+Option+Right to navigate menu options - see VO move onto next button on the page instead of the menu.

DipperTheDan
DipperTheDan previously approved these changes Nov 14, 2024
@edleeks87 edleeks87 self-requested a review November 14, 2024 16:06
@edleeks87 edleeks87 marked this pull request as ready for review November 14, 2024 16:30
@edleeks87 edleeks87 requested review from a team as code owners November 14, 2024 16:30
@@ -679,49 +643,6 @@ test("should hide the additional buttons when a 'Escape' keydown event detected
expect(screen.queryByRole("list")).not.toBeInTheDocument();
});

test("should render with the correct styles when 'align' prop is 'right'", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: are testing this stuff anywhere else? I think PW would be better until we have play functions implemented but unit tests will also be okay in the interim etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed these as they seemed to be pretty much the same as the tests here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also looks like we have PW tests too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice one thanks for checking

);

// check if a child button is focused, if not hide the menu
const checkFocus = useCallback(() => {
const buttonChildren = getButtonChildren();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): You could short-circuit the function or conditionally assign you can avoid checking it's truthyness on line 91

    if (!buttonChildren) return;
or
    const buttonChildren = getButtonChildren() || [] ;

… can navigate menu popup

Fixes issue where screen reader users using VoiceOver could not navigate into the menu buttons using
commands VO+Space to open the menu then VO+right to navigate. The first option will now gain focus
on open using any interaction method, including onClick, keyboard navigation and screen reader
controls.

fix #7054
@harpalsingh
Copy link
Member

I have tested the VO and works as expected now, so will pass a11y review too.

@nuria1110 nuria1110 merged commit 7e3f711 into master Dec 2, 2024
24 checks passed
@nuria1110 nuria1110 deleted the FE-6890 branch December 2, 2024 10:28
@carbonci
Copy link
Collaborator

carbonci commented Dec 2, 2024

🎉 This PR is included in version 144.9.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Multi Action Button still not always working for screenreader despite being tabbable - serious
6 participants