Make VNet connection item behave like other items #40249

ravicious · 2024-04-05T10:59:36Z

VNet was already displayed in the connection list, however it couldn't be selected with a keyboard nor included/excluded from search within the connection list. This PR makes it so that it behaves like any other item.

I also made sure that keyboard navigation works good enough. After you select VNet from the main list, the outermost div of the VNet panel becomes focused, so you can press tab just once to tab through VNet controls.

This is not perfect, there are two issues that I decided to not address for now, but if you feel strongly about them I'd be inclined to fix them:

Tabbing from the main list to the VNet toggle button and pressing enter doesn't work. That's because KeyboardArrowsNavigation steals all key down events and dispatches to them only if there's an active item selected with arrows.
- This could be potentially fixed by making sure that the key down events are stolen only if they originate exactly from the element which calls useKeyboardArrowsNavigation. This would require keeping track of a list of refs.
Once you go to the VNet panel, you can only use the tab to switch focus, arrows do nothing. I think a typical user would expect that up/down arrow in the VNet panel selects the VNet header. But if we use KeyboardArrowsNavigation there, then you cannot press enter on the buttons because of the issue described above.

I also vendored in react-merge-refs because I needed it in one place. It's a small lib with two files (one for implementation, one for tests) and types. I figured if we ever need to update it because of React changes, then it'll be easy because of the types and tests.

Important

To see the UI for VNet, you need to set feature.vnet to true in your app config. For the dev version of the app it's ~/Library/Application\ Support/Electron/app_config.json.

vnet-keyboard.mov

gzdunek

Works fine. In regard to the issues:

Tabbing from the main list to the VNet toggle button and pressing enter doesn't work.

This one seems more important to me, it affects all connection items, you have to press Space to run these secondary actions.

Once you go to the VNet panel, you can only use the tab to switch focus, arrows do nothing.

Having to switch from arrows to tab is quite unexpected, and I would probably think that this particular component doesn't support keyboard navigation :)
But I would treat it as a nice to have thing that can be addressed in the future.

gzdunek · 2024-04-05T13:43:38Z

web/packages/design/src/StepSlider/StepSlider.tsx

  refCallback(node: HTMLElement): void;
-  // next goes to the next step in the flow.
+  /**
+   * next goes to the next step in the flow.


I'm not a fan o repeating variable names in JSDoc:

Avoid repeating type information in comments and variable names. In the best case it is duplicative of type declarations, and in the worst it will lead to conflicting information.

https://effectivetypescript.com/2023/05/31/jsdoc-repeat/

I'm not requesting a change here since you only converted the comments to JSDoc, I'm leaving it up to you :)

I don't really care about it, but it's the standard in Go. https://go.dev/wiki/CodeReviewComments#comment-sentences

The language server I use for Go even updates the name of a function in the comment after I rename a function.

Since JSDoc is pretty flexible in that regard and the Go community isn't, I'd rather accept that some JSDocs will repeat the name of the thing they describe.

is pretty flexible in that regard and the Go community isn't

typical

gzdunek · 2024-04-05T13:47:45Z

web/packages/shared/libs/mergeRefs.ts

+ */
+export function mergeRefs<T = any>(
+  refs: Array<React.MutableRefObject<T> | React.LegacyRef<T> | undefined | null>
+): React.RefCallback<T> {


Here we import React from a global namespace, but I'm not sure if we want to modify vendored files.

gzdunek · 2024-04-05T13:50:57Z

web/packages/teleterm/src/ui/Vnet/VnetConnectionItem.test.tsx

+    await user.tab();
+    expect(listItem).toHaveFocus();
+
+    await user.tab();
+    expect(openDocumentationButton).toHaveFocus();
+
+    await user.tab();
+    expect(toggleButton).toHaveFocus();


ravicious · 2024-04-08T09:27:55Z

web/packages/teleterm/src/ui/Vnet/VnetConnectionItem.tsx

Tabbing from the main list to the VNet toggle button and pressing enter doesn't work.

This one seems more important to me, it affects all connection items, you have to press Space to run these secondary actions.

I tried to make this work but I feel like it's way too complex to implement compared to the time I wanted to spend on this feature.

What I tried is making Enter work for those buttons to the right. It's doable, but then the problem is that switching the active item with arrows does not switch the focus. The focus is kept at the search input at all times and the arrows only change which item is considered to be "active" (but in our terms, not in DOM terms).

So when you press the down arrow three times and then tab once, you are not focused on the button next to the third element. You're focused on the button next to the first element because the Tab switched focused from the search input to that button.

One way to solve it would be to switch focus when arrows are pressed, but forward any other key presses (other than arrows and Enter) to the search input and refocus it. I don't have time to experiment with it, lest it turns out it's a bad idea.

I think I'm just going to roll this into https://github.com/gravitational/teleport.e/issues/862, are you okay with that?

Once you go to the VNet panel, you can only use the tab to switch focus, arrows do nothing.

Having to switch from arrows to tab is quite unexpected, and I would probably think that this particular component doesn't support keyboard navigation :)

True, but then I don't think there's any other place which uses arrows to switch focus. So I wouldn't expect the user to assume that inside the VNet panel they can navigate to those extra buttons with arrow keys.

But if arrows still worked and they switched focus, then perhaps this would work just fine. As in, you open connections, you select VNet with arrows. You press Enter and the VNet panel gets open. You press a down arrow and it focuses on the topmost "header" in the VNet panel. Then you press Tab to get to the extra buttons.

Now that I think about it, it's probably better to support Enter for buttons even if that means that arrow keys don't change focus. I'll try to add this change in a sec.

@gzdunek Gah, I think I'll just leave it like it is. Making it work requires way too much time.

The quick fix that I did (on r7s/vnet-conn-item_fix-keyboard-nav) was adding a ref for each navigable item. On Enter press in key down of KeyboardArrowsNavigation, it'd check if the currently active navigable item contains event.target. If that was true, then it means that the user has shifted focus to a button in the currently active element and we shouldn't intercept it (aka stop propagation and run active item's handler).

But then if you use arrows to select another active item, this stops working, as the focus stays on the button from the previous active item.

I think the most sense would have starting from this (and would be probably much simpler):

Ideally, all key down inputs besides up/down arrow and enter should reset the focus back to the search input and forward that keypress to the input.

Let's leave it as it is.

avatus · 2024-04-08T16:18:59Z

web/packages/design/src/StepSlider/StepSlider.tsx

  refCallback(node: HTMLElement): void;
-  // next goes to the next step in the flow.
+  /**
+   * next goes to the next step in the flow.


is pretty flexible in that regard and the Go community isn't

typical

ravicious added 5 commits April 5, 2024 12:14

Add JSDoc comments to StepComponentProps

825c3f4

Vendor in react-merge-refs

801e232

FilterableList: Ignore case when filtering

085a944

Make VNet connection item behave like other items

9b50b58

Refactor VNet toggle to maintain focus between state transitions

50e5913

ravicious added the no-changelog Indicates that a PR does not require a changelog entry label Apr 5, 2024

ravicious requested a review from gzdunek April 5, 2024 10:59

github-actions bot added size/md ui labels Apr 5, 2024

github-actions bot requested a review from avatus April 5, 2024 11:00

gzdunek approved these changes Apr 5, 2024

View reviewed changes

ravicious commented Apr 8, 2024

View reviewed changes

ravicious added 2 commits April 8, 2024 13:09

Always enable VNet in tests

826310a

Add tests for navigating connections

c4c5926

ravicious force-pushed the r7s/vnet-conn-item branch from 71c11fe to c4c5926 Compare April 8, 2024 11:52

avatus approved these changes Apr 8, 2024

View reviewed changes

ravicious added this pull request to the merge queue Apr 9, 2024

Merged via the queue into master with commit a001c6a Apr 9, 2024
36 checks passed

ravicious deleted the r7s/vnet-conn-item branch April 9, 2024 10:21

ravicious mentioned this pull request Apr 9, 2024

Connections: Avoid creating component during rendering & remove non-reusable hook #40347

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make VNet connection item behave like other items #40249

Make VNet connection item behave like other items #40249

ravicious commented Apr 5, 2024 •

edited

Loading

gzdunek left a comment

gzdunek Apr 5, 2024

ravicious Apr 5, 2024

avatus Apr 8, 2024

gzdunek Apr 5, 2024

gzdunek Apr 5, 2024

ravicious Apr 8, 2024

ravicious Apr 9, 2024

ravicious Apr 9, 2024

gzdunek Apr 9, 2024

avatus Apr 8, 2024

Make VNet connection item behave like other items #40249

Make VNet connection item behave like other items #40249

Conversation

ravicious commented Apr 5, 2024 • edited Loading

gzdunek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravicious commented Apr 5, 2024 •

edited

Loading