BrowseMode: don't refuse to report focus changes for replaced controls or focused ancestors #8869
Conversation
…en the focus is replaced with a new focus at the same offsets, or when focus moves to an ancestor.
source/browseMode.py
Outdated
| @@ -1231,14 +1231,21 @@ def _set_selection(self, info, reason=controlTypes.REASON_CARET): | |||
| if obj==self.rootNVDAObject: | |||
| return | |||
| if focusObj and not eventHandler.isPendingEvents("gainFocus") and focusObj!=self.rootNVDAObject and focusObj != api.getFocusObject() and self._shouldSetFocusToObj(focusObj): | |||
There was a problem hiding this comment.
Do you understand this condition well enough to replace any parts of it with a self-documenting variable names. Or perhaps a comment about why each condition is in there?
source/browseMode.py
Outdated
| caretInfo.expand(textInfos.UNIT_CHARACTER) | ||
| if not self._hadFirstGainFocus or not focusInfo.isOverlapping(caretInfo): | ||
| # The virtual caret is not within the focus node. | ||
| if not self._hadFirstGainFocus or not lastFocusInfo or focusInfo.compareEndPoints(lastFocusInfo,"startToStart")!=0 or focusInfo.compareEndPoints(lastFocusInfo,"endToEnd")>0: |
There was a problem hiding this comment.
Could you extract focusInfo.compareEndPoints(lastFocusInfo,"startToStart")!=0 or focusInfo.compareEndPoints(lastFocusInfo,"endToEnd")>0 into a method that explains what it does?
Both to condense it, and to make it easier to understand.
There was a problem hiding this comment.
Like your other suggestion above, I think I'd prefer to just split the line up and add comments for each comparison. A method seems a bit strange as it would be only acting upon its arguments with no state.
|
@feerrenrut I believe I have addressed your review comments. |
|
I am going to revert this from beta as it causes#8895 and it is similarly causing some buttons and links to be repeated when arrowing to them in browse mode on Github. Most likely this is due to Firefox re-framing the accessible due to a css onFocus change. The old accessible is dying, and a new accessible (with a new ID) is focused. |
Note that this was fixed in Firefox some time ago now. Given that some of the issues fixed here are still relevant (#6606, #12073), it might be worth considering this again. |
Link to issue number:
Closes #6606
Closes #8341
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 replacement
Custom checkboxes and expandable controls that draw a checkmark or + symbol using the content css property on the element's ::before selector, can cause CSS reframing in Firefox, meaning that the accessible representing the focus dies and is replaced with a new accessible. NVDA refuses in this case to announce the state change, and also the new focus. At very least, NVDA should announce the new focus.
A similar case (but this time deliberate on the part of the web author) is where the current focus is removed from the DOM completely, and a new element is inserted in its place and then focused. An example of this is the Edit title button on Github pull requests. When pressing the button, the Title edit field appears in its place, and NVDA reports nothing.
Focus moves to an ancestor
Sometimes web authors want to focus an ancestor of the previous focus. this is quite common for single-page web apps. In this case, NVDA reports nothing.
Current implementation
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 how this pull request fixes the issue:
This PR changes browseMode so that:
Testing performed:
Tested all cases in #6606 and #8341
Known issues with pull request:
None known. Note though this does not address the underlying issue in #2039.
Change log entry:
Bug fixes:
NVDA no longer refuses to report the focus on web pages where the new focus replaces a control that no longer exists. (#6606, #8341)