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

Improve accessibility of keyboard focus and skip links in IE11 #18671

Closed
westonruter opened this issue Oct 10, 2018 · 7 comments
Closed

Improve accessibility of keyboard focus and skip links in IE11 #18671

westonruter opened this issue Oct 10, 2018 · 7 comments

Comments

@westonruter
Copy link
Member

westonruter commented Oct 10, 2018

What's the issue?

In WordPress themes it is extremely common for there to be a JS file included like this: https://github.com/Automattic/_s/blob/master/js/skip-link-focus-fix.js

The purpose of this JS is to fix an accessibility issue in IE11 for keyboard-only users. In particular, per Automattic/_s#136:

The problem is that when someone clicks a skip link, the viewport is scrolled to the #content, but pressing tab will bring focus to the site's main navigation. The expected behavior would be to select the first link in the #content, not the site's main navigation. Of course this is the browser's default behavior, and not really a _s bug, but it is an opportunity to make _s-based themes more accessible to users that navigate solely through keyboard input.

I learned about this issue via an article written by Nicholas C. Zakas. He suggests setting focus to the #content using JavaScript. I've adapted his example to use jQuery and to better conform to WordPress coding standards.

In work on the AMP for WordPress plugin, this is a common bit of JS that is encountered which must be suppressed to avoid validation errors. However, in doing so then IE11 users miss out on the a11y fix.

See also:

Should this a11y fix be made part of the AMP runtime?

What browsers are affected?

IE11

@aghassemi
Copy link
Contributor

This should be fairly easy to add since we already do most of the work that script does. We always intercept navigation and know when it is an anchor nav already so no need to listen to hashchange.
All we need is to add the if(IE) { focus() } logic to this line:

scrollIntoViewInternal_(element, parent) {

Anyone interested in contributing this fix?

(Our IE11 support is essentially "it will not be perfect, but it will work", so if this was not an a11y issue it would have been up for debate as this will increase V0 size by a bit. We are looking at only a few bytes anyway, so I am 👍 @kristoferbaxter )

@kristoferbaxter
Copy link
Contributor

I agree with your assessment @aghassemi. Would love to see this change and am happy to find some other bits to remove in our quest to a smaller runtime.

@ampprojectbot ampprojectbot added this to the Pending Triage milestone Oct 16, 2018
@sagarkbhatt
Copy link
Contributor

I am new to amphtml codebase and ready to fix this issue to get familiar with the codebase. Although I can write some good JS and excited to explore web components and progressive enhancement used in amphtml codebase.

@aghassemi
Copy link
Contributor

Thanks @sagarkbhatt. bit.ly/helpamp has some good getting started documentation. Also this talk from AMP Contributor Summit could be helpful: https://www.youtube.com/watch?v=kjn322ELFvs&t=0s&list=PLXTOW_XMsIDQTgsP8P77-aTu26D4_N3Kt

sagarkbhatt added a commit to sagarkbhatt/amphtml that referenced this issue Oct 29, 2018
sagarkbhatt added a commit to sagarkbhatt/amphtml that referenced this issue Oct 30, 2018
Used tryFocus instead of element.focus
@sagarkbhatt
Copy link
Contributor

sagarkbhatt commented Oct 30, 2018

@aghassemi
While debugging I figured out that we are not reaching till scrollIntoViewInternal_ when user click on skip to link instead, runtime will return from handleNavClick_ function in navigation.js

Ref code:
https://github.com/ampproject/amphtml/blob/master/src/service/navigation.js#L450-L463

sagarkbhatt added a commit to sagarkbhatt/amphtml that referenced this issue Oct 30, 2018
@sagarkbhatt
Copy link
Contributor

sagarkbhatt commented Oct 30, 2018

/to @aghassemi I have updated changes to fix the issue that I mentioned above. Can you please take a look at it.

Thank you

sagarkbhatt added a commit to sagarkbhatt/amphtml that referenced this issue Oct 30, 2018
cvializ pushed a commit that referenced this issue Oct 30, 2018
* [#18671], Fix a11y issue for IE platform

* [#18671]: PR Feedback fix

Used tryFocus instead of element.focus

* [#18671]: Update implementation approach

* [#18671], Remove unused import
sagarkbhatt added a commit to sagarkbhatt/amphtml that referenced this issue Oct 31, 2018
Enriqe pushed a commit to Enriqe/amphtml that referenced this issue Nov 28, 2018
* [ampproject#18671], Fix a11y issue for IE platform

* [ampproject#18671]: PR Feedback fix

Used tryFocus instead of element.focus

* [ampproject#18671]: Update implementation approach

* [ampproject#18671], Remove unused import
@westonruter
Copy link
Member Author

westonruter commented Oct 21, 2019

cf. Automattic/_s#1206 and Automattic/_s#136

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants