clients icon indicating copy to clipboard operation
clients copied to clipboard

[PS-1198] Desktop / Browser: Accessibility - make TOTP countdown announce for assistive technology users

Open patrickhlauke opened this issue 2 years ago • 12 comments

Type of change

  • [x] Bug fix
  • [ ] New feature development
  • [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • [ ] Build/deploy pipeline (DevOps)
  • [ ] Other

Objective

Currently, the countdown for a TOTP is not announced dynamically for assistive technology users. Further, when navigating through a cipher/item view with a screen reader, the current timer value is announced as if it was part of the TOTP code itself. See the video in https://github.com/bitwarden/clients/issues/2657

This PR fixes both aspects: when reading through the view, the TOTP code itself is now read out/announced correctly (without the appended timer value). When focus is on the copy button, the countdown is now dynamically announced (so a screen reader user knows when it's ok to activate the button to copy, that there's enough time left for that code they're copying to be valid). This uses an aria-live region, but the region only announces when the copy button itself is focused - otherwise, the live region would have constantly made announcements as soon as the screen reader user entered the cipher/item view.

Closes https://github.com/bitwarden/clients/issues/2657

Code changes

  • apps/browser/src/popup/vault/view.component.html and apps/desktop/src/app/vault/view.component.html: make the visible totp countdown timer aria-hidden (so it's not announced when navigating to the field as part of the code itself); add a copy of the countdown number itself as part of the copy button, set to sr-only, with aria-live="polite" and aria-atomic="true" to make it continuously announce when its content gets updated. change the appA11yTitle on the button just a plain title, and instead add an extra <span class="sr-only"> inside the button to give it an accessible name. This works around an oddity in JAWS at this point - see comment below https://github.com/bitwarden/clients/pull/2660#issuecomment-1120410015
  • apps/browser/src/popup/scss/misc.scss and apps/desktop/src/scss/misc.scss: add a new utility class, naively titled .exists-only-on-parent-focus; this allows to have an element display:none (and thus not exposed at all both visually and to assistive technologies) unless its direct parent element is focused. this, in combination with the change to the view component, ensures that the sr-only live region only "exists" (and thus only does the live region announcements) when the parent totp copy button has focus (otherwise the live region would lead to continuous announcements all the time while the user is anywhere in the cipher/item view...a continuously announced countdown regardless of which element/control the user was currently on/reading, which would be a horrid user experience)

Screenshots

Video on Windows using NVDA

https://user-images.githubusercontent.com/895831/167294971-3a44504c-7af9-43cf-a70f-1756ba8bb894.mp4

0:00 - 0:05 in extension, moving to the TOTP using reading keys. the countdown number is not read out as if it were part of the code 0:05 - 0:22 setting focus to the copy button, note the continuous announcement of the countdown 0:22 - 0:30 moving away from the button, note the countdown stops being announced 0:30 - 0:44 moving back to the button, the countdown is announced again 0:45 - 1:10 in native, setting focus to the copy button and listening to the countdown announcement 1:10 - 1:18 moving away from the button, then using reading keys to return to the TOTP to verify the code itself is announced, without the appended countdown number

Testing requirements

  • go to a cipher/item with a TOTP field
  • test using a screen reader that once focus is on the copy button, there are dynamic announcements for the countdown
  • read through the view with reading keys, and verify that the TOTP code itself is read out correctly, without the current countdown value being appended to it

Note that live regions are generally a bit flaky. Even when testing with NVDA (the most robust on Windows when it comes to this, in my experience), there were times when the live region didn't seem to announce at all. Often this can be due to having started NVDA after Chrome (for the extension) or the native desktop app (for desktop) was launched. In those cases, fire up NVDA first, before the other apps.

Noticed in testing that JAWS 2022 has some fundamental problems it seems with announcing most of the buttons in the extension, and does not seem to announce the live region here at all. It also seems to have much more fundamental problems announcing item content in the list etc. This is either due to some bug in chrome/chromium in combination with JAWS, or something about the way the markup is currently structured that really doesn't gel with JAWS. this will require a much larger investigation beyond the scope of this PR.

It would be good to test this also on macOS with VO, and other combinations like Firefox+NVDA on Windows.

Before you submit

  • [x] I have checked for linting errors (npm run lint) (required)
  • [ ] This change requires a documentation update (notify the documentation team)
  • [ ] This change has particular deployment requirements (notify the DevOps team)

Linting is currently broken.

patrickhlauke avatar May 08 '22 11:05 patrickhlauke

Made a tweak that at least partially helps with the JAWS issue on Windows. Particularly on native desktop, because JAWS wasn't seeing/announcing the live region, the TOTP didn't announce any countdown at all, anywhere. With this tweak (using title instead of appA11yTitle, and adding an sr-only span inside the button to provide the "Copy Value" text as accessible name, the aria-label doesn't override the content of the button, and in JAWS it at least announces the countdown value at the point where the button receives focus (but doesn't dynamically update the announcement at this stage due to whatever bug it has with live region in this case). Moving away from the button and returning to it does announce the most recent countdown number though, so while not perfect, it at least conveys some info to the user about the "freshness" of that TOTP code and if it's worth copying or waiting until a new code is generated.

New video showing very quickly the behaviour after the tweak:

https://user-images.githubusercontent.com/895831/167296035-74e3c19d-6436-4cd7-b2cf-dec639c1b33c.mp4

0:00 - 0:12 showing how NVDA still behaves correctly after the tweak 0:12 - 0:36 JAWS announcing the live region in the extension in chrome (though it has more fundamental problems at the moment in the extension, beyond the scope of this PR) 0:36 - 0:53 JAWS in the native desktop app - as noted, the live region announcement of the countdown doesn't work, but at least it now announces the countdown number at the very moment the button received focus

patrickhlauke avatar May 08 '22 12:05 patrickhlauke

Would still love feedback on how this performs on macOS (in the extension, and the native desktop app). currently not set up on my Mac to test this properly.

In theory, with this latest approach, even if the live regions don't, for some reason, work correctly, at least the button announces the current timer value at the moment it receives focus. Which is an improvement over the current situation where it is not announced (or rather, where it's only announced when using reading keys, and then making it appear as the last one/two digits of the TOTP code, confusingly)

patrickhlauke avatar May 08 '22 12:05 patrickhlauke

For reference, here's how it behaves in Firefox with NVDA. It seems Fx announces a change in the accessible name as a whole for the component (this is where you fall into quirky/undocumented behavior of browser/AT combinations). Still, it only does it on focus. A bit more verbose than the Chrome/NVDA scenario, but it does convey the necessary information to AT users regardless.

https://user-images.githubusercontent.com/895831/167297417-fd1879bf-f2f6-48af-9b4c-824d51d8ffa0.mp4

patrickhlauke avatar May 08 '22 13:05 patrickhlauke

lastly, here is the behavior in Firefox with JAWS. In this case, only the countdown number is announced when it changes. So the weirdness in the previous video (Firefox/NVDA announcing the full "Copy Value 30, Copy Value 29, ..." seems an oddity of NVDA in combination with Firefox) is not present here.

https://user-images.githubusercontent.com/895831/167297950-8adcff58-fcc3-486f-a4e6-53740ba1efd7.mp4

patrickhlauke avatar May 08 '22 13:05 patrickhlauke

Also note that this probably applies to the web-based vault as well (but I've not delved into that particular repo)

patrickhlauke avatar May 08 '22 13:05 patrickhlauke

updated and linted, but it was now throwing me cryptic messages about jslib. my knowledge is quite limited here, not sure how to unmess this. has something changed in master relating to jslib? /cc @Hinton

patrickhlauke avatar Jun 21 '22 19:06 patrickhlauke

@patrickhlauke Thank you for updating your PRs. From the previous commits on this PR I don't see any changes you made in jslib. The git sub-module (jslib) previously was bound to each client. Now with the mono-repo , we no longer have a git sub-module and the shared code lives in /libs.

You should be able to delete all jslib folders locally and only run npm ci from the root dir to get your branch into a working state. The deletion should also clear up the conflicts once they are pushed.

djsmith85 avatar Jun 21 '22 19:06 djsmith85

thanks @djsmith85 ... i'll give it a whirl

patrickhlauke avatar Jun 21 '22 21:06 patrickhlauke

yup, nuking the various jslib folders seems to have done the trick

patrickhlauke avatar Jun 21 '22 21:06 patrickhlauke

though i think after the various PRs i have on the go here are merged, i'll do a completely clean re-grab of this repo, now that it's been stabilised a bit in terms of structure etc. just to make sure i don't have old rubbish hanging around somehow...

patrickhlauke avatar Jun 21 '22 21:06 patrickhlauke

and doh, i just worked out why i was getting all this mess. was diligently doing a git pull on my master branch, and merging master into my PR branches...forgetting that as i'm not on the repo (like i am in some other projects), i needed to do a git fetch upstream / git merge upstream/master instead. sigh...

patrickhlauke avatar Jun 21 '22 22:06 patrickhlauke

not to hassle, but as I'm worried about too much divergence from main branch ... any chance this could be reviewed soon @djsmith85 ?

patrickhlauke avatar Jul 21 '22 12:07 patrickhlauke

Yeah, worth testing in macOS/Safari/VoiceOver etc just to triple-check. each browser/AT has its weird quirks with live regions

patrickhlauke avatar Sep 25 '22 12:09 patrickhlauke

@patrickhlauke This has cleared QA and is ready to merge. This will be included in 2022.11

djsmith85 avatar Oct 03 '22 14:10 djsmith85