Add an option to prevent speech being interrupted while scrolling the braille display by bramd · Pull Request #13131 · nvaccess/nvda

bramd · 2021-12-04T12:28:42Z

Link to issue number:

Summary of the issue:

Some people prefer to be able to read in braille while still listening to the speech output for the current line/object. This is currently not possible, because every braille gesture will interrupt speech.

Description of user facing changes

Adds a new setting: "Interrupt speech while scrolling", default is "enabled".
The new setting determines if speech should be interrupted when the Braille display is scrolled backwards/forwards.
Previous/next line commands are not affected.

This is added as an option because some speech+braille users prefer a bimodal interaction pattern.

For these users, speech may be distracting while reading Braille.
These users repetitively swap between primarily focusing on speech or braille.
For these users, the current and expected UX for cancelling speech when reading braille is to scroll the line of braille.
Considering alternatives:
- Most braille devices cannot detect fingers reading the display.
- Using an additional braille gesture just to cancel speech will slow users down / isn't a good use of a button on a display.

To preserve the existing behavior, the option is enabled by default, interrupting speech when scrolling.

Disabling this option allows speech to be heard while the user is simultaneously reading Braille.
Users who don't find speech distracting with Braille, or rely on both Braille and speech, may prefer to disable the option.

Description of how this pull request fixes the issue:

Add an option to interrupt speech while the display is being scrolled.
This option is enabled by default, so the previous behavior is preserved.
This implementation only works for scroll previous/next display and does not affect other gestures such as previous/next line.

Testing strategy:

Tested using a Handy Tech braille display by turning this setting off/on and checking if the behavior was as expected. No unit tests or system tests have been added.

Known issues with pull request:

This can't be tested without a braille display: blocked by #13917

Change log entries:

Refer to diff

Code Review Checklist:

Pull Request description:
- description is up to date
- change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- 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

…lling the braille display. This is enabled by default to preserve the old behavior.

bramd · 2021-12-04T12:55:38Z

It seems I'm unable to assign any additional reviewers to this. I consider this ready for review, but would like to hear what @LeonarddeR, @dkager and @AAClause think about this since they have been commenting on the issue.

LeonarddeR · 2021-12-04T13:14:03Z

I wonder, would you mind implementing a solution that allows overriding the speech effect on the script rather than on the gesture? I find it slightly ugly that this has to be done on the gesture level, though it is currently by design. However, the current implementation isn't going to work with the SuperBraille braille display, for example.

dkager · 2021-12-04T13:35:33Z

While I agree with Leonard, I must also say this patch is elegantly compact. Though matching on script name is perhaps not as robust as introducing a variable to determine whether it is a scrolling command.

bramd · 2021-12-07T20:28:33Z

Adding an isScrollingCommand or similar property to the scripts is a possibility for sure. However, I'm not sure if it would be correct to put the speech command as a side effect in the scripts.

The Superbraille is a bit of an edge case I didn't consider. That being said, I think this option should work regardless of what the source of the gesture is. That raises the question where to put this behavior. I don't really like adding side effects to scripts (see above), so that would mean we implement this in the base class of all gestures.

LeonarddeR · 2021-12-08T06:31:43Z

I don't really like adding side effects to scripts

Shouldn't wat the gesture does be determined by the script, and shouldn't the script determine what to do with speech?

Qchristensen

That last line in the userGuide.t2t change could be a little ambiguous - maybe split into two sentences?

seanbudd · 2022-02-21T05:45:17Z

Sorry @bramd - we need a chance to suggest an alternative implementation.

seanbudd · 2022-06-21T04:40:56Z

I've pushed some changes to add Tags to the scriptHandler API. This new approach still needs testing.

seanbudd · 2022-06-21T04:46:09Z

Would anybody be able to test the code?

Requested @feerrenrut review for the API change I have introduced to scriptHandler

Qchristensen

User Guide reads well, good work.

seanbudd · 2022-07-14T05:12:39Z

I'm going to also add notes on this in the user guide.

seanbudd · 2022-07-15T01:59:34Z

@feerrenrut - can you review the user guide changes and update to the PR description?

feerrenrut

Can you update the title of the PR to be relative to the added behavior.
The pre-existing behavior is to interrupt, so perhaps: No speech interrupt for scroll braille command

The following seems ambiguous to me:

Previous/next line commands are not affected.

Perhaps this means the new user preference does not change the behavior of these commands, or that these commands don't interrupt speech in the first place.

feerrenrut · 2022-07-15T10:57:13Z

+This setting determines if speech should be interrupted when the Braille display is scrolled backwards/forwards.
+Previous/next line commands are not affected.
+
+Speech may be found distracting while reading Braille.
+As such, this option is enabled by default, interrupting speech when scrolling.
+
+Disabling this option allows speech to be read along with Braille.
+


A general question:

Is "interrupting" a clear enough term for user?

Are there any/few/many prior usages of "Interrupt" is there with regards to speech?

Would "stopping/stopped/cancelled" be clearer?

Suggested change

This setting determines if speech should be interrupted when the Braille display is scrolled backwards/forwards.

Previous/next line commands are not affected.

Speech may be found distracting while reading Braille.

As such, this option is enabled by default, interrupting speech when scrolling.

Disabling this option allows speech to be read along with Braille.

This setting determines if speech should be interrupted when the Braille display is scrolled backwards/forwards.

Previous/next line commands always interrupt speech.

On-going speech might be a distraction while reading Braille.

For this reason the option is enabled by default, interrupting (stopping) speech when scrolling braille.

Disabling this option allows speech to be heard while simultaneously reading Braille.

For other example of the word "interrupt" with speech, see the following options:

Speech interrupt for typed characters

Speech interrupt for Enter key

feerrenrut · 2022-07-15T10:57:58Z

I also made some edits directly to the description. Please check the history.

seanbudd · 2022-07-18T03:30:24Z

@feerrenrut - The edits to the description appear fine, I have also pushed changes to the user guide.

Qchristensen

User guide change reads well.

AppVeyorBot · 2022-07-18T04:39:56Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/le7tymxvvcj9a6xv/artifacts/output/nvda_snapshot_pr13131-25893,2185a6f7.exe

See test results for failed build of commit 2185a6f716

AppVeyorBot · 2022-07-18T06:36:37Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ukatkotvkxjhodgk/artifacts/output/nvda_snapshot_pr13131-25899,2185a6f7.exe

See test results for failed build of commit 2185a6f716

…display (#13932) Link to issue number: Fixup of #13131 Summary of the issue: #13131 incorrectly checks the script commands. The initial PR did not use the correct instance of global commands, rather references to the class methods. Description of user facing changes #13131 works as intended.

bramd added 2 commits December 4, 2021 13:13

Add an option to determine if speech should be interrupted while scro…

90979d3

…lling the braille display. This is enabled by default to preserve the old behavior.

Fix linter errors

c4c0c4c

bramd requested review from a team as code owners December 4, 2021 12:28

bramd requested review from Qchristensen and seanbudd December 4, 2021 12:28

bramd marked this pull request as draft December 4, 2021 12:35

bramd marked this pull request as ready for review December 4, 2021 12:53

seanbudd reviewed Jan 13, 2022

View reviewed changes

Comment thread source/braille.py Outdated

Comment thread source/braille.py Outdated

Qchristensen requested changes Jan 27, 2022

View reviewed changes

Comment thread user_docs/en/userGuide.t2t Outdated

seanbudd marked this pull request as draft February 21, 2022 05:37

seanbudd marked this pull request as ready for review February 21, 2022 05:44

feerrenrut added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 18, 2022

seanbudd added the blocked/needs-code-review label Jun 17, 2022

seanbudd added 3 commits June 21, 2022 14:34

Merge branch 'master' into i2124

077f196

add tags to script api

2e894fe

address review comments

3410e8e

seanbudd requested review from Qchristensen and feerrenrut June 21, 2022 04:45

This comment was marked as off-topic.

Sign in to view

seanbudd self-assigned this Jun 22, 2022

seanbudd added the merge-early Merge Early in a developer cycle label Jun 28, 2022

Qchristensen approved these changes Jul 14, 2022

View reviewed changes

Explain usecases in user guide

068a978

seanbudd requested review from Qchristensen and feerrenrut July 15, 2022 01:54

Explain usecases in user guide

6910934

feerrenrut reviewed Jul 15, 2022

View reviewed changes

seanbudd removed the blocked/needs-code-review label Jul 18, 2022

update user guide further

46274e4

seanbudd mentioned this pull request Jul 18, 2022

Start a style guide for the User Guide #13916

Merged

5 tasks

seanbudd changed the title ~~Add an option to determine if speech should be interrupted while scrolling the braille display~~ Add an option to prevent speech being interrupted while scrolling the braille display Jul 18, 2022

seanbudd requested a review from feerrenrut July 18, 2022 03:31

Qchristensen approved these changes Jul 18, 2022

View reviewed changes

seanbudd added the blocked/needs-testing label Jul 18, 2022

feerrenrut approved these changes Jul 18, 2022

View reviewed changes

seanbudd removed the merge-early Merge Early in a developer cycle label Jul 18, 2022

seanbudd added 4 commits July 18, 2022 17:45

Merge branch 'master' into i2124

0bcbc98

Merge branch 'master' into i2124

a97ee84

fix changes

e6e444b

add options to definition

159bdd9

seanbudd merged commit 8205206 into nvaccess:master Jul 19, 2022

nvaccessAuto added this to the 2022.3 milestone Jul 19, 2022

seanbudd mentioned this pull request Jul 21, 2022

Fixup #13131, ensure speech doesn't interrupt when scrolling braille display #13932

Merged

5 tasks

seanbudd mentioned this pull request Aug 5, 2022

Add Braille device button emulation to Braille viewer #13917

Open

Uh oh!

Conversation

bramd commented Dec 4, 2021 • edited by seanbudd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

bramd commented Dec 4, 2021

Uh oh!

LeonarddeR commented Dec 4, 2021

Uh oh!

dkager commented Dec 4, 2021

Uh oh!

bramd commented Dec 7, 2021

Uh oh!

LeonarddeR commented Dec 8, 2021

Uh oh!

Uh oh!

Uh oh!

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seanbudd commented Feb 21, 2022

Uh oh!

seanbudd commented Jun 21, 2022

Uh oh!

seanbudd commented Jun 21, 2022

Uh oh!

This comment was marked as off-topic.

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

seanbudd commented Jul 14, 2022

Uh oh!

seanbudd commented Jul 15, 2022

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

feerrenrut Jul 15, 2022 • edited by seanbudd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrilleB79 Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

feerrenrut commented Jul 15, 2022

Uh oh!

seanbudd commented Jul 18, 2022

Uh oh!

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Jul 18, 2022

Uh oh!

AppVeyorBot commented Jul 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

bramd commented Dec 4, 2021 •

edited by seanbudd

Loading

feerrenrut Jul 15, 2022 •

edited by seanbudd

Loading