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 Mozilla specific setFocus implementation which waits for a focus event before returning. #10584

Merged
merged 1 commit into from Dec 9, 2019

Conversation

jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Dec 6, 2019

Link to issue number:

None.

Summary of the issue:

This setFocus override was added in #8678 to mitigate focus "rubber-banding" and spurious switching to focus mode when moving fast in browse mode. However, this degrades performance in Firefox.

Description of how this pull request fixes the issue:

This PR simply removes this code. We (@MarcoZehe and I) tested with this code removed for several hours in both normal daily usage and intentionally rapid usage. The rubber-banding/spurious focus mode switching no longer seem to be a problem.

It's likely that changes in Firefox, NVDA or both mitigated this. The changes to Firefox to avoid extraneous tree re-creation are a likely candidate, though I know there was also some refactoring of NVDA's focus handling code for browse mode.

Testing performed:

See above.

Known issues with pull request:

It's possible that we'll see the same issues this code was originally added to mitigate. However, it's worth noting that Chrome also sets focus asynchronously. I confirmed this by calling setFocus and then checking the focused state immediately afterwards. My best guess is that extraneous tree re-creation in older Firefox versions was causing enough of a perf hit to trigger these problems in Firefox... or perhaps Firefox was actually generating a new accessible for the focus when this occurred, which would have been even worse.

Change log entry:

Bug fixes:
- In Mozilla Firefox, moving focus in browse mode is faster. This makes moving the cursor in browse mode more responsive in many cases.

…us event before returning.

This was added in nvaccess#8678 to mitigate focus "rubber-banding" and spurious switching to focus mode when moving fast in browse mode.
However, this degrades performance.
Removing this code, this no longer seems to be a problem.
It's likely that changes in Firefox, NVDA or both mitigated this.
The changes to Firefox to avoid extraneous tree re-creation are a likely candidate.
@jcsteh jcsteh requested a review from michaelDCurran Dec 6, 2019
@michaelDCurran michaelDCurran merged commit f3ac180 into nvaccess:master Dec 9, 2019
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Dec 9, 2019
michaelDCurran added a commit that referenced this issue Dec 9, 2019
@jcsteh jcsteh deleted the mozillaSetFocus branch May 1, 2020
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

3 participants