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

Remove 'Automatically set system focus to focusable elements' #17598

Merged
merged 4 commits into from
Jan 22, 2025

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Jan 8, 2025

Link to issue number:

Closes #16469
Closes #15463
Closes #15780

Summary of the issue:

The behaviour of "Automatically set system focus to focusable elements" is not reliable or well supported.
It is known to cause many bugs and unexpected behaviour in browse mode.
If it is to be reimplemented, clear user stories, well defined behaviour and a better implementation is required.

Description of user facing changes

The setting "Automatically set system focus to focusable elements" is removed from the settings panel and user guide.
The keyboard command to toggle it has been removed, freeing up nvda+8
The behaviour for NVDA now assumes the setting is off (the default).

Description of development approach

  • Replaced config.conf["virtualBuffers"]["autoFocusFocusableElements"] with False and then simplified the logic.
  • removed related code to the setting

Testing strategy:

Smoke testing of NVDA in browse mode

Known issues with pull request:

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@coderabbitai summary

@seanbudd seanbudd requested review from a team as code owners January 8, 2025 04:50
@brunoprietog
Copy link

Why remove this option and simply hide it? This will ruin the experience of add-ons like Object Location Tones. It is tremendously useful for me to focus the elements automatically for better perception. Please, I ask you to reconsider this. It is a terrible step backwards in the experience.

@brunoprietog
Copy link

Also pointed it in #15463. @ABuffEr, @cary-rowen, @lukaszgo1 and @XLTechie also agree to keep the setting and remove the command.

Are you sure this setting has no positive side at all when checked?

As far as I know, some add-ons, such as UnSpoken, rely on this option because they use the gainFocus event.

I agree with to possibly move it to advanced settings, not with to remove it at all. I cannot point to specific cases, but I'm sure to have used it towaydays from time to time, in poorly designed websites, where the alternative would be to move and move and move manually the focus or the mouse pointer.

Some poorly designed web pages may require users to enable this option as a temporary solution. It may be the fault of the web developer, but having a temporary solution is far better than doing nothing.

Finally, this option is off by default.

Removing this setting would be terrible for me. I use the Object Location Tones add-on to hear a sound every time I move through focusable controls. This really helps me to know where an object is on the screen quickly, and helps me to be more efficient when scrolling through the interface. It also helps me get a picture in my head of the layout of the interface, just as I can do when I'm using a touch device like my mobile phone. Isn't it already enough that it's disabled by default? There are only drawbacks to this

The fact that Object Location tones requires this option is caused by the deficiency in NVDA's core. I';d say this should be fixed before removing this option.

It seems that users who need this, know that they need it. As such, wouldn't a happy medium right now, be to remove the keyboard shortcut to prevent accidental activation by users who don't understand the ramifications, or who were reaching for something else?

Often when I read about problems with this option, it's from users who didn't even realize they had it turned on.

Since removing it would cause problems for a vocal minority, limiting its potential accidental use strikes me as a better first step, while we await some better synchronized scrolling to be implemented.

I also support moving it to advanced, but dropping the script is an easy first action.

@seanbudd
Copy link
Member Author

seanbudd commented Jan 9, 2025

@brunoprietog - thanks for re-raising this feedback.
We intend to remove this functionality, but want to ensure impact to add-ons are minimal.

Removing this setting would be terrible for me. I use the Object Location Tones add-on to hear a sound every time I move through focusable controls. This really helps me to know where an object is on the screen quickly, and helps me to be more efficient when scrolling through the interface. It also helps me get a picture in my head of the layout of the interface, just as I can do when I'm using a touch device like my mobile phone. Isn't it already enough that it's disabled by default? There are only drawbacks to this
As far as I know, some add-ons, such as UnSpoken, rely on this option because they use the gainFocus event.
The fact that Object Location tones requires this option is caused by the deficiency in NVDA's core. I';d say this should be fixed before removing this option.

Can these add-ons use the Browse mode cursor like NVDA highlighter? Would adding an extension point or other API endpoint allow them to still be conscious of the browse mode focus.

@brunoprietog
Copy link

I'm really not sure, maybe @josephsl has something to contribute here?

Anyway, this feature is still tremendously useful for dealing with some complicated websites. I also use it frequently when developing and diagnosing accessibility issues. It's very useful to know if an element is focusable but not tabbable, i.e. has tabindex=“-1”.

What would be the downside of moving this to advanced settings? This is already disabled by default and the keyboard shortcut would also be accidents free.

@seanbudd
Copy link
Member Author

seanbudd commented Jan 9, 2025

What would be the downside of moving this to advanced settings? This is already disabled by default and the keyboard shortcut would also be accidents free.

To put it simply, the feature is unsupported, buggy and no longer maintainable. Advanced settings is intended for supported behaviour, even if experimental, but is not a graveyard for features.

@seanbudd
Copy link
Member Author

seanbudd commented Jan 9, 2025

cc @masonasons author of UnSpoken. Can you please read the context of #17598 (comment) and let us know how we can best continue support for this add-on

@cary-rowen
Copy link
Contributor

cary-rowen commented Jan 9, 2025

cc @mltony
The Earcons and speech rules add-on already has the functionality of the unSpoken add-on, but I would still support adding an appropriate extension point for the previous use case.

@masonasons
Copy link

Hi!

As is currently the situation, yes having Automatically set system focus to focusable elements off using unspoken causes you to not hear where links and other page elements are using the earcons. I'm not entirely sure how extension points in NVDA work, I've not gotten around to figuring that out yet and have limited time, but so long as there would be a way to know when NVDA lands on a browse mode element like a link or such and where that is located on the screen, I imagine it would be possible for someone to implement that support into Unspoken.

@seanbudd
Copy link
Member Author

seanbudd commented Jan 9, 2025

@masonasons - I think post_browseModeMove should work for this

@masonasons
Copy link

Is hat documented anywhere? I'll have a look

@seanbudd
Copy link
Member Author

seanbudd commented Jan 9, 2025

@masonasons - it's not documented perfectly but it's mentioned in the developerGuide and has some associate comments in the source code

@brunoprietog
Copy link

cc @dbernaca

@ABuffEr
Copy link
Contributor

ABuffEr commented Jan 9, 2025

Hi,
recently, I enabled this option to show a web page correctly scrolled/focused during a working call.
So, is there an alternative way to get the same result?

@XLTechie
Copy link
Collaborator

XLTechie commented Jan 9, 2025 via email

@seanbudd
Copy link
Member Author

seanbudd commented Jan 9, 2025

@ABuffEr @brunoprietog

I also use it frequently when developing and diagnosing accessibility issues.
It's very useful to know if an element is focusable but not tabbable, i.e. has tabindex=“-1”.
recently, I enabled this option to show a web page correctly scrolled/focused during a working call.
So, is there an alternative way to get the same result?

Could you explain these different tasks in a bit more detail?
It's not immediately clear how this setting is useful for debugging accessibility issues

@brunoprietog
Copy link

Generally, in conjunction with OPbject Location Tones, if I browse the elements and NVDA emits a tone, I can notice that the control is focusable or not. Then, I can differentiate whether it's tabbable or not, in case I can access the control with tab. Capturing these differences is tremendously easy with this mode.

Also, not using this option generates an inconsistency. How could it be explained that when pressing tab a control is focused, but when passing through it in browse mode it's not? There's an easily reproducible case. If you browse GitHub and look for a comment that has reactions, when you focus the button, right below, it shows who has reacted with that reaction. With this mode disabled, you'd have to reach the item manually with tab, because otherwise the button will never get the focus and the event that shows the reactions below won't be triggered. How would you figure that out without this option enabled? Now that I know, I can apply the workaround, but I'd never have noticed that without this mode. Another common case is comboboxes. Many of them show suggestions when you focus them. Again, you'd have to reach it only with tab, because otherwise it'd never trigger the event. These inconsistencies are very annoying to me, at least.

Copy link
Member

@SaschaCowley SaschaCowley left a comment

Choose a reason for hiding this comment

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

Minor changes

source/browseMode.py Outdated Show resolved Hide resolved
source/browseMode.py Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
@LeonarddeR
Copy link
Collaborator

Re scrolling, there is #9919. I had to abandon it due to lack of time and interest, but I'm pretty sure it can be a good starting point to improve scrolling. Sighted dev would be ideal here.
Personally I support the deprecation path as proposed by @XLTechie, with the addition of instantly moving the GUI option to advanced settings.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jan 20, 2025
@seanbudd
Copy link
Member Author

@LeonarddeR @brunoprietog - do you think this issue #6382 / #9919 serves as a good summary or starting point for solving the base issue this setting helped with?

@seanbudd seanbudd requested a review from SaschaCowley January 21, 2025 02:38
Copy link
Member

@SaschaCowley SaschaCowley left a comment

Choose a reason for hiding this comment

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

Thanks, @seanbudd

@brunoprietog
Copy link

For me, the story is different. I started using NVDA with this setting enabled by default, and when I tried to disable it I got all sorts of hard to describe behaviors in addition to my arguments above. Having this option enabled creates predictability and consistency to the navigation.

@Adriani90
Copy link
Collaborator

@brunoprietog that might be the case in certain situations, though that setting enabled causes a lot of cursor jumping and crashes on complex websites with dynamic content. Given Sean's indication that this is impossible to maintain which I second, I think users can live with this setting being disabled. If you want to sync your system focus to the virtual cursor, you can always do that by pressing only one keystroke, namely nvda+shift+backspace. Review cursor always follows virtual cursor, so you can always move the system focus there manually.
Or you can use nvda+space bar to sync virtual and system focus.

You write:

Generally, in conjunction with OPbject Location Tones, if I browse the elements and NVDA emits a tone, I can notice that the control is focusable or not.

You can find out the same when navigating in browse mode with arrow keys, any element with a role is focusable and NVDA always pronounces the role, e.g. button, link, edit field, combo box etc. If you disabled reporting of these roles while using obj location tones add-on, you can always retrieve the tone by pressing nvda+shift+backspace to draw the system focus where you currently are in browse mode.

Removing this setting would be terrible for me. I use the Object Location Tones add-on to hear a sound every time I move through focusable controls. This really helps me to know where an object is on the screen quickly, and helps me to be more efficient when scrolling through the interface. It also helps me get a picture in my head of the layout of the interface, just as I can do when I'm using a touch device like my mobile phone.

You can also get familiar with the interface when moving the mouse and enable tones for mouse movements in the mouse settings category which will give you a rough idea of where objects are located on the screen.
Finally, the add-on itself can be improved to look for NVDA cursor only and not for the system focus.

How could it be explained that when pressing tab a control is focused, but when passing through it in browse mode it's not?

This is not an inconsistency, it is by design. When you are in browse mode and press tab, non focusable objects are traversed as well, and this is a different behavior to focus mode. It could be an option to let this automatic sync setting enabled only when pressing tab or shift+tab while in browse mode, though I am not sure this is feasible.
It is important to understand that the virtual buffer always works separated from the system focus. this is the case for all other screen readers out there.

If you browse GitHub and look for a comment that has reactions, when you focus the button, right below, it shows who has reacted with that reaction. With this mode disabled, you'd have to reach the item manually with tab, because otherwise the button will never get the focus and the event that shows the reactions below won't be triggered. How would you figure that out without this option enabled?

It seems this information is added via aria description or aria describedby, not sure. But this is something NVDA coud improve, e.g. announce "has details" in browse mode on the button with the additional information.
This can be achieved in a separate PR.

Another common case is comboboxes. Many of them show suggestions when you focus them. Again, you'd have to reach it only with tab, because otherwise it'd never trigger the event.

To be honnest it would be a nightmare if every combo box shows suggestion events when I land on them in browse mode. I actually need to know the suggestions only when I interact with combo boxes, and not when I am reviewing things in browse mode. Interactions with combo boxes are possible only in focus mode anyway.

@ABuffEr wrote:

recently, I enabled this option to show a web page correctly scrolled/focused during a working call. So, is there an alternative way to get the same result?

What exactly are you tryin to achieve? You can always check whether the page has scrolled properly by moving the mouse around or by pressing nvda+shift+backspace from time to time in case there are focusable objects near to your browse mode position. Or am I wrong?

@brunoprietog you wrote:

I'm sure to have used it towaydays from time to time, in poorly designed websites, where the alternative would be to move and move and move manually the focus or the mouse pointer.
Some poorly designed web pages may require users to enable this option as a temporary solution.

I never encountered such a website. Can you provide a test website where this is the case? I strongly believe people use this old approach because the newer workarounds have never been given a chance.

Finally, this option is off by default.

Still it causes difficulties in investigation when people report issues and they are caused by this setting.

It's very useful to know if an element is focusable but not tabbable, i.e. has tabindex=“-1”.

Tabindex -1 is not focusable at all with the system focus, it is only focusable with the virtual cursor. I'd say the accessibility inspectors out there give already enough comfort to find out which elements are not focusable, you don't really need this add-on for proofing websites. Anyway, if the add-on owuld provide the option to look only for nvda cursor instead of system focus, then you would get the same result with this setting disabled, so this can be improved within the add-on itself.

@LeonarddeR
Copy link
Collaborator

@seanbudd Yes, I think that this issue and pull request are a good starting point to ensure the scrolling behavior is OK with the automatically set focus option disabled.
That said, I'm not sure whether the experiences as described by @brunoprietog are all related to scrolling.

@ABuffEr
Copy link
Contributor

ABuffEr commented Jan 21, 2025

@Adriani90 wrote:

What exactly are you tryin to achieve? You can always check whether the page has scrolled properly by moving the mouse around or by pressing nvda+shift+backspace from time to time in case there are focusable objects near to your browse mode position. Or am I wrong?

Simply I was showing to colleagues a page with a lot of content (a Udemy course lessons organized in various tables), and enabling that option the page scrolled while I was moving for headers, links and so on.
Pressing nvda+shift+backspace each time or worse trying to control mouse seem to be a doable but not practical solution to me, sincerely.
Anyway, I read there is a PR about scrolling, and it's a good point. I'm not convinced that the focus option should be removed until that PR is not merged, but, well...

Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

Great work

@seanbudd seanbudd merged commit a9a081b into master Jan 22, 2025
5 checks passed
@seanbudd seanbudd deleted the removeAutoFocusFocusableElements branch January 22, 2025 00:16
@github-actions github-actions bot added this to the 2025.1 milestone Jan 22, 2025
@LeonarddeR
Copy link
Collaborator

I'm a bit confused that this is merged as is without noticeably addressing the concerns raised by @XLTechie, @ABuffEr and @brunoprietog.
@seanbudd Could you please elaborate on why the deprecation path as proposed by @XLTechie and seconded by me is not feasible?

@seanbudd
Copy link
Member Author

@LeonarddeR as answered earlier here - #17598 (comment)
To put it simply, the feature is unsupported, buggy and no longer maintainable. Advanced settings is intended for supported behaviour, even if experimental, but is not a graveyard for features.

Additionally, this feature has been disabled and discouraged for several years at this point. Delaying it another year is just delaying pulling off the bandaid, it's going to hurt the same now as later.

@CyrilleB79
Copy link
Collaborator

@seanbudd I understand that NV Access cannot support forever an unsupported buggy and unmaintainable feature. Though the reality is that various people still rely on it.

Thus, will there be a push/a priorization to solve scrolling issues?

Regarding add-ons author relying on this feature, it was up to them to ask for alternative methods (e.g. extension points) since the feature was already deprecated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-breaking-change conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet