fix(multi-action-button): add new keyboard functionality to component FE-5188 #5235

edleeks87 · 2022-06-16T11:14:41Z

Proposed behaviour

Updates the keyboard navigation behaviour for MultiActionButton and SplitButton. Up and down arrow keys can still be used for navigating children buttons but there is no longer any looping back around. When the children container is open tabbing can now be used to navigate the children buttons, the tab key takes you down the list and back tab goes up. When the last child button is focused and tab is pressed it will close the container and move focus to the next focusable element on the page. When the shift and tab keys are pressed and the first child button is focused it will close the container and return focus back to the main button control. Pressing the escape key when the children container is open will now close the list and return focus back to the main button control. All other keyboard functionality remains the same

Current behaviour

Up and down arrow keys navigate children buttons and loop back around. Tabbing moves focus away from the children buttons to the next element on the page, back tabbing moves focus back to main button control

Checklist

Commits follow our style guide
Related issues linked in commit messages if required
Unit tests added or updated if required

QA

Tested in CodeSandbox/storybook
Add new Cypress test coverage if required
Carbon implementation matches Design System/designs
UI Tests GitHub check reviewed if required

Additional context

Testing instructions

Can be tested in storybook

The following CodeSandbox is an example of the broken behaviour.
You can see the new behaviour by looking at the version in the comment by codesandbox[bot].

codesandbox-ci · 2022-06-16T11:16:14Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5121f6a:

Sandbox	Source
carbon-quickstart	Configuration
carbon-quickstart-typescript	Configuration
carbon-quickstart	PR
serene-spence-7dv4bf	Issue #4522

cypress · 2022-06-16T11:24:40Z

Test summary

2446 • 0 • 2 • 0 • 0

Run details


Project	carbon
Status	Passed
Commit	`5121f6a`
Started	Jul 19, 2022 12:23 PM
Ended	Jul 19, 2022 12:29 PM
Duration	06:08 💡
OS	Linux Debian - 10.10
Browser	Chrome 100

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

robinzigmond

A couple of general points:

Having done a quick test, I think there's a few requirements given on FE-5188 that this PR doesn't meet:

when focusing on the parent button and pressing the up arrow, the menu doesn't open
holding command while pressing up or down arrow only moves up/down once, when according to the requirements it should move focus all the way to the first/last menu item. (Control+up/down should do the same on Windows, as should the Home/End buttons - might need some investigation into how presses on those buttons are sent to the browser.)

Also, as a general point: there's so much code here replicated between Multi Action Button and Split Button, can't we DRY it up a bit, at least with some utils perhaps to handle the keyboard functionality? I recognise it was like this before, but I'd have been very tempted to do this if I was rewriting it anyway as you are. Will leave it to your better judgement!

Needless to say, most/all of my many comments on the MultiActionButton changes also apply to SplitButton

robinzigmond · 2022-06-22T08:09:04Z

src/components/multi-action-button/multi-action-button.component.tsx

+            buttonRef.current as HTMLButtonElement
+          );
+
+          elements[indexOf]?.focus();


I realise this wasn't your code and existed before, but is all this really necessary? We're finding the index of buttonRef.current in the elements array, then grabbing the element at that index, and focusing it. In other words, we're focusing the main button which should be accessible at buttonRef.current.

At the risk of sounding stupid, why can't we do this with just buttonRef.current?.focus ?? (I'm not even sure the ?. is needed there but it does no harm and prevents crashes in potential weird situations.)

It seems that according to the new requirements it should be elements[indexOf + 1]?.focus();

src/components/multi-action-button/multi-action-button.component.tsx

src/components/multi-action-button/multi-action-button.spec.tsx

src/components/multi-action-button/multi-action-button.component.tsx

src/components/multi-action-button/multi-action-button.test.js

…lity into custom hook

Implements `useMenuKeyboardNavigation` to Update the keyboard navigation behaviour for `MultiActionButton`. Up and down arrow keys can still be used for navigating children buttons but there is no longer any looping back around. When the children container is open tabbing can now be used to navigate the children buttons, the tab key takes you down the list and back tab goes up. When the last child button is focused and tab is pressed it will close the container and move focus to the next focusable element on the page. When the shift and tab keys are pressed and the first child button is focused it will close the container and return focus back to the main button control. Pressing the escape key when the children container is open will now close the list and return focus back to the main button control. All other keyboard functionality remains the same fix #4522

Implements `useMenuKeyboardNavigation` to update the keyboard navigation behaviour for `SplitButton`. Up and down arrow keys can still be used for navigating children buttons but there is no longer any looping back around. When the children container is open tabbing can now be used to navigate the children buttons, the tab key takes you down the list and back tab goes up. When the last child button is focused and tab is pressed it will close the container and move focus to the next focusable element on the page. When the shift and tab keys are pressed and the first child button is focused it will close the container and return focus back to the main button control. Pressing the escape key when the children container is open will now close the list and return focus back to the main button control. All other keyboard functionality remains the same fix #4522

carbonci · 2022-07-19T12:34:59Z

🎉 This PR is included in version 109.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

edleeks87 added Pending Review Pending QA Pending UX QA Squad Front End labels Jun 16, 2022

edleeks87 self-assigned this Jun 16, 2022

edleeks87 force-pushed the FE-5188-esc-key-fix-for-multi-action-and-split-button branch from 649c4d6 to 5e642c6 Compare June 16, 2022 13:10

robinzigmond self-requested a review June 21, 2022 15:14

robinzigmond reviewed Jun 22, 2022

View reviewed changes

mkrds self-requested a review June 23, 2022 11:42

mkrds reviewed Jun 23, 2022

View reviewed changes

src/components/multi-action-button/multi-action-button.component.tsx Outdated Show resolved Hide resolved

edleeks87 force-pushed the FE-5188-esc-key-fix-for-multi-action-and-split-button branch 2 times, most recently from 9602181 to 5ebe6bc Compare June 27, 2022 15:46

robinzigmond previously approved these changes Jun 28, 2022

View reviewed changes

edleeks87 dismissed robinzigmond’s stale review via 9d78627 June 28, 2022 09:52

edleeks87 force-pushed the FE-5188-esc-key-fix-for-multi-action-and-split-button branch 2 times, most recently from 9d78627 to 3ad69f1 Compare June 28, 2022 10:20

edleeks87 commented Jun 28, 2022

View reviewed changes

src/components/multi-action-button/multi-action-button.test.js Outdated Show resolved Hide resolved

edleeks87 force-pushed the FE-5188-esc-key-fix-for-multi-action-and-split-button branch 3 times, most recently from cf2e54b to 0c829f8 Compare June 28, 2022 15:06

mkrds previously approved these changes Jun 29, 2022

View reviewed changes

robinzigmond previously approved these changes Jun 29, 2022

View reviewed changes

edleeks87 marked this pull request as ready for review June 29, 2022 08:10

edleeks87 requested review from a team as code owners June 29, 2022 08:10

edleeks87 added Passed Review and removed Pending Review labels Jun 29, 2022

harpalsingh added Passed UX QA and removed Pending UX QA labels Jul 1, 2022

edleeks87 dismissed stale reviews from robinzigmond and mkrds via 3e487aa July 18, 2022 10:34

edleeks87 force-pushed the FE-5188-esc-key-fix-for-multi-action-and-split-button branch 3 times, most recently from ef0885c to 6b1dfc8 Compare July 18, 2022 11:07

robinzigmond previously approved these changes Jul 18, 2022

View reviewed changes

mkrds previously approved these changes Jul 18, 2022

View reviewed changes

DlgSHi previously approved these changes Jul 19, 2022

View reviewed changes

DlgSHi added Passed QA and removed Pending QA labels Jul 19, 2022

edleeks87 and others added 3 commits July 19, 2022 13:10

refactor(use-menu-keyboard-navigation): move shared keydown functiona…

cb19ca4

…lity into custom hook

edleeks87 dismissed stale reviews from DlgSHi, mkrds, and robinzigmond via 5121f6a July 19, 2022 12:18

edleeks87 force-pushed the FE-5188-esc-key-fix-for-multi-action-and-split-button branch from 6b1dfc8 to 5121f6a Compare July 19, 2022 12:18

DlgSHi approved these changes Jul 19, 2022

View reviewed changes

mkrds approved these changes Jul 19, 2022

View reviewed changes

edleeks87 merged commit e3996a4 into master Jul 19, 2022

edleeks87 deleted the FE-5188-esc-key-fix-for-multi-action-and-split-button branch July 19, 2022 12:31

carbonci added the released label Jul 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(multi-action-button): add new keyboard functionality to component FE-5188 #5235

fix(multi-action-button): add new keyboard functionality to component FE-5188 #5235

edleeks87 commented Jun 16, 2022 •

edited

Loading

codesandbox-ci bot commented Jun 16, 2022 •

edited

Loading

cypress bot commented Jun 16, 2022 •

edited

Loading

robinzigmond left a comment

robinzigmond Jun 22, 2022

mkrds Jun 23, 2022

carbonci commented Jul 19, 2022

fix(multi-action-button): add new keyboard functionality to component FE-5188 #5235

fix(multi-action-button): add new keyboard functionality to component FE-5188 #5235

Conversation

edleeks87 commented Jun 16, 2022 • edited Loading

Proposed behaviour

Current behaviour

Checklist

QA

Additional context

Testing instructions

codesandbox-ci bot commented Jun 16, 2022 • edited Loading

cypress bot commented Jun 16, 2022 • edited Loading

Test summary

Run details

robinzigmond left a comment

Choose a reason for hiding this comment

robinzigmond Jun 22, 2022

Choose a reason for hiding this comment

mkrds Jun 23, 2022

Choose a reason for hiding this comment

carbonci commented Jul 19, 2022

edleeks87 commented Jun 16, 2022 •

edited

Loading

codesandbox-ci bot commented Jun 16, 2022 •

edited

Loading

cypress bot commented Jun 16, 2022 •

edited

Loading