Skip to content

Improve Braille keyboard shortcut support, especially for Focus displays#13152

Merged
feerrenrut merged 14 commits into
nvaccess:masterfrom
krzysz00:improve-braille-shortcuts
Jan 28, 2022
Merged

Improve Braille keyboard shortcut support, especially for Focus displays#13152
feerrenrut merged 14 commits into
nvaccess:masterfrom
krzysz00:improve-braille-shortcuts

Conversation

@krzysz00

@krzysz00 krzysz00 commented Dec 10, 2021

Copy link
Copy Markdown
Contributor
  • Add support for gestures that toggle multiple modifier keys simultaniously

  • Remove the redundant "pressed" from the announcement for a modifier key

  • Update documentation to document the existense of the virtual modifiers
    and to account for new keyboard shortcuts

Link to issue number:

Fixes #7706

Summary of the issue:

The Freedom Scientific Braille driver doesn't bind the virtual modifier keys, and there are no bindings for toggling several modifier keys at once (which Jaws supports).

Description of how this pull request fixes the issue:

The PR adds support for toggling multiple modifiers to the Braille input system and adds bindings for them to the Freedom Scientific display driver.

Testing strategy:

Launching the changed NVDA and making sure I can use my Focus 40 Blue to input keyboard shortcuts, including Cntr+S in Notepad, Ctrl+Shift+P in VS Code (both with the Ctrl+Shift binding and the Control and Shift bindings), and Alt+1 in the Windows terminal. I also ensured that starting a keyboard shortcut flushed the contracted Braille buffer.

Known issues with pull request:

One caveat is that Shift goes to not 2 and not dot 7, since we need dot7+dot8 for sending contracted Braille.

Change log entries:

New features
- Added commands for toggling multiple modifiers simultaneously with a Braille display (#13152)

Changes
- Added default bindings for toggling modifier keys to Freedom Scientific displays (#13152)

Bug fixes

For Developers

Code Review Checklist:

  • Pull Request description:

    • description is up to date
    • change log entries
  • Testing:

    • Unit tests
    • System (end to end) tests
    • Manual testing

    Manual testing seems to be all I can do

  • API is compatible with existing add-ons.
    (No breaking API changes)

  • Documentation:
    I added some documentation that didn't previously exist

    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:

    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

- Add support for gestures that toggle multiple modifier keys simultaniously

- Remove the redundant "pressed" from the announcement for a modifier key

- Update documentation to document the existense of the virtual modifiers
and to account for new keyboard shortcuts
@CyrilleB79

Copy link
Copy Markdown
Contributor

Hello @krzysz00

Thanks for this PR.
Just to inform you that the modifications in the change log should be reverted.
Indeed, NVAccess takes care to update it with what they find in the initial PR description just before merging the PR. This allow them to avoid too many merge conflicts on this file.

@MarcoZehe

Copy link
Copy Markdown
Contributor

Great initiative!

One thing I noticed is that the chapter in the user guide has a lot of typos still. Might be worth going over these again and clean those up just like you did for the changes entries.

And a question: is it intentional that you omitted the chord from these modifier toggles? In the documentation for JAWS input using Focus displays, it states that one should use Dot 8 chord plus the other modifier dot. If I see your script assignments correctly, you only require dot 8, without the space bar, and the other dot for the toggle. Is this intentional?

@krzysz00

krzysz00 commented Dec 10, 2021

Copy link
Copy Markdown
Contributor Author

And a question: is it intentional that you omitted the chord from these modifier toggles? In the documentation for JAWS input using Focus displays, it states that one should use Dot 8 chord plus the other modifier dot. If I see your script assignments correctly, you only require dot 8, without the space bar, and the other dot for the toggle. Is this intentional?

Does "dot 8 chord" there mean that the space bar should be included? I wasn't familiar with the terminology and so assumed it just meant dot 8. If we want to include the space bar, then I'll also go ahead and put Shift back on dot 7.

(as to the typos, turns out the VSCode spell checker wasn't on for Text2Tags files)

@MarcoZehe

Copy link
Copy Markdown
Contributor

Does "dot 8 chord" there mean that the space bar should be included? I wasn't familiar with the terminology and so assumed it just meant dot 8. If we want to include the space bar, then I'll also go ahead and put Shift back on dot 7.

Yes, that's what "chord" means in the Freedom Scientific, and many other Braille display manufacturers, terminology. Some dots with chord means to press them together with the space bar.

@josephsl

josephsl commented Dec 10, 2021 via email

Copy link
Copy Markdown
Contributor

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit a7f246321e

@krzysz00

Copy link
Copy Markdown
Contributor Author

@josephsl Thank you for the history!

I've updated my PR to make all the commands I introduced chorded and to bring them back in line with the Jaws specifies.

One lingering question I have is whether we want to preemptively support the full combinatorical explosion of modifier chords or whether what I subjectively decided were the common combinations (from my personal experience) are enough for now.

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 06bf71f61b

@krzysz00

Copy link
Copy Markdown
Contributor Author

@CyrilleB79 Should I do anything with the removed changelog entries other than put them in the PR description?

@CyrilleB79

Copy link
Copy Markdown
Contributor

@krzysz00 you have correctly filled the template for the initial description (including the "Change log" paragraph). Thus you do not need to do anything else regarding the change log; NVAccess will take care of it once this PR is reviewed by them and ready to be merged.

If you consider that your work on this PR is finished, i.e. ready to be reviewed by NVAccess, you need to switch this PR to "Ready" state, i.e. do not keep it as draft anymore. NVAccess will then review it and comment according to their priorities.

Note that I am just a contributor and I do know very few regarding the content of this PR, so I will not personally provide a review.

@krzysz00 krzysz00 marked this pull request as ready for review December 14, 2021 01:01
@krzysz00 krzysz00 requested review from a team as code owners December 14, 2021 01:01
@krzysz00

Copy link
Copy Markdown
Contributor Author

@josephsl Are you in a position to review this?

@josephsl

josephsl commented Dec 18, 2021 via email

Copy link
Copy Markdown
Contributor

@krzysz00

Copy link
Copy Markdown
Contributor Author

If the good reviewers aren't assigned here, could someone add them? I don't seem to have the ability to do that.

@krzysz00

Copy link
Copy Markdown
Contributor Author

It's been a month since there's been any movement here - should I do something to help move this along?

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks fine to me.
Can you ensure someone else has been able to test this with a braille display?
@LeonarddeR Since you worked on the emulated kb gestures, could you also review this?

@feerrenrut feerrenrut requested a review from LeonarddeR January 16, 2022 10:01
@krzysz00

Copy link
Copy Markdown
Contributor Author

@feerrenrut Sadly, I don't know anyone else who has one of these displays, so I can't @ them in.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

cc @bramd

@Qchristensen Qchristensen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

UserGuide changes read well. Good job!

@krzysz00

Copy link
Copy Markdown
Contributor Author

@Qchristensen Thanks!

@feerrenrut

Copy link
Copy Markdown
Contributor

Given the time that has past already, we'll just have to go ahead with this and let alpha users test.

@feerrenrut feerrenrut removed the request for review from LeonarddeR January 28, 2022 08:13
@AppVeyorBot

This comment has been minimized.

@feerrenrut feerrenrut merged commit 3c6915b into nvaccess:master Jan 28, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Jan 28, 2022
@cary-rowen

Copy link
Copy Markdown
Contributor

Hello,

I'm translating the pr changes and I'm wondering if toggling modifiers will work for other braille displays, I really don't know what "Virtually toggles the some keyskeys to emulate a keyboard shortcut with braille input" does.
Can anyone provide more information?

Grateful

@krzysz00

Copy link
Copy Markdown
Contributor Author

@cary-rowen The modifier toggling will work for other Braille displays, but only the Focus has default commands defined for it at the moment.

Also, to restate the functions of the commands in a different way, pressing the "Virtually toggle [keys]" command will make the next letter/number/... the user types in act as if they'd been holding down the [keys] while pressing it on the keyboard. The reason I said toggle is that if you press the command twice, it'll turn off the [keys] and make the letter/number/... act normally.

@cary-rowen

Copy link
Copy Markdown
Contributor

Hi @krzysz00

Thanks for your reply, I've discovered the use of this feature on Seika Notetaker before this.
I shouldn't hide comments, maybe someone else needs it too.

best
Cary

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.

Commands with modifier keys don't work with Focus Blue braille displays

10 participants