[PLA-11449] Improve NavigationDropdown keyboard accessibility #143

sampullman · 2025-06-11T13:18:32Z

Currently, only arena rows are selectable. I considered allowing header rows to be selected, and opening/closing on enter, but it didn't seem useful enough to spend time on.

This is messier than I expected it to be, I may have missed some existing functionality of VirtualTree that would have made it easier. I'll leave a couple inline comments.

Edit: is the CLA Assistant supposed to automatically prompt me here? I'm not about the format for signing in a PR comment.

s2.mp4

vercel · 2025-06-11T13:18:42Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
plasmic-cms-i18n	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 13, 2025 4:00pm

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
react-email-demo	⬜️ Ignored (Inspect)			Jun 13, 2025 4:00pm

vercel · 2025-06-11T13:18:44Z

@sampullman is attempting to deploy a commit to the Plasmic Team on Vercel.

A member of the Team first needs to authorize it.

platform/wab/src/wab/client/components/grouping/VirtualTree.tsx

platform/wab/src/wab/client/components/sidebar-tabs/ProjectPanel/NavigationDropdown.tsx

platform/wab/src/wab/client/components/grouping/VirtualTree.tsx

platform/wab/src/wab/client/components/sidebar-tabs/ProjectPanel/NavigationDropdown.tsx

platform/wab/src/wab/client/components/grouping/VirtualTree.tsx

platform/wab/src/wab/client/components/sidebar-tabs/ProjectPanel/NavigationDropdown.tsx

sampullman · 2025-06-12T06:43:01Z

Extracting useTreeData wasn't too bad, but the LeftGeneralTokensPanel.tsx refactor was a little more complicated that I expected, so it might be worth verifying those changes closely. Overall I think it's a good change, and it enabled a few more simplifications. I'll add a few inline comments!

sampullman · 2025-06-12T06:44:10Z

platform/wab/src/wab/client/components/grouping/useTreeData.tsx

+
+  const [selectedIndex, setSelectedIndex] = React.useState<number>();
+
+  // Reset selectionIndex when visibleNodes change


This seems messy, but I couldn't think of a better way to ensure there is a valid selection when the tree first opens.

Yeah it feels a bit weird because everything is triggered bay prop changes instead of a function call. I think the iteration is okay, but it might be improved by having useTreeData expose a setQuery function (similar to selectNextRow). Then the parent component can call setQuery to trigger all the changes more naturally (e.g. call buildVisibleNodes, reset selectedIndex, recompute expanded nodes, etc).

I explored this briefly, it looks like it will simplify some things but is a pretty substantial change. Would you rather I go for it here, or do it in parallel with other tasks (or leave it be for now)?

platform/wab/src/wab/client/components/sidebar-tabs/ProjectPanel/NavigationDropdown.tsx

sampullman · 2025-06-12T06:46:51Z

platform/wab/src/wab/client/components/sidebar-tabs/ProjectPanel/NavigationDropdown.tsx

@@ -364,6 +358,14 @@ function NavigationDropdown_(
    return { onClose };
  }, [onClose]);

+  const navigateToArena = React.useCallback(


I extracted the arena navigation from NavigationRows and use a new onClick prop instead. I did have to move the ArenaTreeRow function into NavigationDropdown_ to make it work.

Moving the ArenaTreeRow into here looks suspicious. ArenaTreeRow is considered a component by React, and now the component is being regenerated on every render, so React won't know that it's the same component.

What was the reason it needed to be moved in here? Is it depending on some data?

Perhaps the correct thing here would be to make a getNodeAction or something similar in useTreeData so we can pass these actions in. But yeah, I think creating the component inside another component degrades React performance, so we want to avoid that.

Yes, that makes sense. I thought I saw some nested components here and there, but probably didn't understand the full context. I'll see about doing it via getNodeAction or similar!

platform/wab/src/wab/client/components/sidebar/LeftGeneralTokensPanel.tsx

jaslong

looking good!

jaslong · 2025-06-12T07:13:29Z

platform/wab/src/wab/client/components/grouping/useTreeData.tsx

+
+  const [selectedIndex, setSelectedIndex] = React.useState<number>();
+
+  // Reset selectionIndex when visibleNodes change


Yeah it feels a bit weird because everything is triggered bay prop changes instead of a function call. I think the iteration is okay, but it might be improved by having useTreeData expose a setQuery function (similar to selectNextRow). Then the parent component can call setQuery to trigger all the changes more naturally (e.g. call buildVisibleNodes, reset selectedIndex, recompute expanded nodes, etc).

jaslong · 2025-06-12T07:16:00Z

platform/wab/src/wab/client/components/grouping/useTreeData.tsx

Prefer keeping this in VirtualTree to make the diff easier to read. Or add a commit before this commit just for the move.

jaslong · 2025-06-12T07:16:57Z

platform/wab/src/wab/client/components/sidebar-tabs/ProjectPanel/NavigationDropdown.tsx

@@ -364,6 +358,14 @@ function NavigationDropdown_(
    return { onClose };
  }, [onClose]);

+  const navigateToArena = React.useCallback(


Moving the ArenaTreeRow into here looks suspicious. ArenaTreeRow is considered a component by React, and now the component is being regenerated on every render, so React won't know that it's the same component.

What was the reason it needed to be moved in here? Is it depending on some data?

IcaroG

Last point from me, all other stuff looks good.

IcaroG · 2025-06-12T16:08:34Z

platform/wab/src/wab/client/components/sidebar-tabs/ProjectPanel/NavigationDropdown.tsx

@@ -364,6 +358,14 @@ function NavigationDropdown_(
    return { onClose };
  }, [onClose]);

+  const navigateToArena = React.useCallback(


Perhaps the correct thing here would be to make a getNodeAction or something similar in useTreeData so we can pass these actions in. But yeah, I think creating the component inside another component degrades React performance, so we want to avoid that.

sampullman · 2025-06-13T04:46:48Z

@jaslong @IcaroG I made the following changes, pinging because the re-review button isn't available.

Update the useTreeData refactoring commit to keep it in VirtualTree.tsx
Move ArenaTreeRow back out of the NavigationDropdown_ scope

Edit: rebased against master

IcaroG · 2025-06-13T15:07:33Z

platform/wab/src/wab/client/components/grouping/VirtualTree.tsx

  renderElement: RenderElement<T>;
-  defaultOpenKeys?: "all" | Key[];
+  nodeData;


Missing nodeData type?

Oh right, I somehow messed that up when merging useTreeData back to VirtualTree.tsx. I'm used to relying on noImplicitAny 😅

IcaroG · 2025-06-13T15:12:40Z

platform/wab/src/wab/client/components/grouping/VirtualTree.tsx

-interface LinearTreeNode<T> {
+type SelectDirection = 1 | -1;
+
+type NodeAction<T> = (node: T) => void | Promise<void>;


I think the return should probably be Promise<void> only, so the user has to always handle it.

IcaroG

Will wait for Jason's review as well

vercel bot deployed to Preview – plasmic-cms-i18n June 11, 2025 13:19 View deployment

sampullman commented Jun 11, 2025

View reviewed changes

platform/wab/src/wab/client/components/grouping/VirtualTree.tsx Outdated Show resolved Hide resolved

sampullman commented Jun 11, 2025

View reviewed changes

platform/wab/src/wab/client/components/sidebar-tabs/ProjectPanel/NavigationDropdown.tsx Outdated Show resolved Hide resolved

sampullman commented Jun 11, 2025

View reviewed changes

platform/wab/src/wab/client/components/grouping/VirtualTree.tsx Outdated Show resolved Hide resolved

IcaroG reviewed Jun 11, 2025

View reviewed changes

platform/wab/src/wab/client/components/sidebar-tabs/ProjectPanel/NavigationDropdown.tsx Outdated Show resolved Hide resolved

platform/wab/src/wab/client/components/grouping/VirtualTree.tsx Outdated Show resolved Hide resolved

jaslong reviewed Jun 12, 2025

View reviewed changes

vercel bot deployed to Preview – plasmic-cms-i18n June 12, 2025 06:38 View deployment

sampullman force-pushed the pla-11449-keyboard-accessibility branch from b1e1c15 to 03932d8 Compare June 12, 2025 06:40

vercel bot deployed to Preview – plasmic-cms-i18n June 12, 2025 06:40 View deployment

sampullman commented Jun 12, 2025

View reviewed changes

platform/wab/src/wab/client/components/sidebar-tabs/ProjectPanel/NavigationDropdown.tsx Show resolved Hide resolved

sampullman commented Jun 12, 2025

View reviewed changes

platform/wab/src/wab/client/components/sidebar/LeftGeneralTokensPanel.tsx Show resolved Hide resolved

jaslong reviewed Jun 12, 2025

View reviewed changes

IcaroG reviewed Jun 12, 2025

View reviewed changes

sampullman requested review from jaslong, asimkhan73301 and IcaroG June 12, 2025 16:36

sampullman force-pushed the pla-11449-keyboard-accessibility branch from 03932d8 to 3fff057 Compare June 13, 2025 04:42

vercel bot deployed to Preview – plasmic-cms-i18n June 13, 2025 04:42 View deployment

sampullman force-pushed the pla-11449-keyboard-accessibility branch from 3fff057 to 38f4ec6 Compare June 13, 2025 04:43

vercel bot deployed to Preview – plasmic-cms-i18n June 13, 2025 04:44 View deployment

[PLA-11449] Improve NavigationDropdown keyboard accessibility

65f5879

sampullman force-pushed the pla-11449-keyboard-accessibility branch from 38f4ec6 to ff8cec3 Compare June 13, 2025 05:10

vercel bot deployed to Preview – plasmic-cms-i18n June 13, 2025 05:11 View deployment

IcaroG reviewed Jun 13, 2025

View reviewed changes

sampullman added 3 commits June 13, 2025 23:55

[PLA-11449] extract useTreeData hook and refactor usage

43bbe2a

[PLA-11449] bubble onClick in NavigationArenaRow

6f27052

[PLA-11449] add nodeAction to VirtualTree

09b97df

sampullman force-pushed the pla-11449-keyboard-accessibility branch from ff8cec3 to 09b97df Compare June 13, 2025 15:59

vercel bot deployed to Preview – plasmic-cms-i18n June 13, 2025 16:00 View deployment

sampullman requested a review from IcaroG June 13, 2025 16:02

IcaroG approved these changes Jun 13, 2025

View reviewed changes

jaslong approved these changes Jun 13, 2025

View reviewed changes


		const [selectedIndex, setSelectedIndex] = React.useState<number>();

		// Reset selectionIndex when visibleNodes change

[PLA-11449] Improve NavigationDropdown keyboard accessibility #143

Are you sure you want to change the base?

[PLA-11449] Improve NavigationDropdown keyboard accessibility #143

Uh oh!

Conversation

sampullman commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Jun 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sampullman commented Jun 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sampullman Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaslong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IcaroG left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sampullman commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IcaroG left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sampullman commented Jun 11, 2025 •

edited

Loading

vercel bot commented Jun 11, 2025 •

edited

Loading

sampullman Jun 13, 2025 •

edited

Loading

sampullman commented Jun 13, 2025 •

edited

Loading