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

[#1730, #1773, #1820, #1849]Added screen reader alerts for various actions throughout the app. #1873

Merged
merged 1 commit into from Sep 19, 2019

Conversation

@tonyanziano
Copy link
Contributor

tonyanziano commented Sep 17, 2019

#1730, #1773, #1820, #1849

===

  • Added ariaAlertService.ts in packages/app/client
    • creates an invisible <span> with role="alert" that will be read by any screen reader technologies and then removed after 5 seconds

image

  • Added screen reader alerts for the following actions:
    • saving app settings
    • bot URL validation messages in Open Bot dialog
    • hiding, showing, and copying bot secret in Create Bot dialog
    • navigating to previous and next bot states in the inspector
    • toggling diff mode in the inspector
    • copying JSON in the inspector
  • Fixed accessibility around bot secret input in Create Bot dialog
    • previously, secret input was always disabled, so it was invisible to screen readers
    • secret input is now read-only
    • fixed label on secret input (was not being narrated before)
@tonyanziano tonyanziano force-pushed the toanzian/sr-toast branch from 7546241 to 88b3991 Sep 17, 2019
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 17, 2019

Coverage Status

Coverage remained the same at ?% when pulling 58afba4 on toanzian/sr-toast into e8b9ede on master.

@tonyanziano tonyanziano force-pushed the toanzian/sr-toast branch from 88b3991 to ccf2fdf Sep 18, 2019
Copy link
Member

corinagum left a comment

Some questions/suggestions before approval :)

const alert = document.createElement('span');
alert.innerText = msg;
alert.setAttribute('role', 'alert');
alert.setAttribute('style', 'position: absolute; top: -9999px;');

This comment has been minimized.

Copy link
@corinagum

corinagum Sep 18, 2019

Member

should we also make it a 0x0px <div> so in the unlikely chance it gets on screen, it's still invisible?

This comment has been minimized.

Copy link
@tonyanziano

tonyanziano Sep 18, 2019

Author Contributor

I tried this and you can't hide an element with text content: Example

I could give it an opacity of 0, but some screen readers do not read content with an opacity of 0.

This comment has been minimized.

Copy link
@corinagum

corinagum Sep 19, 2019

Member

Oh yeah, I meant should we change it from <span> to <div> so that it is adjustable to 0px by 0px. Fiddler shows that as working with overflow: hidden
https://jsfiddle.net/sxygke3n/1/

This comment has been minimized.

Copy link
@corinagum

corinagum Sep 19, 2019

Member

(also good to know about the opacity 0 thing, I didn't know that)

packages/app/client/src/ui/a11y/ariaAlertService.ts Outdated Show resolved Hide resolved
@tonyanziano tonyanziano force-pushed the toanzian/sr-toast branch from eaa2dc7 to b95ebc5 Sep 19, 2019
@tonyanziano

This comment has been minimized.

Copy link
Contributor Author

tonyanziano commented Sep 19, 2019

Addressed feedback

Copy link
Member

corinagum left a comment

Up to you on whether you want to implement <span> to <div> with overflow hidden. I still think it's a good idea.

@tonyanziano

This comment has been minimized.

Copy link
Contributor Author

tonyanziano commented Sep 19, 2019

@corinagum oh duh, overflow. I'll add that to the CSS. Good call

Copy link
Member

corinagum left a comment

Awesome! :D

@tonyanziano tonyanziano merged commit 97d88a1 into master Sep 19, 2019
2 checks passed
2 checks passed
Emulator-CI-PR #79790 succeeded
Details
license/cla All CLA requirements met.
Details
@tonyanziano tonyanziano deleted the toanzian/sr-toast branch Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.