route review cursor and system caret#15044
Conversation
LeonarddeR
left a comment
There was a problem hiding this comment.
While this looks interesting, I think the approach needs debate.
- I'd personally prefer to avoid introducing a new toggle as well as expanding the routing script like this. It is probably better to expand the routing behavior in the braille module instead.
- Have you considered making the routing script support multiple key presses? You can route at first press, set focus ad second press, move system caret at third press or something similar.
- Rather than copying most of the behavior of script_navigatorObject_moveFocus, please consider storing most of it logic in one or more helper methods and call these methods from both scripts to avoid duplicating code
| @script( | ||
| # Translators: Input help mode message for cycle through | ||
| # braille route review cursor and system caret command. | ||
| description=_("Cycle through the braille route review cursor and system caret states"), |
There was a problem hiding this comment.
I think this needs rewording, though I'm not yet sure how.
| _( | ||
| # Translators: There is no focusable object e.g. cannot use | ||
| # tab and shift tab to move to controls. | ||
| "No focus" |
There was a problem hiding this comment.
I don't think cursor routing should have spoken feedback like this.
| # ensure the navigatorObject does not contain secure information | ||
| # before setting focus to this object | ||
| if objectBelowLockScreenAndWindowsIsLocked(navigatorObject): | ||
| ui.message(gui.blockAction.Context.WINDOWS_LOCKED.translatedMessage) |
There was a problem hiding this comment.
Avoid messages in this script.
| # ensure the review object does not contain secure information | ||
| # before displaying this object in braille line | ||
| if objectBelowLockScreenAndWindowsIsLocked(review.obj): | ||
| ui.reviewMessage(gui.blockAction.Context.WINDOWS_LOCKED.translatedMessage) |
There was a problem hiding this comment.
Avoid messages in this script
| # of the review cursor but there is no caret. | ||
| ui.message( | ||
| _( | ||
| "No caret" |
There was a problem hiding this comment.
Avoid messages in this script
|
Thank you @LeonarddeR for your comments. I selected new setting and gesture because I would like that user can decide how he/she likes that review and routing work. I suppose that user wants to do the task with one press. If he/she wants to review with display without affecting system caret and route caret to current position if needed, he/she wants this with one press. This has been general behavior in screenreaders/braille display software for decades. I agree with you that same code should be in one place. However maybe modifying of script_navigatorObject_moveFocus could be separate issue. I'm unsure why messages should be avoided in this script although they are presented in braille as well. Hopefully fixed "tethering automatically" problem. Also removed duplicate "braille.handler.routeTo(gesture.routingIndex)" line and changed doc strings to generic comments. |
See test results for failed build of commit a151f2ac3b |
|
@burmancomp I saw some discussion took place in issue #14885 where @michaelDCurran provided three possible pathways to implement this, suggesting the second. It looks like you went for the first. |
|
@LeonarddeR I combined options 1 and 2. Option 2 would require extra step from user to get NVDA to follow review cursor. Putting options 1 and 2 together user can select if he/she wants to use automatic tether and take extra step or use tether to review, set "Route review cursor and system caret" to enabled and avoid that extra step. When reviewing text without affecting system caret and then routing caret to desired position has been in braille display softwares/screenreaders for decades. I think they have worked so that no extra step to route are needed. What is the reason that there should be extra step? Why users who like to review text without affecting caret and then want to move caret should take that step? Approach I'm using has no effect on user experience unless user wants it has. He/she can continue using braille as before but if he/she wants do things somewhat differently it would be possible. Selecting option 2 - if I understand it correctly - would mean that decision what users want/need would have been made for behalf of them but combining option 1 and 2 gives user possibility to decide. |
|
I am not an expert in Braille nor a native English speaker, but I am wondering if there could not be a better phrasing for this option. The option is called: "routeReviewCursorAndSystemCaret" and it can take the values "Enabled" and "Disabled".
Maybe a wording such as "Route system caret along with review cursor"? Cc @XLTechie for your usual good English rewordings. |
fc33be0 to
b9ad4e1
Compare
I came at |
See test results for failed build of commit e5f4e50f6f |
|
@CyrilleB79 I am also not a braille user, and am not certain I understand this
feature well enough. Here is a first attempt.
"When moving the review cursor with routing buttons, also move the system
caret"?
Is that too long? Is it even true? :) Which came first? I am assuming review
cursor, meaning that disable would only move that.
|
|
I changed it to "Route system caret along with review cursor" as @LeonarddeR suggested. |
|
Converting this to a draft until #15064 converges towards a solution, so we can decide on approach |
Appropriate and acceptable solution in this situation. I am confident that @LeonarddeR gets #15064 ready soon and then this can be closed. My only little concern is focusing objects. If user can focus objects when using speech output and keyboard, there should be same possibility with braille output and braille display buttons. Different users do/want to do things differently, and this should be taken into account. screenreader should give possibilities and leave decision to user. |
|
This pull request is not needed anymore because #15064 is implementing all requested things. |
Fixes #14885 Fixes #3166 Summary of the issue: It has not been possible to move system focus/caret with single routing button press when braille is tethered to review, or when braille is tethered to automatically and follows temporarily review cursor. When user wants to review editable text without affecting system caret position (braille tethered to review continuously or temporarily), it maybe useful if he/she can route caret when needed for example to correct typos. Although thiskinds of routing has been possible, it has required at least one extra routing button press. Description of user facing changes A new setting Move system caret when Routing review cursor is added: Never (Default): No change Only when tethered automatically: Routing keys will move the caret or focus when tethered automatically. When explicitly tethered to review, routing keys won't move the caret Always: Regardless whether tethered to review or automatically, routing the review cursor will move the caret or focus Description of development approach This pr takes a different approach to #15044. The logic is not in the routing scripts in globalCommands but instead on the braille.ReviewTextInfoRegion class as well as two new region types for review cursor and objects. First of all, TextInfoRegion.routeTo has been split into two methods. This ensures that we can override the routing behavior for the review cursor regions without touching the BrailleInput logic. Furthermore, the following changes were added/changed: ReviewNVDAObjectRegion: simply sets focus to the object before executing the default action ReviewCursorManagerRegion. Inherrits from ReviewTextInfoRegion and CursorManagerRegion to ensure that ReviewTextInfoRegion._routeToTextInfo calls the right super class on cursor managers ReviewTextInfoRegion: implemented _routeToTextInfo to support the new behavior. It always calls the super class to execute the normal routing action (i.e. moving the review cursor and activating the position). When routing should move the system caret, it calls setFocus on an object when the underlying TextInfo class is DisplayModel, i.e. when in screen review. Otherwise, it calls setCursor on the super class, which translates to moving the system or cursor manager caret.
Link to issue number:
14885
Summary of the issue:
It has not been possible to move system focus/caret with single routing button press when braille is tethered to review, or when braille is tethered to automatically and follows temporarily review cursor. When user wants to review editable text without affecting system caret position (braille tethered to review continuously or temporarily), it maybe useful if he/she can route caret when needed for example to correct typos. Although thiskinds of routing has been possible, it has required at least one extra routing button press.
Description of user facing changes
There is new setting "Route review cursor and system caret" in braille category. Setting is disabled by default. User can also add input gesture. If braille is tethered to focus, setting is hidden and gesture reports itself as unavailable.
If setting is enabled, and braille is tethered to review, or to automatically and braille follows temporarily review, routing buttons try to move system focus/caret.
Description of development approach
This is modification from navigatorObject_moveFocus script so results should be similar.
Testing strategy:
Tested using Albatross 80 model.
Known issues with pull request:
none
Change log entries:
New features
It is possible to move system caret with routing buttons especially in edit areas when braille is tethered to review.
Changes
Bug fixes
For Developers
Code Review Checklist: