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

Automatically switch braille tethering between review and focus #2385

Closed
nvaccessAuto opened this issue May 25, 2012 · 20 comments · Fixed by #7489
Closed

Automatically switch braille tethering between review and focus #2385

nvaccessAuto opened this issue May 25, 2012 · 20 comments · Fixed by #7489
Assignees
Labels
component/braille enhancement p4 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority
Milestone

Comments

@nvaccessAuto
Copy link

Reported by aliminator on 2012-05-25 12:32
When a braille display is tethered to focus and object navigation is used it does not refresh properly.
E.g. Notepad is used with some text entered. The text still shows up when navigating to another application using object navigation.
Nevertheless, this works as expected when the braille display is tethered to review.

@nvaccessAuto
Copy link
Author

Comment 1 by jteh on 2012-05-26 04:50
This is intended behaviour. Focus refers to the focus object and system caret. Review refers to the object navigator and text review within it. If you want to follow the object navigator and review cursor, you need to tether to review.
Changes:
Added labels: worksforme
State: closed

@nvaccessAuto
Copy link
Author

Comment 2 by aliminator (in reply to comment 1) on 2012-05-29 10:04
Replying to jteh:
Replying to jteh:
Allowing object navigation in both modes (tethered to focus and review) should be possible due to the fact that there are users using braille output /navigation only. For those speech does not play an important role.
However, currently there is no way to navigate and trigger actions using the braille display only.
When the display is tethered to focus it shows the same text when using object navigation.
When the display is tethered to review navigating objects is fine but no action can be executed because routing keys are not supported in that way.
Hence, the principle described in comment:1 should be modified accordingly.

@nvaccessAuto
Copy link
Author

Comment 3 by jteh (in reply to comment 2) on 2012-05-30 00:35
Replying to aliminator:

Allowing object navigation in both modes (tethered to focus and review) should be possible

This simply doesn't make sense. You can't be tethered to both focus and review at the same time. Otherwise, NVDA can't know which cursor you want it to show. They are very separate cursors, which is why they have separate commands.

due to the fact that there are users using braille output /navigation only. For those speech does not play an important role.

Whether a user uses speech or not isn't relevant here. The experience is the same for speech users. They still have two separate cursors.

However, currently there is no way to navigate and trigger actions using the braille display only.

True, but braille users can use a keyboard just as well as speech users can. Perhaps this calls for more braille display key bindings. This needs to be done separately for each display and users of those displays need to make suggestions about this.

When the display is tethered to review navigating objects is fine but no action can be executed because routing keys are not supported in that way.

This is true for speech users as well. Speech users have to activate the object using NVDA+numpadEnter or route the focus to the object using NVDA+shift+numpadMinus.

@nvaccessAuto
Copy link
Author

Comment 4 by jteh on 2012-05-30 00:37
Perhaps you're asking for an option whereby braille can automatically tether to review when object navigation commands are used. However, if we do this, what will cause it to tether back to focus?

@nvaccessAuto
Copy link
Author

Comment 5 by aliminator (in reply to comment 4) on 2012-05-31 11:14
Replying to jteh:
The issue is more principal and basically is the inability to explore the screen using the braille display only. One has to switch between focus and review mode all the time.
E. g. when clicking on properties on a local drive one has to do the following to get the information using braille only:
1)

@nvaccessAuto
Copy link
Author

Comment 6 by jteh (in reply to comment 5) on 2012-06-07 08:30
Replying to aliminator:

The issue is more principal and basically is the inability to explore the screen using the braille display only. One has to switch between focus and review mode all the time.

If you want to explore the screen in places the system focus can't go, you use review. This is true for both speech and braille users. Thus, tethering ot review makes sense.

E. g. when clicking on properties on a local drive one has to do the following to get the information using braille only:

I think your example is missing. :)

@nvaccessAuto
Copy link
Author

Comment 7 by aliminator on 2012-06-13 15:27
O true, it is missing. But I would open up another ticket with that example because it does not fit here...

@nvaccessAuto
Copy link
Author

Comment 8 by jteh on 2012-06-15 09:56
I'm morphing this slightly to cover my thought in comment:4. However, we will have to very carefully think about when this should occur to avoid confusion and inconsistency.
Changes:
Changed title from "Braille display does not refresh when using object navigation" to "Automatically switch braille tethering between review and focus"
Removed labels: worksforme
State: reopened

@LeonarddeR
Copy link
Collaborator

@jcsteh commented:

Perhaps you're asking for an option whereby braille can automatically tether to review when object navigation commands are used. However, if we do this, what will cause it to tether back to focus?

I think it is perfectly valid to switch back to focus as soon as the focus is explicitly changed by the user, with pressing tab for example.

cc @dkager, @bramd, @josephsl

@derekriemer
Copy link
Collaborator

derekriemer commented May 11, 2017 via email

@LeonarddeR
Copy link
Collaborator

After looking at the code as well as some discussion with @jcsteh and @dkager, here are some rough implementation ideas:

  1. Ad a new boolean option to the config spec, autoTether
  2. Replace the getter and setter for braille.BrailleHandler.tether with getTether and setTether methods. This is because setTether needs an additional keyword argument to determine whether it was an automatic or manual tether action. Of course, keep the old tether auto property for backwards compatibility
  3. Always show ui.reviewMessage in braille when auto tethering is on
  4. Tether to review for several scripts in globalCommands (e.g. for next/previous/child/parent objects and moving by character,word,line for review
  5. Add a new shouldAutoTether keyword argument to braille.BrailleHandler.handleGainFocus and braille.BrailleHandler.handleCaretMove. This argument should default to True, and can be set to False in places in the code base where this is explicitly required. Of course, shouldAutoTether should be ignored if the auto tethering option is disabled globally.

@LeonarddeR
Copy link
Collaborator

Ok, this is causing major headaches. I've tried two things so far.

  1. The implementation as proposed above. Auto tethering to focus is done using Braillehandler.handleGainFocus and Braillehandler.handleReviewMove. Auto tethering to review is done using the several scripts in globalCommands. This has the drawback that, if you are auto tethered to review and you change focus, the review cursor changes before the focus is handled. In other words, the braille will first present the focus object in review mode and then auto tethers to focus.

  2. After some discussion with @dkager, we decided that using the scripts for this might not be the nicest way to do. To keep things in sync nicely, auto tethering shouhld be done by handleReviewMove instead. However, this has the following implications:

    A. handleReviewMove should not auto tether when the move is caused due to the review cursor following the focus or the caret. Since most of the review following focus is done using api.setNavigatorObject with the isFocus parameter set to True, api.setNavigatorObject should somehow communicate this to handleReviewMove. However, setNavigatorObject doesn't execute handleReviewMove itself, but executes the becomeNavigatorObject event, which executes handleReviewMove. The isFocus parameter gets lost in this process. This can be fixed by creating a caching variable, e.g. eventHandler.lastReviewMoveDueToFollowing. This new variable should also be set properly by api.setReviewPosition based on a new parameter isCaret. At least for setNavigatorObject, I'd rather pass this info as an event parameter, but that means we break backwards compatibility. Side note, this is why I belief add-ons should implement events like

    def event_becomeNavigatorObject(self, obj, **kwargs):
    

    B. When going for point A, there are some edge cases that need to be changed in setting the navigatorObject. For example, in the explorer appmodule on the SuggestionListItem class, the navigator object is set for event_UIA_elementSelected. This call of setNavigatorObject would need the extra isFocus parameter. Honestly, it turns out that this code isn't doing what it should do anyway, since event_UIA_elementSelected turns out to be a sort of focus event which shouldn't make the navigator object follow if reviewFollowFocus is off. CC @michaelDCurran

Whatever we do, there's some discussion required about the how and the why.

@LeonarddeR
Copy link
Collaborator

To make clear, I myself prefer the second approach. @dkager, you?

@dkager
Copy link
Collaborator

dkager commented Jul 13, 2017

Yes, I prefer option 2 even if it takes some refactoring.

@ruifontes
Copy link
Contributor

ruifontes commented Jul 13, 2017 via email

@jcsteh
Copy link
Contributor

jcsteh commented Aug 16, 2017

@leonardder commented on 13 Jul 2017, 16:18 GMT+10:

I'd rather pass this info as an event parameter, but that means we break backwards compatibility. Side note, this is why I belief add-ons should implement events like

 def event_becomeNavigatorObject(self, obj, **kwargs):

We could consider making eventHandler use the new extensionPoints.callWithSupportedKwargs function I added in #7484, which would allow us to pass additional kwargs without breaking backwards compat:

def callWithSupportedKwargs(func, *args, **kwargs):
	"""Call a function with only the keyword arguments it supports.
	For example, if myFunc is defined as:
		def myFunc(a=None, b=None):
	and you call:
		callWithSupportedKwargs(myFunc, a=1, b=2, c=3)
	Instead of raising a TypeError, myFunc will simply be called like this:
		myFunc(a=1, b=2)
	"""

@jcsteh jcsteh removed their assignment Aug 16, 2017
@LeonarddeR
Copy link
Collaborator

@jcsteh commented on 16 aug. 2017 13:49 CEST:

We could consider making eventHandler use the new extensionPoints.callWithSupportedKwargs function I added in #7484, which would allow us to pass additional kwargs without breaking backwards compat:

That would be awesome, I think. Does this fit in the scope of #7484?

@josephsl
Copy link
Collaborator

josephsl commented Aug 16, 2017 via email

@jcsteh
Copy link
Contributor

jcsteh commented Aug 17, 2017 via email

@LeonarddeR LeonarddeR self-assigned this Aug 17, 2017
@LeonarddeR
Copy link
Collaborator

There has been some discussion going in #7484 about using extensionPoints.callWithSupportedKwargs in eventHandler. This would be great in situations where we want to add an additional argument to a well established event, such as becomeNavigatorobject. However, there is a problem that some parts in the code call positional arguments as if they were kwargs, and vise versa. This is why I propose callWithSupportedKwargs as a fallback for cases where the event handler tries to call a function with too many arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/braille enhancement p4 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants