Conversation
| return node.getSourceBlock(); | ||
| } else if (node instanceof Icon) { | ||
| return node.getSourceBlock() as BlockSvg; | ||
| } |
There was a problem hiding this comment.
worth adding a if typoeof node.getSourceBlock === function return node.getSourceBlock() to sort of make this work as if we had a IBelongsToBlock interface or something? Or worth formalizing that as an interface? potentially as a followup.
| /** | ||
| * Returns an ID for the visual "row" this input is part of. | ||
| * | ||
| * @internal |
There was a problem hiding this comment.
I don't think this should be internal since it would be called from a navigation policy which is externally modifiable. right?
but if it's not internal, you can still add a disclaimer that the value only holds meaning for keyboard navigation and the value is not guaranteed to be stable, to discourage people from relying on this to do shenanigans
There was a problem hiding this comment.
Yeah, I was on the fence about this. You can create your own NavigationPolicy, subclass the existing ones, change the set that a Navigator uses, or subclass Navigator. There isn't inherently a reason to need this method though; we don't provide any ability to add new kinds of entities to blocks, where the block version of this might be needed, and you could for example subclass the Field or Connection navigation policies which call this method but just not override getRowId() in those, although again I don't think there's any reason to now that the navigation policies themselves don't really contain business logic. Navigator just calls getRowId() on the navigation policy, not this method, so you can even get it there with public API. If you make a new entity and put it in the flyout you'd need to specify a row ID in your navigation policy, but that would be your own synthetic ID, not an input or block one. So I think @internal is fine, especially since it's a sign not a cop? In general I figured we could start it off that way, and if someone came to us with a compelling reason why they needed it we could remove it. WDYT?
There was a problem hiding this comment.
I was thinking the case where someone registers their own input navigation policy, they would need to call this. But that's fine, I think you're right it can start this way until needed.
| * @param current The element to return the row ID of. | ||
| * @returns The row ID of the given element. | ||
| */ | ||
| getRowId(current: T): string; |
There was a problem hiding this comment.
the one remaining thing I find odd about this is that things that aren't on blocks also have this, like workspace comments. but i can't think of a different way to do this that's better
There was a problem hiding this comment.
In theory the bar button icons could be one row and the editor field another; it's not currently set up that way, but I don't think it's too much of a stretch. And the comment itself is of course a form of row.
The basics
The details
Proposed Changes
This PR backports the core support for keyboard navigation of the workspace, toolbox and flyout from the blockly-keyboard-experimentation repo. It also refactors things to provide a more cohesive API for interacting with navigation. The general aspirational approach is:
*_navigation_policy.tsfiles specify, for each kind of focusable/navigable entity in Blockly, what its parent, first child, and previous/next siblings are, from a "just the facts"/AST perspective.Navigatorprovides methods to query the "just the facts" relationships between elements, but also provides methods that incorporate business logic to make navigation order in response to the arrow keys make sense/feel right/pass the vibe test.FlyoutNavigatorandToolboxNavigatorcustomize the business logic for navigation in a flyout and toolbox context, respectively, and operate on a set of navigation policies relevant to focusable/navigable entities that exist in those contextsIFocusableTreeprovides aNavigatorinstance which should be used to handle navigation within that treeNavigatorfor the active focus treeThis means that keyboard navigation does not have to keep track of the current state/context as in the keyboard-experimentation repo, because that maps precisely to the currently focused tree, which the
FocusManageralready keeps track of.I backported the navigation business logic from the add-screen-reader-support-experimental branch, where up/down move between "rows" and left/right navigate within the current "row".
Future Work
Breaking Changes
MarkerManager,Marker, andLineCursorhave all been removed, along with their accessors. If you were interacting with one of these classes,Navigatorshould be close to a drop-in replacement.IFocusableTreemust implementgetNavigator(), which should return aNavigatorinstance or subclass appropriate for navigating the treeFlyoutno longer implementsIFocusableNodeorIFocusableTree. The flyout's workspace continues to implement those interfaces, and should be used in place of the flyout itself.IToolboxprovides agetToolboxItems()method that returns an array of all itemsIToolboxItemprovides agetParentToolbox()method that returns a reference to its containing toolbox