fix: Improve missing node resiliency #8997
Merged
Conversation
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
This adds better resiliency in FocusManager when trying to focus nodes that are no longer attached to their parents (which can happen in some asynchronous cases).
This helps to be clear on another case now possible for IFocusableTree.getRestoredFocusableNode.
BenHenning
commented
May 6, 2025
|
@rachel-fenichel also assigning to you in case you have time to review this before @gonfunko. |
1 task
rachel-fenichel
approved these changes
May 6, 2025
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The basics
The details
Resolves
Fixes #8994
Proposed Changes
This removes an error that was previously thrown by
FocusManagerwhen attempting to focus an invalid node (such as one that's been removed from its parent).Reason for Changes
#8994 (comment) goes into more detail. While this error did cover legitimately wrong cases to try and focus things (and helped to catch some real problems), fixing this 'properly' may become a leaky boat problem where we have to track down every possible asynchronous scenario that could produce such a case. One class of this is ephemeral focus which had robustness improvements itself in #8981 that, by effect, caused this issue in the first place. Holistically fixing this with enforced API contracts alone isn't simple due to the nature of how these components interact.
This change ensures that there's a sane default to fall back on if an invalid node is passed in. Note that
FocusManagerwas designed specifically to disallow defocusing a node (since fallbacks can get messy and introduce unpredictable user experiences), and this sort of allows that now. However, this seems like a reasonable approach as it defaults to the behavior when focusing a tree explicitly which allows the tree to fallback to a more suitable default (such as the first item to select in the toolbox for that particular tree). In many cases this will default back to the tree's root node (such as the workspace root group) since sometimes the removed node is still the "last focused node" of the tree (and is considered valid for the purpose of determining a fallback; tree implementations could further specialize by checking whether that node is still valid).Test Coverage
Some new tests were added to cover this case, but more may be useful to add as part of #8910.
Documentation
No documentation needs to be added or updated as part of this (beyond code documentation changes).
Additional Information
This original issue was found by @RoboErikG when testing #8995. I also verified this against the keyboard navigation plugin repository.