fix: Fix bug that prevented keyboard navigation in flyouts. #8687
Conversation
core/keyboard_nav/ast_node.ts
Outdated
| resultIndex >= 0 && | ||
| resultIndex < flyoutContents.length && | ||
| !( | ||
| flyoutContents[resultIndex].element instanceof BlockSvg || |
There was a problem hiding this comment.
Can you file a bug to create an isNavigable API or similar for flyout items? This will do for now but eventually those custom items will need to be navigable (a text entry box for searching being the most obvious case).
There was a problem hiding this comment.
Yes, this is why I was asking about ASTNode in the chat :) I think we probably just want to add that method to IASTNodeLocation; should I just go ahead and do that in this PR?
There was a problem hiding this comment.
I think so; @cpcallen to confirm. This would be in the v12 beta so we'll still have time to change it.
There was a problem hiding this comment.
I think one of us might be confused, because it doesn't make sense to me for IASTNodeLocation to have an isNavigable method, because (in practice) an IASTNodeLocation is a thing one has already navigated to (i.e., that is the value of a .location property of an ASTNode—so you'd have to have called .isNavigable on the possible location before you create the ASTNode for it (which is the first time it definitely needs to fulfil the interface), and you won't have any reason to call isNavigable again because you already know it's navigable. :-/
I think the method should probably be called isFocusable and be on FlyoutItem, but I'm not sure as I don't fully understand the flyout changes in v12.
There was a problem hiding this comment.
Thank you, I was the confused one – I didn't realize the already navigated to aspect of IASTNodeLocation. As suggested, I've added isFocusable() to FlyoutItem. Since this is still only in beta and part of a larger breaking change, I took the liberty of refactoring the APIs to be a bit more consistent.
| constructor( | ||
| private element: IBoundedElement, | ||
| private type: string, | ||
| private focusable: boolean, | ||
| ) {} |
There was a problem hiding this comment.
Is there some reasonably foreseeable scenario in which this class might be subclassed? If not, it might be better to make the members (public) readonly and omit the get* methods. The styleguide has this rule for get/set accessors (get foo()) which I think seems like good advice also for ordinary getter methods (getFoo()).
Getters and setters, also known as accessors, for class members may be used…
If an accessor is used to hide a class property, the hidden property may be prefixed or suffixed with any whole word, like internal or wrapped. When using these private properties, access the value through the accessor whenever possible. At least one accessor for a property must be non-trivial: do not define pass-through accessors only for the purpose of hiding a property. Instead, make the property public (or consider making it readonly rather than just defining a getter with no setter).
Of course if this class is likely to get subclassed then the current approach seems sound.
There was a problem hiding this comment.
I could imagine a situation where somebody with a custom flyout might want to add additional metadata to the items in it, so perhaps? Happy to adjust if you think that's preferable though.
The basics
The details
This PR fixes an issue that caused
findNextLocationInFlyout()to return invalid items. With the flyout changes in v12, spacers (and potentially custom user-defined items) are included in the list of flyout contents, but are not valid keyboard navigation locations. As such, simply taking the next/previous item relative to the current one is insufficient; that item may be inspected, but if it is of a non-navigable type, the subsequent item needs to be checked until a valid navigation location is found.