Keyboard navigation in mobile web (Issue #2159) #2166

LMulvey · 2022-10-11T16:21:07Z

Description

Few a11y fixes related to the Header component

Fixed bug where toggle was being called every time onKeyDown was triggered on the hamburger button

Introduced focus trapping for the mobile navigation bar while open - you can no longer focus out of the nav via the keyboard without closing it

Upgraded @testing-library/jest-dom, @testing-library/react and @testing-library/user-event - the dependency update was largely to use the updated userEvent.keyboard event support from @testing-library/user-event but I wanted to ensure the dependencies were in sync.

Corresponding Issue

#2159

Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!

…ng-library/user-event to allow better a11y testing

LMulvey · 2022-10-11T17:30:40Z

Obviously broke some things with the test upgrades. I'm working through the changes - there were a few API updates with @testing-library over some versions.

@julianguyen - happy to update all existing tests to still function. The benefit here is that we get a more realistic testing environment from user-event (See: Differences from fireEvent) but I definitely don't want to disrupt things too much so let me know if you'd like me to step back here and I can update the Header.spec tests I've added to try and get them to work with the older APIs.

julianguyen · 2022-10-11T17:52:31Z

@LMulvey Thanks for the summary and for taking this on 🎉 ! Yeah, let's update the existing tests like you said.

LMulvey · 2022-10-11T19:14:24Z

Okay! Tests are updated but there are a few caveats.

I've had to skip three tests for now - I need to think about an approach a bit. I left a comment on all three tests describing why they were skipped. Still, in summary, @testing-library/user-event v14 dropped support for event.keyCode (See: testing-library/user-event#842).

react-autosuggest is pretty outdated and still exclusively looks for event.keyCode when detecting whether or not a user hit the Enter key. Because of this, our Enter inputs from tests are not working with any components that use react-autosuggest.

I've tried manually dispatching an event with JSDom (you can see my scraps in the InputTag.spec tests) but it wasn't working. I can try to submit a PR to react-autosuggest but it seems to be unmaintained at this point.

Does anyone have any ideas? 🤔

cc: @julianguyen

julianguyen

Thanks so much for taking this on! Thanks for also clarifying your approach and debugging! 🎉 Hope you're well :)

julianguyen · 2022-10-12T03:52:40Z

client/app/components/Header/__tests__/Header.spec.jsx

+    await user.keyboard('[Space]');
+    expect(container.querySelector('#headerMobile')).toBeInTheDocument();
+    await user.keyboard('[Space]');
+    expect(container.querySelector('#headerMobile')).not.toBeInTheDocument();


Could you explain what exactly we're testing when we hit the Space key twice?

Ensuring that it opens AND closes the navbar with the space key!

julianguyen · 2022-10-12T03:53:28Z

client/app/components/Header/__tests__/Header.spec.jsx

+    await user.keyboard('{Tab}');
+    expect(screen.getByRole('link', { name: /link 1/i })).toHaveFocus();
+
+    // Shift-tab back to the hamburger and close the mobile menu


julianguyen · 2022-10-12T03:54:49Z

client/app/components/Header/index.jsx

+
+        if (event.key === 'Tab') {
+          if (event.shiftKey) {
+            if (document.activeElement === firstFocusableElement) {


I think the if-locks on lines 42 and 43 can be combined with an && to reduce nesting.

client/app/components/Header/index.jsx

julianguyen · 2022-10-12T03:59:54Z

client/app/components/Input/__tests__/InputTag.spec.jsx

@@ -9,6 +9,13 @@ const getComponent = (extraProps = {}) => createInput(inputTagProps, extraProps)
 const component = getComponent();
 const { checkboxes } = inputTagProps;

+function setup(jsx) {


Would it be useful to create some kind of utility function so that it doesn't have to be re-created in multiple tests files?

julianguyen · 2022-10-12T04:01:40Z

client/app/components/Input/__tests__/InputTextarea.spec.jsx

-        userEvent.click(button);
-        expect(pell.exec).toHaveBeenCalledWith(...expectedArgs);
-      });
+      await Promise.all(


Can we use waitFor instead?

What's the rationale for waitFor vs Promise.all? I'm not sure I follow! My larger gut feeling is to break out of doing loops here entirely and write out all the Button conditions explicitly – just to avoid any hack-y-ness 🤔

Could you do something like this instead with waitFor?

buttons.forEach(({ title, expectedArgs }) => { const button = screen.getByTitle(title); userEvent.click(button); await waitFor(() => expect(pell.exec).toHaveBeenCalledWith(...expectedArgs)); });

Wahoo! Worked like a charm 🎉

julianguyen · 2022-10-12T04:01:57Z

client/app/components/Input/__tests__/InputTextareaTemplate.spec.jsx

-        userEvent.click(button);
-        expect(pell.exec).toHaveBeenCalledWith(...expectedArgs);
-      });
+      await Promise.all(


Similar question as above.

julianguyen · 2022-10-12T04:03:35Z

client/package.json

@@ -144,5 +144,6 @@
  },
  "browserslist": [
    "defaults"
-  ]
+  ],
+  "packageManager": "yarn@1.22.19"


What's the rationale for this? Is it necessary?

🤦🏻 Not at all. My setup uses Yarn v3 and to avoid changing the lockfile formatting, I ran yarn set-version classic - which introduced a bunch of random changes throughout the repo - I thought I had caught them all. Turns out it's not super intuitive to run a separate version of Yarn in a specific workspace. I'll uncommit this and watch for it again!

julianguyen · 2022-10-12T04:34:21Z

client/app/components/Header/index.jsx

+        const firstFocusableElement = focusableElements[0];
+        const lastFocusableElement = focusableElements[focusableElements.length - 1];
+
+        if (event.key === 'Tab') {


When I do the following, the focus trap does not work:

Tab into the hamburger icon

Hit Enter to open the mobile menu

Tab through the menu items

When the last item of the menu is reached, and I hit Tab again, the focus trap does not work and the contents behind the menu are hit.

The expected behaviour is for the focus to go back to logo at the top of the menu.

I think the problem is that document.activeElement is not giving you the active element that is being focused on. It's giving the element before it in the DOM. I think this is happening because it needs to be in a useEffect().

I think we want to make this focus trap code we're using in the Modal component a custom hook that we can reuse here.

Another issue I see is that lastFocusableElement is the "Sign out" button when it should be the "Resources" link.

@julianguyen Very interesting! I was testing it in an unauthenticated state on the homepage–I feel like this is a separate state. I'll take a look (and look at lifting the Modal component useEffect into a reusable hook! 💪🏻 )

This is fixed now! It was related to a display: none element in the Links - it isn't technically focusable by the browser but still appeared in our focusableElements array. I've added an extra step to filter out non-visible elements here – the Focus Trap now works as expected while authenticated or unauthenticated.

julianguyen · 2022-10-12T04:55:33Z

client/app/components/Input/__tests__/InputTag.spec.jsx


    checkbox = screen.getByRole('checkbox', queryOptions);
    expect(checkbox).toBeInTheDocument();
  });

-  it('selects a value with keydown without specifying text', () => {
+  /**
+   * Skipping this one for now. react-autosuggest still checks for event.keyCode, which


Have you tried using fireEvent instead in these skipped tests? I know it's not ideal since we want to simulate a complete interaction via userEvent.

I did try that - turns out it also writes the keyCode is 0 so we end up with the same issue 😭

Thanks for trying all this! Hmm, I'm okay with skipping this test for now, but we should have a fast follow to fix this. Since react-autosuggest is deprecated, I think we'll need to migrate to another library. Could you create an issue for this? Are you able to take this on?

I think we can just keep the original contents of this test since it's being skipped anyways.

Co-authored-by: Julia Nguyen <julianguyen@users.noreply.github.com>

…us trapping bug

…none from focusable elements

julianguyen

Awesome work! 🎉 Thanks for the updates. Tested things locally, and the keyboard navigation works as expected.

Shared some feedback around moving forward with react-autosuggest and also the tests

julianguyen · 2022-10-12T23:45:24Z

client/app/components/Input/__tests__/InputTag.spec.jsx


    checkbox = screen.getByRole('checkbox', queryOptions);
    expect(checkbox).toBeInTheDocument();
  });

-  it('selects a value with keydown without specifying text', () => {
+  /**
+   * Skipping this one for now. react-autosuggest still checks for event.keyCode, which


Thanks for trying all this! Hmm, I'm okay with skipping this test for now, but we should have a fast follow to fix this. Since react-autosuggest is deprecated, I think we'll need to migrate to another library. Could you create an issue for this? Are you able to take this on?

I think we can just keep the original contents of this test since it's being skipped anyways.

julianguyen · 2022-10-12T23:48:20Z

client/app/components/Input/__tests__/InputSwitch.spec.jsx

@@ -35,7 +35,7 @@ describe('InputSwitch', () => {
      expect(screen.getByRole('checkbox')).not.toBeChecked();

      /**
-       * TODO: Follow up on `userEvent.type(inputSwitch, '{enter}')` in v12.1.7.
+       * TODO: Follow up on `await userEvent.type(inputSwitch, '{enter}')` in v12.1.7.


Let's also make a GitHub issue for this, so it doesn't get lost in comments.

This is an old comment that I was able to just resolve & remove as part of this upgrade 🎉 !

julianguyen · 2022-10-12T23:54:41Z

client/app/components/Input/__tests__/InputTextarea.spec.jsx

-        userEvent.click(button);
-        expect(pell.exec).toHaveBeenCalledWith(...expectedArgs);
-      });
+      await Promise.all(


Could you do something like this instead with waitFor?

buttons.forEach(({ title, expectedArgs }) => { const button = screen.getByTitle(title); userEvent.click(button); await waitFor(() => expect(pell.exec).toHaveBeenCalledWith(...expectedArgs)); });

julianguyen · 2022-10-12T23:55:15Z

client/app/components/Modal/index.jsx

-  useEffect(() => {
-    const focusableElements = 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])';
-    const modal = modalEl.current;
+  useFocusTrap(modalEl, open);


julianguyen · 2022-10-12T23:56:08Z

client/app/widgets/QuickCreate/__tests__/QuickCreate.spec.jsx

@@ -135,7 +147,19 @@ describe('QuickCreate', () => {
  });

  describe('when the form', () => {
-    it('is submitted it adds a new checkbox from data', async () => {
+    /**


Similar comment around keeping original test like stated above!

…ach + waitFor

…fireEvent

LMulvey · 2022-10-13T05:39:52Z

@julianguyen Thank you for all the feedback! I've pushed up another round of fixes and created #2170 to cover off the react-autosuggest changeover task. I should be able to take that on next week once this PR is wrapped up 🤝

julianguyen

Awesome work! 🎉 Thanks so much for taking this on and navigating all the gotchas in a good way.

LMulvey added 3 commits October 10, 2022 21:20

feat: update testing-library/jest-dom + testing-library/react + testi…

a58318e

…ng-library/user-event to allow better a11y testing

feat(Header): improve keyboard accessibility with Header

5467b7b

feat(Header): Trap focus on mobile navbar while it is open

213746c

LMulvey added the hacktoberfest label Oct 11, 2022

LMulvey requested a review from julianguyen October 11, 2022 16:21

test: update tests to use new @testing-library/user-event API

52bb84b

chore: properly lint code and update to please the linter

4c11f3e

julianguyen reviewed Oct 12, 2022

View reviewed changes

LMulvey and others added 3 commits October 12, 2022 10:53

fix: change desktop displayLinks to use short-circuit evaluation

f8b591b

Co-authored-by: Julia Nguyen <julianguyen@users.noreply.github.com>

feat: lift up useFocusTrap hook for Modal and Header + fix Header foc…

b148a74

…us trapping bug

fix(useFocusTrap): update filtering strategy to only filter display: …

3be61b1

…none from focusable elements

julianguyen requested changes Oct 12, 2022

View reviewed changes

LMulvey added 2 commits October 12, 2022 23:22

test(InputTextarea*): remove use of Promise.all and replace with forE…

699af39

…ach + waitFor

test(InputSwitch): update test and resolve Todo regarding use of RTL …

15dc31d

…fireEvent

LMulvey mentioned this pull request Oct 13, 2022

Implement react-autosuggest alternative #2170

Open

julianguyen approved these changes Oct 13, 2022

View reviewed changes

julianguyen merged commit 72f396e into ifmeorg:main Oct 13, 2022

julianguyen added the hacktoberfest-accepted label Oct 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyboard navigation in mobile web (Issue #2159) #2166

Keyboard navigation in mobile web (Issue #2159) #2166

LMulvey commented Oct 11, 2022

LMulvey commented Oct 11, 2022

julianguyen commented Oct 11, 2022

LMulvey commented Oct 11, 2022 •

edited

Loading

julianguyen left a comment

julianguyen Oct 12, 2022

LMulvey Oct 12, 2022

julianguyen Oct 12, 2022

julianguyen Oct 12, 2022

julianguyen Oct 12, 2022

julianguyen Oct 12, 2022

LMulvey Oct 12, 2022

julianguyen Oct 12, 2022

LMulvey Oct 13, 2022

julianguyen Oct 12, 2022

julianguyen Oct 12, 2022

LMulvey Oct 12, 2022

julianguyen Oct 12, 2022

julianguyen Oct 12, 2022 •

edited

Loading

julianguyen Oct 12, 2022

LMulvey Oct 12, 2022

LMulvey Oct 12, 2022

julianguyen Oct 12, 2022

LMulvey Oct 12, 2022

julianguyen Oct 12, 2022

julianguyen left a comment

julianguyen Oct 12, 2022

julianguyen Oct 12, 2022

LMulvey Oct 13, 2022

julianguyen Oct 12, 2022

julianguyen Oct 12, 2022

julianguyen Oct 12, 2022

LMulvey commented Oct 13, 2022

julianguyen left a comment

Keyboard navigation in mobile web (Issue #2159) #2166

Keyboard navigation in mobile web (Issue #2159) #2166

Conversation

LMulvey commented Oct 11, 2022

Description

Corresponding Issue

LMulvey commented Oct 11, 2022

julianguyen commented Oct 11, 2022

LMulvey commented Oct 11, 2022 • edited Loading

julianguyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianguyen Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianguyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LMulvey commented Oct 13, 2022

julianguyen left a comment

Choose a reason for hiding this comment

LMulvey commented Oct 11, 2022 •

edited

Loading

julianguyen Oct 12, 2022 •

edited

Loading