Improve navigation accessibility #2707

dcalhoun · 2021-03-02T18:18:10Z

Fix

Fixes #979 and fixes #980. Improve navigation accessibility via the following:

Add focus trap to navigation sidebar and revisions selector.
Hide unrelated content while navigation sidebar or revisions selector is open.
Reorder navigation elements to match visual presentation.
Adjust styles to improve active focus indication.

VoiceOver Interaction: Navigation

voiceover-interaction.mov

VoiceOver Interaction: Revisions Selector

Screen.Recording.2021-03-03.at.08.27.23.mov

Styles Before	Styles After

Test

Scenario 1

Enable macOS VoiceOver or similar screen reader technology.
Open the navigation sidebar by pressing the menu button in the top-left
corner of the screen.

Expected Outcome:

The first element in the navigation has focus.
Moving selection backwards does not move focus past "All Items" menu item.
Moving selection forwards does not move focus past "About" menu item.
Pressing Esc dismisses the navigation, deactivates the focus
trap, and returns focus to the "Menu" button.
Clicking outside of the navigation dismisses the navigation, deactivates the focus
trap, and returns focus to the "Menu" button.

Scenario 2

Enable macOS VoiceOver or similar screen reader technology.
Open the navigation sidebar by pressing the menu button in the top-left
corner of the screen.
Open "About" dialog by pressing the "About" link at the end of the navigation
list.

Expected Outcome:

Navigation focus trap is disabled while dialog is open.
Dialog contents are focused.
Dismissing the dialog with Esc causes "About" link to regain focus.

Scenario 3

Enable macOS VoiceOver or similar screen reader technology.
Open the revisions selector by pressing the history button in the top-right
corner of the screen.

Expected Outcome:

The first element in the navigation has focus.
Moving selection backwards does not move focus past "History" heading.
Moving selection forwards does not move focus past "Restore" button.
Pressing Esc dismisses the navigation, deactivates the focus
trap, and returns focus to the note editor.
Clicking outside of the navigation dismisses the navigation, deactivates the focus
trap, and returns focus to the note editor.

Release

RELEASE-NOTES.md was updated with:

Improve navigation sidebar and revision selector accessibility for keyboards and screen readers.

dmsnell

can you share a quick synopsis of the difference that focus-trap-react brings from react-onclickoutside?

also, should we be removing the react-onclickoutside dependency? if it's still in use in other modules, should they also be updated?

dcalhoun · 2021-03-02T20:01:57Z

@dmsnell absolutely. This is still a bit of a work in progress, but I'll be sure to include information on the packages utilized.

dcalhoun · 2021-03-03T15:13:27Z

can you share a quick synopsis of the difference that focus-trap-react brings from react-onclickoutside?

also, should we be removing the react-onclickoutside dependency? if it's still in use in other modules, should they also be updated?

@dmsnell I went ahead and refactored the Revisions Selector to improve its accessibility and remove its usage of react-onclickoutside. The remaining location using react-onclickoutside is the notes Info sidebar, but @sandymcfadden mentioned that area is actively being refactored to a dialog, so I left it as-is. Once that Info sidebar is a dialog, react-onclickoutside could be removed from the project.

Package	Gzipped Size	Click Outside	Focus Trap	Escape Dismiss
`react-onclickoutside`	1.6kb	✅	❌	❌
`focus-trap-react`	4.2kb	✅	✅	✅
`react-focus-lock`	5.4kb	❌	✅	❌
`react-focus-on`	8.7kb	✅	✅	✅

As represented above, I chose to leverage focus-trap-react because it provides functionality to "trap" focus within a portion of the UI, which improves keyboard and screen reader navigation. I opted against react-focus-lock due to its larger size¹. Please let me know if believe an alternative approach may be better. Thanks!

react-focus-lock technically offers better tree shaking and supports lazy loading some of its capabilities. However, I decided introducing that complexity was not worth it at this time.

Edit: Updated chart to correct misrepresentation react-focus-lock. It does not include escape/click outside to dismiss. We must either build that ourselves or include react-focus-on instead, which includes react-focus-lock.

dcalhoun · 2021-03-03T15:15:55Z

We should also consider tablist / tabpanel semantics for the NavigationBar so it makes sense in screen readers.

One additional note is that I did not address the above from #979. I believe that will likely require a larger investigation and discussion regarding to how to best organized the navigation items and their relation to the note list and note editor. I.e. how to associate navigation items with specific states for the notes list and note editor. I plan to create a separate issue for exploring this piece.

dmsnell · 2021-03-03T19:51:14Z

@dcalhoun we're not talking about a big difference in code size between those libraries beyond react-onclickoutside. Looks like you noticed that react-focus-lock may end up compiling smaller if it's architecture is better suited to tree-shaking.

What are your thoughts on the quality and maintainership of those libraries? Is one clearly better for accessibility? I don't have experience with either.

dcalhoun · 2021-03-03T22:50:39Z

What are your thoughts on the quality and maintainership of those libraries? Is one clearly better for accessibility? I don't have experience with either.

@dmsnell great questions!

Package	Weekly Downloads	Maintenance	Quality
`focus-trap-react`	226k	Maintainers changed in Aug 2020.	Source quality appears good. Has test coverage. Provides accessibility features we need. Not as feature rich as competing packages, which means source is a bit more straightforward.
`react-focus-lock`	1,646k	Author appears active in package and community, particularly on this topic.	Source quality appears very good. Has test coverage. Provides accessibility features we need. It covers more functionality and thus the source is a bit more complex (e.g. offers `react-focus-on` to compose all features together).

I think the two packages are fairly comparable in terms of quality. However, it is very evident that react-focus-lock is significantly more popular in terms of downloads. I believe I opted for focus-trap-react because (1) I was familiar with it and (2) the smaller size without the need to side load portions of its source.

That said, after restating these thoughts out for consideration, it does seem like react-focus-lock may be a better longterm choice. What do you think?

Relatedly, one day React may includes its own focus management. However, I do not believe that is likely to occur soon.

dmsnell · 2021-03-04T02:23:38Z

thanks for providing such helpful info @dcalhoun

What do you think?

sounds like both are probably fine given what you said. you can pick the direction and set it 😉 I feel like I understand the context much better now though, and hopefully soon we'll be able to get rid of the react-outsideclick as well.

this is great work you've been providing. I'm sure it's helping out in some of the worst usability areas with our app

dcalhoun · 2021-03-04T22:02:09Z

After considering the available options again, I have chosen to continue using focus-trap-react.

focus-trap-react (~4.2kb) includes click outside and escape to close features. react-focus-lock (~5.4kb) does not which means we would need to either (A) build that logic ourselves or (B) use react-focus-on (~8.7kb) instead, which includes features we do not need.
In order to reduce the initial load size of react-focus-on we would need to leverage sidecar to lazy load dependencies. From my testing the resulting variation in size bundle between focus-trap-react and lazy loaded react-focus-on is negligible.
From my perspective, using focus-trap-react is more straightforward and provides the accessibility improvements we set out to accomplish.

Plan to move forward with focus-trap-react unless I receive guidance otherwise. 👍🏻

belcherj · 2021-03-10T15:18:04Z

lib/app-layout/index.tsx

              {showRevisions ? (
-                <NotePreview noteId={openedNote} note={openedRevision} />
+                <NotePreview
+                  aria-hidden={hiddenByRevisions}


why use aria-hidden as opposed to ariaHidden?

Largely for consistency between DOM elements and React elements (i.e. use kebab-case in both instances).

That said, I don't have a strong opinion. Happy to change the key for React elements to be camelcase. Would you like me to do that?

@codebykat @sandymcfadden Any thoughts? I don't feel super strongly about it.

I think the kebab-case is fine, I don't have strong feelings either.

lib/components/note-preview/index.tsx

sandymcfadden

This might be good to have someone else look over as well, but from my look and testing it all seems to be working well as expected.
Love the accessibility improvements!

While the navigation sidebar is open, focus will be trapped to the sidebar. This improves keyboard and screen reader navigation by preventing focus from jumping to UI outside of the sidebar. The focus-trap-react package was added to accomplish this because its quality and ease-of-use. Hide unrelated content while navigation is open Prevent screen readers from reading content outside of the navigation while the navigation is open. Reorder navigation elements to improve focus navigation The previous ordering resulted in illogical focus navigation. This change ensures that the navigation elements still render visually in the same order as before, but now the stepping through the elements via keyboard or screen reader focus matches the visual ordering. Fix return focus when dismissing About dialog Previously, dismissing the About dialog would result in the first focusable element within the navigation to be focused, instead of the About link. Leveraging the `paused` property is more appropriate for our use case and fixes this issue. Adjust navigation styles to improve active focus indication Refactor styles so that is more apparent when an element is focused. Match latest develop styles Improve accessibility of revisions selector Add focus trap and aria-hidden qualities when the revisions selector is open to improve the navigation experience for keyboards and screen readers. Consolidate aria-hidden management to app layout Attempt to avoid spreading aria-hidden status across multiple components. Hopefully placing it in one location, app layout, will allow easier management in the future. Add dialog role to Revision Selector Improve accessibility for screen reader navigation by grouping Revision Selector as a dialog. Improve accessibility of note History dialog Describe the dialog and the current revision selected. Remove unused import

dmsnell reviewed Mar 2, 2021

View reviewed changes

dcalhoun marked this pull request as ready for review March 3, 2021 15:13

dcalhoun requested a review from dmsnell March 3, 2021 15:13

dcalhoun force-pushed the improve-navigation-accessibility branch from f5aea83 to 80ebd75 Compare March 3, 2021 21:23

codebykat added this to the 2.8.0 milestone Mar 4, 2021

dcalhoun force-pushed the improve-navigation-accessibility branch from e878a30 to 276b846 Compare March 4, 2021 21:42

dcalhoun requested a review from a team March 4, 2021 22:03

dcalhoun force-pushed the improve-navigation-accessibility branch from 276b846 to 65607aa Compare March 5, 2021 13:59

dcalhoun mentioned this pull request Mar 9, 2021

UI Spring Cleaning: Note info panels #2622

Merged

codebykat modified the milestones: 2.8.0, 2.9.0 Mar 10, 2021

belcherj reviewed Mar 10, 2021

View reviewed changes

sandymcfadden approved these changes Mar 16, 2021

View reviewed changes

dcalhoun mentioned this pull request Mar 16, 2021

Improve keyboard and screen reader accessibility #2726

Merged

dcalhoun force-pushed the improve-navigation-accessibility branch from 65607aa to 0dec0e1 Compare March 17, 2021 13:51

sandymcfadden force-pushed the improve-navigation-accessibility branch from 0dec0e1 to de04d19 Compare March 20, 2021 18:22

sandymcfadden merged commit 0b2622c into develop Mar 20, 2021

sandymcfadden deleted the improve-navigation-accessibility branch March 20, 2021 18:52

dcalhoun mentioned this pull request Mar 26, 2021

Tab focus highlights in dark mode are a bit jarring #2812

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve navigation accessibility #2707

Improve navigation accessibility #2707

dcalhoun commented Mar 2, 2021 •

edited

Loading

dmsnell left a comment

dcalhoun commented Mar 2, 2021

dcalhoun commented Mar 3, 2021 •

edited

Loading

dcalhoun commented Mar 3, 2021

dmsnell commented Mar 3, 2021

dcalhoun commented Mar 3, 2021

dmsnell commented Mar 4, 2021

dcalhoun commented Mar 4, 2021

belcherj Mar 10, 2021

dcalhoun Mar 10, 2021 •

edited

Loading

belcherj Mar 10, 2021

sandymcfadden Mar 10, 2021

sandymcfadden left a comment

Improve navigation accessibility #2707

Improve navigation accessibility #2707

Conversation

dcalhoun commented Mar 2, 2021 • edited Loading

Fix

Test

Scenario 1

Scenario 2

Scenario 3

Release

dmsnell left a comment

Choose a reason for hiding this comment

dcalhoun commented Mar 2, 2021

dcalhoun commented Mar 3, 2021 • edited Loading

dcalhoun commented Mar 3, 2021

dmsnell commented Mar 3, 2021

dcalhoun commented Mar 3, 2021

dmsnell commented Mar 4, 2021

dcalhoun commented Mar 4, 2021

belcherj Mar 10, 2021

Choose a reason for hiding this comment

dcalhoun Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

belcherj Mar 10, 2021

Choose a reason for hiding this comment

sandymcfadden Mar 10, 2021

Choose a reason for hiding this comment

sandymcfadden left a comment

Choose a reason for hiding this comment

dcalhoun commented Mar 2, 2021 •

edited

Loading

dcalhoun commented Mar 3, 2021 •

edited

Loading

dcalhoun Mar 10, 2021 •

edited

Loading