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

Allow disabling automatic focus of focusable elements in browse mode #9511

Merged
merged 47 commits into from May 15, 2019

Conversation

Projects
None yet
6 participants
@leonardder
Copy link
Collaborator

commented Apr 22, 2019

Link to issue number:

Follow up of #9114 and #9360. Initial work by @mltony

Summary of the issue:

From #9114 (comment)

NVDA has systematic problems with browse mode in all browsers, such as:

  • Rubber band effect: NVDA sporadically undoes the last browse mode keystroke by jumping to the previous location
  • Edit boxes steal system focus when arrowing down through them on some websites
  • Browse mode keystrokes are slow to respond.

A pull request was provided by @mltony to fix this, but unfortunately had to be reverted. Another pull request (#9360) had also be closed.

Description of how this pull request fixes the issue:

This pr takes the work by @mltony, stripping it down to the absolute basics. Changes since #9360 are.

  1. Reworded the new option in browse mode settings. Honestly, I considered the wording "focus follows browse mode" a bit vague, given that the focus is usually updated explicitly. It is also not very clear from the initial wording when the focus is updated and when it is not. I reworded this to "Automatically set system focus to focusable elements" which in my opinion more closely resembles what's happening under the hood. As always, this change is open to debate and I"m happy to revert it.

  2. #9360. contained an issue that caused state changes to be reported inconsistently. I'm pretty sure this is fixed now.

  3. In comparison with #9114, this removes all the asynchronous logic. This is actually not exactly true, since the whole work still relies on async logic (i.e. calling setFocus and than handling focus changes in event_gainFocus). However, setting the focus to objects is no longer queued asynchronously. I also removed the uniqueID property from the several NVDAObjects, since it's no longer used.

  4. I added shift+applications and shift+f10 as additional gestures that should cause the focus to be set to the last focusable object.

  5. Reworded some additional method names, such as changing maybeSyncFocus to focusLastFocusableObject. To avoid confusion, focusLastFocusableObject is now explicitly delegated to this new functionality, i.e. only supposed to be used when automatic focus of focusable elements is off.

Testing performed:

Tested on several websites, including examples as provied by @mltony and others. This is still a draft pr due to the error sensitivity of this code.

Known issues with pull request:

  1. When opening the following from an address bar:
    data:text/html, <details><summary>Collapse or expand this</summary>Some text</details>
    and pressing the button when unfocused, NVDA reports unknown and then returns back to the browse mode document.

  2. When enabling pass through with NVDA+Space, NVDA announces the last focusable object as it now gains focus.

Change log entry:

t.b.d.

mltony added some commits Dec 31, 2018

@lukaszgo1

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

What about the bug described in this commend? Is it fixed with the current approach?

@michaelDCurran michaelDCurran marked this pull request as ready for review May 15, 2019

@michaelDCurran michaelDCurran merged commit 7ffc65f into nvaccess:master May 15, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2019.2 milestone May 15, 2019

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

commented May 16, 2019

@mltony: Congrats on having this merged. We really couldn't have done this without you starting it.
Note that, after discussion with @feerrenrut and @michaelDCurran, we decided to store the gui option in the advanced settings for now.

Having said that, I'm pretty happy with the feature as is. The only thing that pisses me of is the regular unknown announcements. I really want to know how to avoid these.

@bramd

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Nice feature, glad it has been merged.

I have issues activating dialogs of my Lastpass extension in Chrome when this experimental option is used. The extension injects a frame with a dialog at the end of the DOM for example when a new account can be saved or a password change is detected. I didn't find an easy reproduceable minimal test case for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.