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

Browse mode: only filter out speaking of focus events caused by NVDA, rather than trying to work out if the caret is still within the last focus. #14611

Merged
merged 4 commits into from Feb 15, 2023

Conversation

michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Feb 6, 2023

Link to issue number:

Fixes #12896
Fixes #6606
Fixes #14494
Fixes #14495
Fixes #14606

Summary of the issue:

NVDA sometimes fails to report focus in browseMode documents, as it is trying to filter out redundant focus events caused by moving the caret.
Particular cases:

  • Focus moves from a control inside a gridcell or interactive list item, back to the ancestor gridcell or interactive list item.
  • Focus moves from a gridcell or interactive list item, to the first control inside the gridcell or interactive list item.
    Currently, NVDA chooses to ignore any focus in browseMode that overlaps with the browseMode caret. This is so that any focus event that resalts from moving the caret is not reported as this would be redundant. However, this rule is clearly too limiting as it stops the above examples from working.

Description of user facing changes

In Browse mode, NVDA will no longer incorrectly ignore focus moving to a parent or child control e.g. moving from a control to its parent lit item or gridcell.
Note however that this fix only applies when the Automatically set focus to focusable elements option in Browse Mode settings is turned off (which is the default).

Description of development approach

Rather than ignoring focus changes in browse mode if the new focus's range overlaps the existing caret position, focus is only ignored if it is the same object that NvDA has specifically requestes etting focus to, such as when pressing the applications key or activating a control.

Testing strategy:

Performed steps in issues #14494, #14495, #12896, #6606.
Ensured that fixed issue #8341 is not regressed.

Known issues with pull request:

This fix only applies if the Automatically set focus to focusable elements option in Browse Mode settings is turned off (which is the default). Thus anyone who has turned on this option (to get back very old behaviour before 2019) won't get this fix. However, it is planned to remove this option completely in the near future.

Change log entries:

New features
Changes
Bug fixes

  • In Browse mode, NVDA will no longer incorrectly ignore focus moving to a parent or child control e.g. moving from a control to its parent list item or gridcell. Note however that this fix only applies when the Automatically set focus to focusable elements" option in Browse Mode settings is turned off (which is the default).
    For Developers

Code Review Checklist:

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

… rather than trying to work out if the caret is still within the last focus.
@michaelDCurran michaelDCurran requested a review from a team as a code owner February 6, 2023 00:33
@AppVeyorBot
Copy link

See test results for failed build of commit 25cb221da3

@AppVeyorBot
Copy link

See test results for failed build of commit f17541e498

@brunoprietog
Copy link

@michaelDCurran Why do you plan to remove this setting? I always have it enabled and it is very useful for me in conjunction with the ObjectLocationTones add-on. It allows me to have a spatial notion of the location of screen elements. If that option is removed, I will no longer be able to automatically know the location of the object, because it will not be automatically focused. Thanks

@fastfinge
Copy link

Chiming in to say I also keep this setting enabled. Both for this case, and also because some menus/controls will only expand on focus, it makes demos easier when working with sighted folks, and in the past when it's turned off I've had issues with the webpage not scrolling to the position of the virtual cursor. @brunoprietog Unfortunately, I don't have enough data to make a strong case for keeping this feature, and this feels off-topic for this pull request. Shoot me an email; perhaps this is something the two of us should work together on. @michaelDCurran is there any timeline for this removal? I do depend on this feature, and I'd like to advocate for it with something a bit more convincing than "Well, I like it and I think it should be turned on, and sometimes it causes me issues when it's off".

michaelDCurran added a commit that referenced this pull request Aug 1, 2023
…n the same link. (#15234)

Fixes #15211

Summary of the issue:
After merging of pr #14611, tabbing through a message in Windows Mail would cause the caret / focus to get stuck and not move forward to the next element in tab order.
This seems to be because BrowseModeDocument's gainFocus event updates the caret to the position of the element in the document that just gained focus. This is appropriate for web documents where NVDA manages the caret itself, but not for documents where the caret is managed by the application, such as the MS Word mail message viewer.
This has most likely been a problem in some way for a long time, but pr #14611 made it much more obvious as it greatly increased the amount of focus events within documents now spoken / handled.

Description of user facing changes
NVDA no longer gets stuck when tabbing through a message in Windows Mail.

Description of development approach
Suppressing of caret movement in BrowseModeDocument's gainFocus event is controled by a new private variable on the CursorManager class. It is False by default, which does not allow the caret movement to occur. However, for ReviewCursorManager and VirtualBuffer classes, it is True, allowing the existing caret movement behaviour, as in these situations the caret is managed entirely by NVDA.
This then means that in the cases where a CursorManager is used that is not VirtualBuffer or ReviewCursorManager, which is just Microsoft Word browse mode, NVDA will not move the caret itself on gainFocus events, as the application itself will already do this.
seanbudd pushed a commit that referenced this pull request Jan 10, 2024
…ically focus focusable elements disabled (#16016)

Fixes #15653

Summary of the issue:
Two bugs are seen if automatically focus focus elements in browse mode is disabled. Possibly introduced when that setting was introduced, and made worse when browse mode focus code was further refacted in #14611.

In iTunes, arrowing up and down inside a combo box incorrectly switches back to browse mode.
In Firefox / Chrome, closing a combo box does not automatically switch back to browse mode.
Description of user facing changes
NVDA will again switch back to browse mode when closing combo boxes with escape or alt+upArrow in Firefox or Chrome. (Action Items in iTunes not accessible #15653)
Arrowing up and down in combo boxes in iTunes will no longer inappropriately switch back to browse mode. (Action Items in iTunes not accessible #15653)
Description of development approach
BrowseModeDocument's collapseOrExpandControl script: if automatic focus focusable elements is disabled, delay the rest of the script to give time for the control to actually get focus.

Webkit (iTunes) virtualBuffer: do not treat the list items of a combo box as being part of the virtualBuffer.
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
…ically focus focusable elements disabled (nvaccess#16016)

Fixes nvaccess#15653

Summary of the issue:
Two bugs are seen if automatically focus focus elements in browse mode is disabled. Possibly introduced when that setting was introduced, and made worse when browse mode focus code was further refacted in nvaccess#14611.

In iTunes, arrowing up and down inside a combo box incorrectly switches back to browse mode.
In Firefox / Chrome, closing a combo box does not automatically switch back to browse mode.
Description of user facing changes
NVDA will again switch back to browse mode when closing combo boxes with escape or alt+upArrow in Firefox or Chrome. (Action Items in iTunes not accessible nvaccess#15653)
Arrowing up and down in combo boxes in iTunes will no longer inappropriately switch back to browse mode. (Action Items in iTunes not accessible nvaccess#15653)
Description of development approach
BrowseModeDocument's collapseOrExpandControl script: if automatic focus focusable elements is disabled, delay the rest of the script to give time for the control to actually get focus.

Webkit (iTunes) virtualBuffer: do not treat the list items of a combo box as being part of the virtualBuffer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment