Banner accessibility fix #771
Conversation
There was a problem hiding this comment.
I hate to be so negative as this has clearly been a lot of work, but I feel like this solution is very brittle and relies on working around React instead of with it.
I've put together a minimal example of a more idiomatic approach to managing element auto-focus on render using a 'callback ref' in this CodeSandbox: https://codesandbox.io/s/dark-voice-oiho69, based on this blog post about refs and side effects.
If we need to be able to pass refs to Source buttons but currently can't then that's not an issue we should have to mess around the edges of, it's something the design system ought to support.
| // Timeout required to allow component to settle before identifying focussable elements within it | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Arbitrary timeouts to let some other process finish are usually not a good idea. What does the component 'settling' mean, and are we confident it will always take less than 1000 milliseconds? Is there a better way to detect this?
| const closeButtonElements: HTMLElement[] = [ | ||
| ...node.querySelectorAll<HTMLElement>('[aria-label="Close"]'), | ||
| ].filter( | ||
| el => | ||
| !el.hasAttribute('disabled') && | ||
| !el.getAttribute('aria-hidden') && | ||
| !checkIfElementIsHidden(el), | ||
| ); | ||
| const focusableElements: HTMLElement[] = [ | ||
| ...node.querySelectorAll<HTMLElement>( | ||
| 'button, a[href], input, [tabindex]:not([tabindex="-1"]', | ||
| ), | ||
| ].filter( | ||
| el => | ||
| !el.hasAttribute('disabled') && | ||
| !el.getAttribute('aria-hidden') && | ||
| !checkIfElementIsHidden(el), | ||
| ); |
There was a problem hiding this comment.
I feel like it would be much more straightforward to use React refs to get instances of the elements we might want to focus, rather than having to do a DOM query for stuff we already control the creation and rendering of.
If we can't currently pass refs to focusable Source components this is something we need to bring up with CSTI and potentially make a PR to Source to implement.
| <Button | ||
| aria-label="Close" | ||
| onClick={onClick} | ||
| css={button} | ||
| size="small" | ||
| icon={<SvgCross />} | ||
| hideLabel | ||
| > | ||
| Dismiss the banner |
There was a problem hiding this comment.
This is a degradation of the experience for screen reader users- 'Dismiss the banner' is a much richer and more useful prompt than 'Close' (close what, specifically?). It's generally best to avoid using ARIA if regular ol' HTML will do the job, which a button with hideLabel does.
If there's no way around doing DOM queries to get focusable elements and you need an arbitrary selector for these buttons, it would be better to use a data attribute.
| if (el.style.display === 'none' || el.style.visibility === 'hidden') { | ||
| return true; | ||
| } | ||
| const computed = window.getComputedStyle(el); |
There was a problem hiding this comment.
Calling getComputedStyle will cause style recalculation and potentially layout reflow. Determining visibility programmatically is tricky, but traversing up a DOM tree and calling this on every node will be terrible for performance, especially as this effectively has no exit condition until you reach document.body.
If we absolutely cannot avoid doing this, I think it would be better to use a TreeWalker (generally much more performant than a JS while loop) with the main container element of the banner as its root node, so it won't work its way up the entire DOM tree.
What does this change?
We need to improve the accessibility of the Banner. Currently:
tabandshift + tabkeyboard actions are not constrained to the banner while it is displayed;[TODO: complete PR details]