fix: Short-circuit node lookups for missing IDs #9174
Conversation
BenHenning
left a comment
There was a problem hiding this comment.
Spot checked changes.
|
Looks like this is a slightly less simple fix than I was hoping. Core tests are passing, but the nav tests might have issues (perhaps indicating a setup issue). Will need to dig. |
Also, add a new check and warning, plus tests, for more robust FocusManager behavior.
RoboErikG
left a comment
There was a problem hiding this comment.
One suggestion, otherwise LGTM.
core/focus_manager.ts
Outdated
| // Fall back to a reasonable default since there's no valid node to focus. | ||
| const focusableNodeElement = focusableNode.getFocusableElement(); | ||
| if (!focusableNodeElement.id || focusableNodeElement.id === 'null') { | ||
| console.warn('Trying to focus a node that has an invalid ID.'); |
There was a problem hiding this comment.
Should this clear isUpdatingFocusedNode and return early? It looks like the only time the rest of this method would do anything is if the element is a root node.
There was a problem hiding this comment.
I don't think that's correct, but perhaps I'm misunderstanding you.
If there's an invalid ID, then findFocusedNode will return null causing the condition on line 369 to fail, thus leading to the node to focus falling back to the tree's defaults (which can be one of several nodes depending on the tree and its state).
I'll add a line comment here to make it a bit more clear that it's intentionally falling through.
The line comment is meant to clarify why execution doesn't stop when the new invalid state is encountered in focusNode().
BenHenning
left a comment
There was a problem hiding this comment.
Self-reviewed latest.
|
@RoboErikG I'll go ahead and merge this, but per the conversation thread please let me know if I missed something and if there's something else you think needs to be addressed here. |
The basics
The details
Resolves
Fixes #9155
Proposed Changes
In cases when an ID is missing for an element passed to
FocusableTreeTraverser.findFocusableNodeFor(), always returnnull.Additionally, the new short-circuit logic exposed that
Toolboxactually wasn't being set up correctly (that is, its root element was not being configured with a valid ID). This has been fixed.Reason for Changes
These are cases when a valid node should never be matched (and it's technically possible to incorrectly match if an
IFocusableNodeis set up incorrectly and is providing a focusable element with an unset ID). This avoids the extra computation time of potentially calling deep intoWorkspaceSvgand exploring all possible nodes for an ID that should never match.Note that there is a weird quirk with
nullIDs actually being the string"null". This is a side effect of howsetAttributeand attributes in general work with HTML elements. There's nothing really that can be done here, so it's now considered invalid to also have an ID of string"null"just to ensure thenullcase is properly short-circuited.Finally, the issue with toolbox being configured incorrectly was discovered with the introducing of a new hard failure in
FocusManager.registerTree()when a tree with an invalid root element is registered. From testing there are no other such trees that need to be updated.A new warning was also added if
focusNode()is used on a node with an element that has an invalid ID. This isn't a hard failure to follow the convention of other invalidfocusNode()situations. It's much more fragile forfocusNode()to throw thanregisterTree()since the former generally happens much earlier in a page lifecycle, and is less prone to dynamic behaviors.Test Coverage
New tests were added to validate the various empty ID cases for
FocusableTreeTraverser.findFocusableNodeFor(), and to validate the new error check forFocusManager.registerTree().Documentation
No new documentation should be needed.
Additional Information
Nothing to add.