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

Fix details polyfill for JAWS and NVDA in IE #440

Merged
merged 1 commit into from Apr 7, 2017

Conversation

Projects
None yet
5 participants
@selfthinker
Member

selfthinker commented Apr 4, 2017

What problem does the pull request solve?

As reported in #130, the details/summary cannot be opened when using JAWS or NVDA in Internet Explorer.
As part of my investigation I also found that using the enter key on the summary would also submit a form in IE if details is used within a form.
This fixes both issues.

Some screen readers map the screen reader key + space (or enter) to the click event. As you can see in Event Handlers and Screen Readers, the click event is usually the only event fired for IE and JAWS/NVDA combinations.

Our polyfill is currently listening for the keyup, keypress and mouseup events. It used to listen to the click instead of the mouseup event, but that was changed because it made it trigger twice in Chrome.

I've now changed it to use click and keydown. As far as I know, it does not trigger twice anymore.
I've also done some minor refactoring.

How has this been tested?

I have not tested within Elements but have isolated all the important bits into a JSbin. (I have added some debugging under that, not in a console because I wanted to easily test in older and mobile browsers.) See the current version with the bugs and the debugging in a JSbin for a comparison.

I have tested in various browsers and screen reader combinations, mainly IE and Firefox with JAWS and NVDA, both and Chrome on its own. I found that it's more likely to be troublesome if the browser doesn't support details, so I concentrated on testing in older browsers via Browserstack (Firefox 48 and under, Safari 5.1 and under, Android Browser 3 and under, Windows Phone; there is no version of Chrome which is old enough). I tested with mouse and keyboard and touch and Dragon when I could.

What type of change is it?

Bug fix (non-breaking change which fixes an issue)

@gemmaleigh gemmaleigh temporarily deployed to govuk-elements-review-pr-440 Apr 4, 2017 Inactive

@tvararu

tvararu approved these changes Apr 4, 2017

I haven't tested this but the code looks good and from the very detailed problem description it makes sense. 👍

@selfthinker

This comment has been minimized.

Member

selfthinker commented Apr 4, 2017

I can see that @gemmaleigh has now deployed this to Heroku. (Thanks! Or does that happen automatically?)
That means you can now test this change in-situ on https://govuk-elements-review-pr-440.herokuapp.com/typography/#typography-hidden-text

@gavboulton

This comment has been minimized.

Contributor

gavboulton commented Apr 4, 2017

❤️

@gemmaleigh

This comment has been minimized.

Contributor

gemmaleigh commented Apr 4, 2017

@selfthinker I set it up so it happens automatically for each PR, I thought it might be useful for others to help out with reviewing.

Thanks so much for your work on this! 💯

@gemmaleigh

This comment has been minimized.

Contributor

gemmaleigh commented Apr 5, 2017

There's only one thing I have noticed.

In Chrome, the current behaviour is that you can press either space, or enter to toggle the hidden content.

With this solution, only using the spacebar works.

From this article: http://accessibleculture.org/articles/2012/03/screen-readers-and-details-summary/

Clicking or pressing the Enter or Space keys on the summary element should toggle the visibility of the details element's content.

So I think we'd want to preserve that existing behaviour.

@selfthinker

This comment has been minimized.

Member

selfthinker commented Apr 5, 2017

Thanks @gemmaleigh, I didn't spot that. I tested in Chrome, but this shows that it's better to test in the real environment because it works in my JSBin but not when used within Elements.
I'll have a look.

@selfthinker selfthinker force-pushed the details-polyfill branch from 742a825 to 5393b4d Apr 6, 2017

@gemmaleigh gemmaleigh temporarily deployed to govuk-elements-review-pr-440 Apr 6, 2017 Inactive

@selfthinker selfthinker force-pushed the details-polyfill branch from 5393b4d to 12aeb5b Apr 6, 2017

@gemmaleigh gemmaleigh temporarily deployed to govuk-elements-review-pr-440 Apr 6, 2017 Inactive

@selfthinker

This comment has been minimized.

Member

selfthinker commented Apr 6, 2017

I have pushed a new version.
It fixes previously mentioned bugs plus a new bug we found: The space key would not open the details in Firefox (it would trigger two clicks, so it would open and close quickly again).
Sorry, I usually like granular commits but I tried and couldn't disentangle everything, so it is all in one single commit unfortunately.

Fixing the enter key was easy but that created a new bug and fixing that led to even more bugs... So, all in all it turned out to be more complex than I initially though. Big thanks to @tvararu, @joelanman and @splitbrain who helped me solve most issues.

I re-tested the automatically deployed version in Firefox and IE 11 in Windows 10 with and without JAWS and NVDA, macOS Firefox, Chrome, Safari (all latest), Edge and Chrome in Win 10, Firefox 45 (a version which doesn't support details), IE 9, IE 8, IE 7, Safari 5.1 and some more.
This version doesn't work as expected in two of those browser combinations:

  • IE 8 and under don't work with the enter key (but work with space and click)
  • Safari 5.1 and under only work with click, so neither space nor enter works; I suspect the same Safari version in iOS wouldn't work either when a keyboard is attached, but I don't know how to test that

I haven't tried to debug those two new issues yet. I will do that tomorrow.
I would think the IE 8 issue is the least important because we only support IE 8-10 on a functional level and it is functional enough. The Safari 5.1 issue bugs me though, as it would mean no keyboard access at all. If it's too difficult to fix, I might leave it as it is.

Fix details polyfill
This fixes various issues in the details polyfill
and refactors the code a bit.

**Fix details not working with screen readers in IE**

Change the `mouseup` event to `click`
as that is the only event that JAWS and NVDA use in IE,
even though the keyboard is used.
See also http://unobfuscated.blogspot.co.uk/2013/05/event-handlers-and-screen-readers.html
This fixes #130.

**Fix enter on details' summary submitting form**

When details/summary is used within a form,
the enter key submits the form in some browsers
(e.g IE). This prevents that by
a) preventing the default action for enter and
b) changing `keyup` to `keypress` as `keyup` would be too late.

**Fix space key not opening details in Firefox**

When trying to open the details with the space key in Firefox,
they open and close again.
Not sure why that is happening, but preventing keyup fixes it.
@selfthinker

This comment has been minimized.

Member

selfthinker commented Apr 7, 2017

I have fixed IE 8 and under, and Safari 5.1 now works with enter but not with space. I'm happy with that.
All in all, this fixes things in 3 modern browser combinations (IE with JAWS or NVDA, IE without them and Firefox) and partly breaks something in one old browser (Safari 5.1 and under).
(I have also just updated the original description to reflect the changes.)

@gemmaleigh

This comment has been minimized.

Contributor

gemmaleigh commented Apr 7, 2017

Thanks @selfthinker! Very happy to merge this. 👍

@gemmaleigh gemmaleigh merged commit 9eb924d into master Apr 7, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gemmaleigh gemmaleigh deleted the details-polyfill branch Apr 7, 2017

@jassalrichy

This comment has been minimized.

jassalrichy commented on 40c554e Apr 7, 2017

Arrow heads no longer appear when using latest version of FireFox (52.0.2)

This comment has been minimized.

Member

selfthinker replied Apr 7, 2017

I cannot reproduce that. I just tested in the same version (52.0.2) on a Mac as well as Windows 10 and it worked fine.
I know the arrows generally don't show in Firefox (which was the same before this change) and that it's _shame.scss which will bring them back.

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