-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Screen reader support #1182
Screen reader support #1182
Conversation
This was causing new lines to join words together
Does this respect my keyboard echo settings? If not, that's amazingly annoying. I mention it because there is an anti-pattern in i.e. google docs where you get the screen readers to read via echoing to a live region, and this both doesn't respect settings and doesn't work out with punctuation (some is silent because it's trying to read it as words). As someone who very intentionally doesn't use character or word echo, being forced into it would be suboptimal. It's distracting and not very useful when i.e. on conference calls or whatever, because either it's pulling you away from whatever else is important or everyone else has to hear it if you're unlucky enough not to be able to have headphones. |
@camlorn character echo settings are respected, word echo does not work currently but it could potentially later on (I made a note in the PR). The live region is used to read out the command output, if you have anything I need to watch out for here I'd love to hear it. The live region seemed to handle perfectly what I wanted with the web APIs I have access to; to allow reading out of a stream of text (that may be incomplete) and stop the screen reader upon typing or when there is too much output. |
27a7182
to
a19b05f
Compare
I was planning on allowing calls to scrollPage/scrollToTop/scrollToBottom to scroll and focus in navigation mode, unfortunately screen readers take complete control of the keyboard in these modes and I cannot intercept the needed keyboard strokes. In case it's useful later, here's the commit b138264 |
646813a
to
7c8b64a
Compare
This is ready for review |
Pushing to v3.2.0 as this is a big PR and we have a bunch out for review in v3.1.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🍣
FYI @jluk
This PR adds support for screen readers (targeting NVDA on Windows initially). Screen reader support is disabled by default and must be opt in (
screenReaderMode
setting), this is so we can avoid all the DOM manipulation when we don't need it for performance reasons.Currently supported
Known issues
Part of #731