Skip to content

Improve Canvas Tab Support#399

Open
PauloMFJ wants to merge 3 commits into
datacommonsorg:mainfrom
madebypxlp:paulo/improve-tab-support
Open

Improve Canvas Tab Support#399
PauloMFJ wants to merge 3 commits into
datacommonsorg:mainfrom
madebypxlp:paulo/improve-tab-support

Conversation

@PauloMFJ

@PauloMFJ PauloMFJ commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Overview

Adds keyboard tab navigation through card content on the tldraw canvas, which tldraw normally swallows to cycle shape selection.

Changes

  • New 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 calls markEventAsHandled so tldraw ignores the event.
  • Wired into AtlasProvider — registers on editor mount, cleans up on unmount.
  • Card CSS — reveal the action bar on :focus-within (not just selection) so it's reachable via Tab.
  • useFocusTrap improvements — export and broaden TABBABLE_ELEMENTS (add inputs/selects/textareas, exclude disabled) for reuse; guard against non-HTMLElement active elements.
  • Re-enabled focus trap in the chart options menu — removed the stale TODO now that Tab handling works.
  • CSS layer reorderbase now sits after primitive. This was changed because tldraw has a default outline: none on * elements which was overriding our base style. Our base is quite lean so this change is safe.

@PauloMFJ PauloMFJ self-assigned this Jul 6, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread dataweaver/apps/web/src/components/scopes/atlas/register_card_tab_navigation.ts Outdated
Comment thread dataweaver/apps/web/src/components/scopes/atlas/register_card_tab_navigation.ts Outdated
Comment thread dataweaver/apps/web/src/hooks/use_focus_trap.ts

@nick-nlb nick-nlb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's content in the FRONTEND.md documentation that refers to the order and still documents it the other way around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants