Improve Canvas Tab Support#399
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces custom tab navigation for cards within the tldraw canvas, allowing users to seamlessly step through focusable card elements. It also refactors the focus trap utility and updates related CSS styles. The review feedback highlights several critical improvements: ensuring the capture-phase tab listener does not break custom interactive elements (like menus or dialogs), fixing a strict weak ordering violation in the card sorting logic, correctly targeting the last focusable element when navigating backwards with Shift+Tab, and excluding hidden inputs from the tabbable elements selector.
nick-nlb
left a comment
There was a problem hiding this comment.
The functionality over all works well.
However, if you have a card selected, and start tabbing between cards, the subsequent cards where the focus now falls does not get selection (i.e. the original selected card remains the selected one, and for a time both cards have an action bar at the top (until focus leaves the action bar).
This in itself might be correct behavior, but it brings up a question that we should put up for discussion: the keyboard navigation is powerful, but you don't have a way to select or deselect cards.
Otherwise a minor comment and a task that we could either do in this PR or move to the tracker.
|
|
||
| useKeydown('Escape', onClose); | ||
|
|
||
| // TODO: For now this doesn't seem to really work due to TLDraw consuming |
There was a problem hiding this comment.
I made a comment about this in a previous PR (which is now out-of-date due to this change).
| return; | ||
| } | ||
|
|
||
| const focusables = getCardFocusables(editor); |
There was a problem hiding this comment.
This runs on every tab hit. This means that as a user tabs through the canvas, with each key press the focusables array is rebuild (despite it only changing if the canvas actually changes).
It's not critical at this stage, but this could get costly on a very large canvas with many cards. Can we create a tracker item to update this to cache it (and invalidate the cache whenever the canvas changes)?
| @@ -1 +1 @@ | |||
| @layer reset, root, base, primitive; | |||
| @layer reset, root, primitive, base; | |||
There was a problem hiding this comment.
There's content in the FRONTEND.md documentation that refers to the order and still documents it the other way around.
Overview
Adds keyboard tab navigation through card content on the tldraw canvas, which tldraw normally swallows to cycle shape selection.
Changes
registerCardTabNavigation— a capture-phase Tab/Shift+Tab handler that steps through a flat list of every card's focusable elements in reading order (top-to-bottom, left-to-right), wrapping at the ends. It skips culled cards, stays out of the way of open dialogs and tldraw text editing, dives into the selected card when tabbing in from the canvas, and callsmarkEventAsHandledso tldraw ignores the event.AtlasProvider— registers on editor mount, cleans up on unmount.:focus-within(not just selection) so it's reachable via Tab.useFocusTrapimprovements — export and broadenTABBABLE_ELEMENTS(add inputs/selects/textareas, exclude disabled) for reuse; guard against non-HTMLElementactive elements.basenow sits afterprimitive. This was changed becausetldrawhas a defaultoutline: noneon*elements which was overriding ourbasestyle. Ourbaseis quite lean so this change is safe.