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

Adding message for screenreader #78

Merged
merged 3 commits into from
Jul 3, 2020

Conversation

nataliecarey
Copy link
Contributor

@nataliecarey nataliecarey commented Jun 11, 2020

Previously screen readers were having problems when the countdown reached a number of seconds. Either they would refuse to read it because it kept changing or they'd try to read it every second.

This introduces a separate counter for screen readers when there's one minute remaining.

I've only been able to test on Safari on Mac, I'd love it if someone else could test on other tools.

One thing I've been tempted to fix but haven't is that the elements regenerate every second even when they don't need to change. That's been part of this implementation and it hasn't caused problems as far as I'm aware. I'm nervous about changing this without evidence that it's working properly. This change should (IMHO) be added to Assets Frontend and a new release made, that way people who haven't upgraded to govuk-frontend yet can get through DAC assessment.

The new solution can be previewed here https://hmrc-timeout-dialog-a11y.herokuapp.com/ (username: timeout, password dialog) to confirm that it behaves in an appropriate way on different screen readers.

@adamliptrot-oc
Copy link
Contributor

adamliptrot-oc commented Jun 11, 2020

In JAWS 18, JAWS 2020 and NVDA it’s continually saying “For your security we will sign you out in 2 minutes”, over and over.
That will be because it is updating the time display on every tick, so triggering the live region.
Only update the time display when you need to change the actual display and the should improve it.

@adamliptrot-oc
Copy link
Contributor

On Voiceover I didn’t get any announcements after the initial one. I didn’t hear any updates for the other time periods mentioned - nothing for 1 minute, 40 seconds or 20 seconds as indicated in the Slack channel discussion.

Copy link
Contributor

@matthewmascord matthewmascord left a comment

Choose a reason for hiding this comment

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

I tested it out on Safari but I also couldn't get any read-outs after the initial one. I think it's due to the aria-live attribute being dynamically turned on and off. Perhaps I'm missing something but I thought this attribute should be set on any region that receives dynamic updates as per https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Live_Regions

It seems to work better if instead of dynamically calling setAriaLive, you just set it on the containing div .

@matthewmascord
Copy link
Contributor

I think you can just add aria-live="polite" to

var $dialog = utils.generateDomElementFromString('<div id="hmrc-timeout-dialog" tabindex="-1" role="dialog" class="hmrc-timeout-dialog">')

@nataliecarey
Copy link
Contributor Author

I've updated the PR to resolve the issues raised during testing in different screenreaders.

I've also updated the prototype example: https://hmrc-timeout-dialog-a11y.herokuapp.com/ (username: timeout, password dialog)

@matthewmascord
Copy link
Contributor

I've updated the PR to resolve the issues raised during testing in different screenreaders.

I've also updated the prototype example: https://hmrc-timeout-dialog-a11y.herokuapp.com/ (username: timeout, password dialog)

I tried again in Google Chrome with Apple Voiceover. I got an audio announcement for the 2 minute expiry and then a further announcement saying I had 20 seconds remaining. However, the visible text said I had only 7 seconds remaining. The visible text counted down to zero before redirecting to the signed out page.

Copy link
Contributor

@matthewmascord matthewmascord left a comment

Choose a reason for hiding this comment

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

image
Looks like there is a visual regression in the new version.

@adamliptrot-oc
Copy link
Contributor

I tried again in Google Chrome with Apple Voiceover. I

You should only be using Safari with Voiceover for testing. Chrome can give you odd results.

@matthewmascord
Copy link
Contributor

I get the same in Safari with Voiceover.

@adamliptrot-oc
Copy link
Contributor

Certainly better with VO on Mac.
Got the initial warning followed by 2 updates. 👍
I can still navigate to the skip link when the dialog is present however.
Might be worth stopping the countdown once it reaches zero. I've seen it count to -1 a couple of times and my wifi dropped and it just kept counting backwards (-43 seconds) which looked a bit odd.

Copy link
Contributor

@matthewmascord matthewmascord left a comment

Choose a reason for hiding this comment

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

When spinning up the app with npm start, the timeout dialog displayed at http://localhost:3000/components/timeout-dialog/preview does not count down properly. It first says you have 2 minutes remaining (which isn't the case), and you then only get another change when there is only 7 seconds remaining.

The example is using the default parameter values with a timeout of 70 seconds and a countdown at 68 seconds remaining.

I think I've narrowed the problem down to the way the setTimeout is being set in runUpdate(). If there is more than 60 seconds remaining on the countdown, runUpdate is not executed for another 60 seconds, which means that it's not run until there's only 8 seconds remaining.

I've suggested a change below that makes the existing test fail.

This might be an unsupported edge case, but it is the default example, so we should either make it work for this example or update the default example to be an example that is valid and document the valid values for timeout.

'data-sign-out-url': 'logout'
})

pretendSecondsHavePassed(10)
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails if I do:

      pretendSecondsHavePassed(5)
      pretendSecondsHavePassed(5)

@@ -198,19 +236,17 @@ function TimeoutDialog ($module) {
if (counter <= 0) {
signOut()
}
currentTimer = window.setTimeout(runUpdate, counter > 60 ? 60000 : 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this condition needs to be counter > 120 or just set the timeout for every second. Otherwise if timeout is between 60 and 120 you are not going to get a countdown until only a few seconds are remaining.

@nataliecarey nataliecarey force-pushed the PLATUI-454_timeout-dialog-accessibility branch 3 times, most recently from 27a364a to 7db4397 Compare July 3, 2020 15:54
@nataliecarey nataliecarey force-pushed the PLATUI-454_timeout-dialog-accessibility branch from 7db4397 to 31e52ca Compare July 3, 2020 15:59
@nataliecarey nataliecarey dismissed matthewmascord’s stale review July 3, 2020 16:03

This is fixed in a later version.

matthewmascord
matthewmascord previously approved these changes Jul 3, 2020
@nataliecarey nataliecarey force-pushed the PLATUI-454_timeout-dialog-accessibility branch from d863279 to 4704411 Compare July 3, 2020 16:27
@nataliecarey nataliecarey merged commit 7eea042 into master Jul 3, 2020
@matthewmascord matthewmascord deleted the PLATUI-454_timeout-dialog-accessibility branch January 25, 2021 20:03
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.

3 participants