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

Add braille viewer braille routing #11804

Merged
merged 29 commits into from Nov 18, 2020
Merged

Conversation

feerrenrut
Copy link
Contributor

I initially created a prototype of this while fixing a braille issue a while ago, I have since refined this further in my own time.

Link to issue number:

None

Summary of the issue:

Sighted developers without a Braille display are unable to test features related to braille routing.

Description of how this pull request fixes the issue:

Adds braille routing to the braille viewer.

Pressing buttons on the braille viewer window would not work, since this would change to focus and routing would no longer have the intended effect. A mechanism had to be devised that allows the user to interact with the virtual braille cells without changing focus. The solution presented in this PR is to allow the user to hover the mouse over a braille cell for a short time. Feedback is given via the background color of the cell. The cell background immediately changes to a new color (yellow) to indicate the timer has started, then transitions gradually to orange, then snaps to green when routing is activated.

Testing performed:

Tested with notepad, web browsers and the NVDA GUI.

Known issues with pull request:

  • This feature is not very useful for users without some vision.
  • The colors (chosen to be somewhat reminiscent of a traffic light system) may not be appropriate for all users.

Change log entry:

In New features section:
- Braille routing is now supported with the Braille Viewer, hover to route to a braille cell

@feerrenrut feerrenrut added component/braille audience/nvda-dev PR or issue is relevant to NVDA / Add-on developers audience/a11y-tester Accessibility Testers labels Nov 2, 2020
@AppVeyorBot

This comment has been minimized.

@LeonarddeR
Copy link
Collaborator

Pressing buttons on the braille viewer window would not work, since this would change to focus and routing would no longer have the intended effect.

Mmm. Is it really impossible to avoid the window taking focus when clicking? Have you done any research into workarounds for this

@feerrenrut
Copy link
Contributor Author

Mmm. Is it really impossible to avoid the window taking focus when clicking? Have you done any research into workarounds for this

In short I'm not sure, I suspect there are ways that we could do that but I don't think it would be better. The braille cells are small compared to a regular button, and making them as large as a button would look really bad. Because they are small, it is handy to have some visual feedback for the cell that will be activated, eg highlighting the background. We could keep the background highlight and look into requiring the user to click, and then reject focus, but this gets complicated because we want the window to be focusable in other situations. Then after a click users expect some feedback that the action was performed (hence the final green color of the background), if the window rejects focus this is another thing to consider. We could use the same background color trick. It's hard to say whether it would actually be more intuitive.

Copy link
Contributor

@lukaszgo1 lukaszgo1 left a comment

Choose a reason for hiding this comment

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

I agree with @LeonarddeR that it'd be great to make this work for blind users somehow.

shouldHoverRouteToCell = boolean(default=false)
secondsOfHoverToActivate = float(min=0.0, default=1.0)
# Devices with 40 cells are quite common.
defaultCellCount = integer(min=20, max=160, default=40)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some displays smaller than 20 cells. Not enforcing the lower limit would make the implementation more flexible IMO.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. There are displays with 12 cells. And even some prototype displays in a rolling wheel shape with only 1 cell. So I think we should ensure this works for smaller than 20.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, this config is not exposed as a user setting by this PR. I think these considerations should be made when that happens, these values were just chosen as conservative values (to keep testing to minimum, and common utility high).

I'd prefer to follow this PR up with with one to make this configurable, and look more closely at the range that we should support. However, if a physical braille display is connected the braille viewer will adjust to match the number of cells

Realistically, the braille viewer would have very limited utility with only one cell. One cell relies on the ease of scrolling that comes with rolling the wheel. Further I'm not sure I understand the use case of trying to mimic this with the braille viewer.

michaelDCurran
michaelDCurran previously approved these changes Nov 16, 2020
Copy link
Member

@michaelDCurran michaelDCurran left a comment

Choose a reason for hiding this comment

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

A part from changing the minimum, I'm good with all of this.

shouldHoverRouteToCell = boolean(default=false)
secondsOfHoverToActivate = float(min=0.0, default=1.0)
# Devices with 40 cells are quite common.
defaultCellCount = integer(min=20, max=160, default=40)
Copy link
Member

Choose a reason for hiding this comment

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

I agree. There are displays with 12 cells. And even some prototype displays in a rolling wheel shape with only 1 cell. So I think we should ensure this works for smaller than 20.

@feerrenrut
Copy link
Contributor Author

it'd be great to make this work for blind users somehow.

@lukaszgo1 I'm not sure that is what @LeonarddeR was requesting. Though this may have been the first inquiry towards this goal.

Making this tool more accessible may be possible, but I fear it wouldn't be very intuitive. First it would be good to understand the particular use cases. So far I the main use case I have had in mind for this tool are sighted developers who are not good at reading braille, and do not have a braille device, allowing them to test the braille output of NVDA.

@feerrenrut feerrenrut merged commit 376edfc into master Nov 18, 2020
@feerrenrut feerrenrut deleted the addBrailleViewer-brailleRouting branch November 18, 2020 06:12
@nvaccessAuto nvaccessAuto added this to the 2020.4 milestone Nov 18, 2020
@feerrenrut
Copy link
Contributor Author

I had a chat with @michaelDCurran about this. We agreed that the cell count could be looked at subsequently if it is important for a user to be able to set.

@lijianwei2019
Copy link

lijianwei2019 commented Nov 18, 2020

@feerrenrut alpha-21371
The braille viewer is not working properly. nvda will restart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audience/a11y-tester Accessibility Testers audience/nvda-dev PR or issue is relevant to NVDA / Add-on developers component/braille
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants