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

Feature: add skip to content links #96

Merged
merged 2 commits into from
Jun 23, 2021
Merged

Conversation

KeshavChawla
Copy link
Contributor

@KeshavChawla KeshavChawla commented Mar 30, 2021

What kind of change does this PR introduce? (check at least one)

  • Bugfix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Code style update
  • Refactor (refactoring or adding test which isn't a fix or add a feature)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

Adviser warns of an error with netmask (high).

Advisory code is 1658: https://www.npmjs.com/advisories/1658**

Did you test your solution?

  • I lightly tested it in one browser
  • I deeply tested it in several browsers
  • I wrote tests around it (unit tests, integration tests, E2E tests)

Problem Description

Per WCAG 2.1 AA compliance having bypass blocks on each page is necessary.

Solution Description

One notably method of solving this is by:

An alternate solution for Jam3 projects would include adding a subnav which satisfies the criteria:


This PR creates a tab-able, screen readable skip to content button which the user can click to skip the navbar on each page. It does so by using an anchor link at the bottom of the nav page to change focus.

Notable findings:

  • This link must be the first targetable focus on each page
  • While screen readers should be able to pick up content with opacity: 0; ChromeVox seems to ignore elements with opacity 0 all together when a user is navigating with their keyboard. A fix for this is to set the element with opacity: 0.0001. While this is not ideal, it guarantees the maximum number of screen readers will be able to read the link while not impacting design. In the future, when this ChromeVox bug is patched, opacity should be adjusted back to 0.

Unfocused link:
Screen Shot 2021-03-30 at 1 11 51 PM

Focused Link:
Screen Shot 2021-03-30 at 1 11 44 PM

Test with ChromeVox (audio) showing that selecting the Skip to Content skips the Nav and goes directly to focus on the "Welcome to Jam3."

Skip.To.Content.ChromeVox.mp4

Side Effects, Risks, Impact

Let me know if I should add the advisor warning to adviserrc.json.

Additional comments:

@KeshavChawla KeshavChawla changed the title feature: add skip to content links Feature: add skip to content links Mar 30, 2021
@KeshavChawla KeshavChawla added the enhancement New feature or request label Mar 30, 2021
Copy link
Collaborator

@DonghyukJacobJang DonghyukJacobJang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KeshavChawla This is great to have! Thanks for your work!

left: 0;
height: fit-content;
pointer-events: none;
opacity: 0.0001;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can opacity just be 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @DonghyukJacobJang!

Normally I would say yes, but ChromeVox has a bug where it skips reading out content with opacity 0 when a user is navigating with their keyboard. The hacky fix for this was to just set the element with opacity: 0.0001;. While not ideal, I think it guarantees the maximum number of screen readers will be able to read the link while not impacting design. That being said, if we don't need to support ChromeVox then I can set it to opacity 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DonghyukJacobJang Keshav was correct on this. For AAA sites any use of opacity animation has to use 0.0001 for it's starting opacity. So this is good to have. Let's approve and merge this if everything is groovy. ✌️

@alemesa
Copy link
Member

alemesa commented Jun 23, 2021

Merging this @DonghyukJacobJang we tested this in Tonal and worked fine

@alemesa alemesa merged commit a5aa7c2 into master Jun 23, 2021
@DonghyukJacobJang DonghyukJacobJang deleted the feature/skip-to-content branch July 28, 2021 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants