Remove Mozilla specific setFocus implementation which waits for a focus event before returning.#10584
Merged
Merged
Conversation
…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.
michaelDCurran
approved these changes
Dec 9, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.