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

Fides-js accessibility wrap up #3510

Merged
merged 4 commits into from
Jun 9, 2023
Merged

Fides-js accessibility wrap up #3510

merged 4 commits into from
Jun 9, 2023

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Jun 8, 2023

Closes #3309

Code Changes

The bulk of the a11y work was done while developing, but this should cover the straggling items.

  • Makes the banner not come up in the tab order when it's hidden
  • Prepend the overlay instead of appending so that the banner is the first tabbed to item

Steps to Confirm

  • Bring up either cookie house or fides-js-components-demo.html
  • As a keyboard user, tab into the page. You should immediately be tabbing through the banner (before, you'd have to tab through the whole page before getting to the banner. this is far more noticeable on cookie house than the demo page)
  • Close the banner. Tab through the page. You should be able to tab through without issue (before, there would be a few tabs where it looked like nothing was happening, since it was tabbing through the hidden banner)

Pre-Merge Checklist

Description Of Changes

I went through a few sites, and I don't think there's actual regulation on where a banner should appear in a tab order. This blog post recommends having them come up as soon as possible, though I ran into some sites that have them at the very end (where you have to tab through the entire page, including footers, before you can get to the banner. example: https://stackoverflow.com/) but other sites make sure they're the first thing you tab through. example: https://liveramp.com/). I thought it made sense to have it come up as soon as possible, though this means we document.body.prepend instead of document.body.appendChild. In other words, the overlay becomes the top DOM element. Because of this, I also gave the overlay an overall high z-index to increase the chance we stay on top of the host site's elements (we probably wanted this anyway)

There might be other ways to have the banner receive focus first other than doing a prepend:

  • use the autoFocus attr on a button in the banner, however this apparently has a11y drawbacks (vscode warns against it)
  • use a portal. I don't think we need one, since we're already controlling where in the DOM we are injecting our overlay, but I think that might be a standard way of doing this kind of thing. however, adding preact-portal is another >1kb!

I haven't come across any drawbacks with prepending instead of appending yet, but we'll see!

@cypress
Copy link

cypress bot commented Jun 8, 2023

Passing run #2606 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 0998729 into 9a7cf8b...
Project: fides Commit: 5a5b2fc19b ℹ️
Status: Passed Duration: 00:49 💡
Started: Jun 9, 2023 8:23 PM Ended: Jun 9, 2023 8:24 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@allisonking allisonking marked this pull request as ready for review June 8, 2023 20:20
Base automatically changed from feature/privacy-components to main June 8, 2023 22:34
This is so that the overlay will be the first thing a keyboard user can navigate to and to help the overlay stay on top of all other elements
@eastandwestwind
Copy link
Contributor

Manually testing this- just to confirm, a side effect of this is that the banner no longer animates when it is closed. This is fine by me (and makes more sense than animating it when the user wishes it to be closed), but want to make sure it's expected.

@allisonking
Copy link
Contributor Author

Manually testing this- just to confirm, a side effect of this is that the banner no longer animates when it is closed. This is fine by me (and makes more sense than animating it when the user wishes it to be closed), but want to make sure it's expected.

ahh good catch, no that wasn't intentional! I put in a quick fix so that we get the animation when closing again. if we don't want it in the future, we can revert 0998729 but for now I didn't want to add something unintentionally :)

@allisonking allisonking merged commit e4078a7 into main Jun 9, 2023
@allisonking allisonking deleted the aking/3309/fides-js-a11y branch June 9, 2023 20:26
allisonking added a commit that referenced this pull request Jun 9, 2023
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.

Accessibility requirements for fides-js
2 participants