Skip to content

Commit

Permalink
fix(a11y): Fix keyboard navigation issues with sidebar (#8769)
Browse files Browse the repository at this point in the history
This PR fixes a number of keyboard accessibility issues with the
feedback sidebar. They are (in no particular order):
1. The radio inputs don't have a focus style for `focus-visible` (when
keyboard focused).
2. There's two close buttons there for some reason? One is invisible,
but you can tab to it?
3. The sidebar doesn't trap focus, so you can tab out of the modal and
continue tabbing through the main page (with the modal still open)
4. The sidebar doesn't steal focus. When you open it, your focus remains
on the button you used to open it. So if you want to navigate to it, you
have to go through the entire page (behind the modal) to get to it.
5. The sidebar can't be closed by 'escape'.

The fixes are:
1. Apply the same styles when focus visible as when hover
2. Wrap the component in the `BaseModal` component
3. Wrap the component in the `BaseModal` component
4. Wrap the component in the `BaseModal` component
5. Wrap the component in the `BaseModal` component

(see a theme here?)

Additionally, because the base modal has its own `open` state, I removed
the wrapping conditionally render, reducing nesting by one stop. Most of
the changes in the file are just whitespace changes.


![image](https://github.com/user-attachments/assets/28832756-cfb3-4ced-8b8c-a344edced036)

I considered also applying an auto-focus to the first input in the
sidebar, but our linter doesn't like it. Additionally MDN lists the
following [accessibility
concerns](https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/autofocus#accessibility_concerns)

> Automatically focusing a form control can confuse visually-impaired
people using screen-reading technology and people with cognitive
impairments. When autofocus is assigned, screen-readers "teleport" their
user to the form control without warning them beforehand.
>
> Use careful consideration for accessibility when applying the
autofocus attribute. Automatically focusing on a control can cause the
page to scroll on load. The focus can also cause dynamic keyboards to
display on some touch devices. While a screen reader will announce the
label of the form control receiving focus, the screen reader will not
announce anything before the label, and the sighted user on a small
device will equally miss the context created by the preceding content.

So I'll leave it off.
  • Loading branch information
thomasheartman authored Nov 15, 2024
1 parent b4d1986 commit 0465731
Showing 1 changed file with 180 additions and 187 deletions.
Loading

0 comments on commit 0465731

Please sign in to comment.