-
Notifications
You must be signed in to change notification settings - Fork 233
Fix problems with Braille output devices #1396
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
Conversation
…he mjx-speech node
…ut to the help dialog.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1396 +/- ##
===========================================
- Coverage 86.49% 86.48% -0.01%
===========================================
Files 340 340
Lines 85813 85818 +5
Branches 3177 4815 +1638
===========================================
- Hits 74220 74218 -2
+ Misses 11593 11577 -16
- Partials 0 23 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zorkow
left a comment
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.
Only one comment.
I suppose we should also add a line for NVDA to the helpData in KeyExplorer.
ts/ui/menu/Menu.ts
Outdated
| if (speech && this.settings.speech) { | ||
| Menu.loading++; // pretend we're loading, to suppress rerendering for each variable change | ||
| this.menu.pool.lookup('speech').setValue(false); | ||
| Menu.loading--; |
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.
Could that be wrapped into the noRerender method?
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.
Yes it should. I think it wasn't available in the develop branch when the top-braille branch (on which this was based) was created. I'll make the change.
I had written a paragraph about Braille output and thought it was included (one of the commits even says that). But it seems to be missing, so I must have messed up the commit and lost it. I'll write it again. |
…nd use noRender() as requested in review
|
I've made the changes. It looks like the hiding of the "replace speech" menu item was also lost, so I added that again, too. Not sure what I did wrong, there. Do you want to re-review, or shall I just merge? |
zorkow
left a comment
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.
|
I've added the "b" key to toggle the Braille menus' "Combine with Speech" option from within the explorer, as requested, and updated the help dialog to include it. |
This PR fixes problems identified with using a Braille output device in JAWS and NVDA on Windows. It turns out that for the
aria-braillelabelto be read in JAWS, therolebeingapplicationdoesn't work, whilerole="img"does. But we need the speech element to be in an element withrole="application"in order to remain in focus mode, so this PR wraps the speech node in an additionalmjx-speech-containerelement withrole="application"and makes the speech node haverole="img".Not all screen readers handle the
aria-braillelabelattribute, however (e.g., NVDA), but if Braille is in thearia-label, they will pass it on to the Braille device. So we have added a menu option to include the Braille as part of the speech string. This lists the Braille first, followed by 40 blank Braille cells, followed by the speech string. The brail device should show the braille, but the speech will be off the right-hand edge and will not be displayed, but it will be spoken. In NVDA the Braille itself is not spoken (but in JAWS and some other readers it is, so this can't be the default setting).We also add an option to replace the speech with the Braille, but that item is hidden and only kept as a backup in case it is needed.
Finally, we set the
aria-braillroledescriptionto try to override theimgrole description, for those screen readers that process it.