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

Disable "automatically set focus to focusable elements" by default #11190

Merged
merged 6 commits into from Jun 25, 2020

Conversation

codeofdusk
Copy link
Contributor

@codeofdusk codeofdusk commented May 24, 2020

Link to issue number:

None.

Summary of the issue:

Especially after #11130, it may be worth considering disabling "automatically set system focus to focusable elements" by default.

Description of how this pull request fixes the issue:

  • Disables the option by default in the config spec.
  • Moves the checkbox to the browse mode panel (with the other virtual buffers options).
  • Updates the user guide.

Testing performed:

Tested that toggling the checkbox works as intended, and that NVDA+8 still works.

Known issues with pull request:

None.

Change log entry:

=== Changes ===

@codeofdusk
Copy link
Contributor Author

codeofdusk commented May 24, 2020

I've been using fast browse mode on my system for a while since #11130 was merged, but leaving as a draft until I know it's stable enough/there are no known major issues.

Cc @jcsteh, @leonardder, @mltony, @michaelDCurran.

@Adriani90
Copy link
Collaborator

Adriani90 commented May 24, 2020

At least from an user perspective I can tell that it's working very good now, so it could be enabled by default. This needs to be documented properly in the changes.t2t, in the userguide and maybe also in the developers guide (i.e. shift+tab will still report the element with system focus, switching to focus mode and pressing tab shows a different behavior than pressing tab in browse mode because system focus will move from its last position which is different from the virtual cursor, etc.). A good documentation is very crucial here because otherwise we will get many bug reports by accessibility testers regarding NVDA not scrolling the screen, pressing tab not doing what is expected to do etc.
And elieve me, to provide the proper information by commenting on every bug and saying this is expected behavior can cost alot of time and can be quite frustrating after some time.

@jcsteh
Copy link
Contributor

jcsteh commented May 24, 2020

@codeofdusk
Copy link
Contributor Author

codeofdusk commented Jun 14, 2020

PR no longer blocked, since #11206 was merged.

source/config/configSpec.py Outdated Show resolved Hide resolved
@feerrenrut
Copy link
Member

feerrenrut commented Jun 16, 2020

I'd like this to have a longer run in alpha, I'll look to review / merge this after branching beta.

@feerrenrut feerrenrut added this to the 2020.3 milestone Jun 16, 2020
@codeofdusk
Copy link
Contributor Author

codeofdusk commented Jun 18, 2020

Now that master has moved onto 2020.3, I think this is worth merging. This should be well tested for as long as possible.

@feerrenrut feerrenrut self-requested a review Jun 18, 2020
Copy link
Member

@feerrenrut feerrenrut left a comment

Happy to merge this, one minor issue that I missed previously.

source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>
Copy link
Member

@feerrenrut feerrenrut left a comment

Thanks @codeofdusk

@feerrenrut feerrenrut merged commit 675c4c5 into nvaccess:master Jun 25, 2020
1 check passed
@Adriani90
Copy link
Collaborator

Adriani90 commented Jun 25, 2020

@feerrenrut, @jcsteh, now that this has become the Default behavior, what should we do now with issues like #2039, #5251, #5831, #6282, #6678 or #8999?

#2039 and #8999 describe similar behavior, however I let both open because both contain valuable Input.
Suggestions are saying that this is related to bad web authoring, but it could also be a Change in Rendering time between NVDA 2018.3 and 2018.4 which led to this behavior of Focus jumping.
Having in mind that this PR solves the issues referenced in this comment, is there a reason to Keep them open still? Will there be any Investigation to further improve this even with automatic set Focus to focusable Elements turned on?

@Adriani90
Copy link
Collaborator

Adriani90 commented Jun 25, 2020

Ah there are also issues #8079 and #8010. These would also be fixed when using the default behavior provided here, but not when the focus is set automatically to focusable elements.

@codeofdusk
Copy link
Contributor Author

codeofdusk commented Jun 25, 2020

Since fast browse mode is now the default and users are warned that disabling it may have performance and stability implications, I see no need to fix these issues when fast browse mode is off.

@jage9
Copy link
Contributor

jage9 commented Jun 25, 2020

One of the main reasons for keeping this on for me was to use the mouse routing command to click on elements that could not be accessed otherwise. How will the new browse mode effect this? Should there be/is there a command to temporarily move focus, or is it a part of the move mouse to navigator object command? I turned the new option off since this is the recommended behavior, so will report any results.

@Adriani90
Copy link
Collaborator

Adriani90 commented Jun 25, 2020

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Jun 25, 2020

Having this enabled by default is quite problematic for people who have disabled "caret moves review cursor" as there is no reliable way to move navigator object to the position of the browse mode caret. Being able to move it there is critical as we can only route mouse to the navigator not to the caret. I've created #9622 some time ago and I believe that if it would be fixed this situation would dramatically improve. @feerrenrut I understand that we are in the features freeze and #9622 can be technically classified as a feature request / enhancement but without it the new behavior is quite problematic for some people as explained above. Would you accept a pr fixing it i.e moving navigator object to focus moves navigator to the position of the browse mode caret?

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Jul 6, 2020

@feerrenrut Have you seen this commend of mine? Could you please share your opinion? In the current state people who are more comfortable working with review cursor not following caret are forced to enable "set focus to focusable elements" to be able to work effectively on webpages requiring mouse/object navigation.

@feerrenrut
Copy link
Member

feerrenrut commented Jul 6, 2020

@lukaszgo1 Could you please create a new issue to explain this issue fully? Or are you saying that #9622 already covers the situation? If so, please update #9622 to explain how this PR fits in.

Would you accept a pr fixing it i.e moving navigator object to focus moves navigator to the position of the browse mode caret?

I'm not really sure that I understand this.

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Jul 6, 2020

I hope #9622 after my recent edit clears things up

@Adriani90
Copy link
Collaborator

Adriani90 commented Aug 10, 2020

since #9622 has been fixed, I think there is no other issue open on this. Given that the screen scrolls automatically and the focus is moved to the virtual cursor when swithicng to focus mode automatically or with nvda+space bar, I think it makes sense to close those issues dealing with focus jumping etc. I will proceed with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants