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

Add focus-trap and tab-key keyboard navigation to ModalDialog #931

Merged
merged 4 commits into from
Mar 23, 2023

Conversation

lyzadanger
Copy link
Contributor

@lyzadanger lyzadanger commented Mar 22, 2023

This PR adds a new hook, useTabKeyNavigation, modeled after useArrowKeyNavigation, and enables it by default for ModalDialog.

This establishes trap-focus behavior and tab/shift-tab keyboard navigation for ModalDialogs, which we need for accessibility.

Note that useTabKeyNavigation duplicates behavior from useArrowKeyNavigation, per voice discussion earlier today. The life expectancy of useTabKeyNavigation is short-ish, so this time-saving measure is perhaps justifiable.

This PR also contains a commit that removes a no-longer-needed testing workaround for useArrowKeyNavigation's tests.

Part of #77

@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #931 (cf629d2) into main (a798382) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #931   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           64        65    +1     
  Lines          807       876   +69     
  Branches       289       313   +24     
=========================================
+ Hits           807       876   +69     
Impacted Files Coverage Δ
src/components/feedback/ModalDialog.tsx 100.00% <100.00%> (ø)
src/hooks/use-tab-key-navigation.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of these tests, or at least their structure, were copied from the tests for useArrowKeyNavigation. I'll call out where they differ.

Comment on lines +52 to +57
/**
* @param {string|Partial<KeyboardEvent>} key - Either a string name of a
* key (e.g. 'ArrowLeft') or a partial `KeyboardEvent` object, e.g.
* `{ key: 'Tab', shiftKey: true }`
*/
function pressKey(key) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I adjusted the parameters for this utility function so it can imitate more types of KeyboardEvents.

Comment on lines +167 to +176
it('should not respond to keys if hook is not enabled', () => {
renderToolbar({
enabled: false,
});
['Tab', { key: 'Tab', shiftKey: true }].forEach(key => {
findElementByTestId('bold').focus();
pressKey(key);
assert.equal(currentItem(), 'Bold');
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hook can be "enabled" or "disabled", which is a feature that useArrowKeyNavigation does not have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This implementation is very close to useArrowKeyNavigation but slightly simpler. It does add an enabled option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've completed aWith these changes, most of the behavioral features of these components are in place so I'm removing the TODO list.

@lyzadanger lyzadanger requested a review from acelaya March 22, 2023 23:37
/**
* Don't respond to any keyboard events if not enabled. This allows selective
* enabling by components using this hook, as hook use itself cannot be
* conditional.
Copy link
Contributor

Choose a reason for hiding this comment

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

as hook use itself cannot be conditional.

Which is sad, because it would be useful sometimes 😅

@lyzadanger lyzadanger merged commit edcb2dc into main Mar 23, 2023
@lyzadanger lyzadanger deleted the use-tab-key-navigation branch March 23, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants