Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement multi-tab-stop #7215

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

patrickkuhlmann
Copy link
Contributor

@patrickkuhlmann patrickkuhlmann commented Oct 21, 2024

draft for the following issue: #7121

I have implemented the behaviour described in the previously mentioned issue.

Known issue 1: When you alt+tab out and in onFocus is called and the manager.isFocused state triggers the effect in the selectable item moving the focus to item, but this was already in the previous version.

Known issue 2: When you shift tab onto the collection it scrolls to the last element before we programmatically move the focus.

@devongovett
Copy link
Member

I'm wondering if we could handle this the same way as useGridListItem does via the keyboardNavigationBehavior prop. We could add the same logic to useGridCell. See the changes here.

@nwidynski
Copy link
Contributor

nwidynski commented Oct 21, 2024

@devongovett I figure this means you want multi tab mode support only inside grid list hooks? Or are you referring to adding a Tab handler inside useSelectableItem() to stop propagation?

At the moment, useGridListItem() would be missing the logic from this PRs useSelectableItem() to maintain tab order when re-entering from the bottom. We would also have to forward the keyboardNavigationBehavior flag to useSelectableCollection() to disable arrow key handlers when focus is not on the cell itself.

Likely we should also find a better way to forward the modality refs we currently use, but we figured it was a starting point at least to get the idea across. While I understand the ask to exclude <ListBox /> from edit mode behavior, I want to highlight that this decision could affect the future API design of collection components.

For example, we designed our <Carousel /> around the API of <ComboBox />, with the <Popover /> being swapped for a <ScrollView />. This means that <Carousel /> would need to support both <GridList /> & <ListBox /> contexts in order to support complex focusable content while staying coherent with other component APIs.

Maybe this is a good thing though?

@patrickkuhlmann
Copy link
Contributor Author

patrickkuhlmann commented Oct 22, 2024

I'm wondering if we could handle this the same way as useGridListItem does via the keyboardNavigationBehavior prop. We could add the same logic to useGridCell. See the changes here.

hm we'd need to share information regardless because the item doesn't know how to find out the dom element after or before the collection without a ref to it, but its less cumbersome than the 3 refs I proposed.

What do you think about having the tabbing direction globally available just like the modality logic? Could it be as easy as having a onFocus listener globally that checks if relatedElement exists or not and is before or after the currentTarget/target probably in capture phase so you can use that information further down

@devongovett
Copy link
Member

Looking back at the issue thread, I'd summarize your problems with using useGridList as it exists today (with `keyboardNavigationBehavior="tab") as the following two issues:

  1. You'd like shift tabbing into a card to focus the last child within it rather than the card itself.
  2. You'd like a way to support child elements within cards that use the arrow keys, without moving between cards.

Let me know if I missed something else there. I think both of these are reasonable features to support.

I think both of these could be implemented within useGridListItem specifically, rather than useSelectableCollection / useSelectableItem. Those hooks are very low level and used in a lot of places. To support interactive children in items, we need to use the right ARIA role (typically grid), so I think these more specific higher level hooks make sense.

  1. Could be implemented in the onFocus event by marshaling focus to the last child if the relatedTarget is after the card (and not coming from another card?), otherwise to the card itself.
  2. Could be implemented by swapping from onKeyDownCapture to onKeyDown when keyboardNavigationBehavior="tab". This would allow child elements to stopPropagation when they handle arrow key events, preventing navigation to the next cell. As a bonus we could detect common native elements like <input type="text"> that should not handle the arrow keys by default (even without someone stopping propagation on them). The Escape key could also be a way to quickly get from a child back to the cell.

The reason for the capturing listener is that historically we only supported keyboardNavigationBehavior="arrow". In that mode, users use the arrow keys to move between the children inside cells rather than tabbing. Therefore it was important that the cell level arrow key handlers overrode handlers on child elements (e.g. menus). However, with keyboardNavigationBehavior="tab" this is not an issue since tab is used to move between items, and arrows only move between cells. If the user is on a component such as a text field where arrow keys move the cursor, they'd need to either shift tab or press escape to get back to the cell itself, and then press the arrow keys from there.

@nwidynski
Copy link
Contributor

nwidynski commented Oct 25, 2024

@devongovett Thanks for circling back on this! We agree useGridListItem() is a great place to implement this minimally invasive.

1.) This is doable, but we need to prevent marshaling in more scenarios. Preventing it only when previously on another card means we would marshal on shift+tab from child 1 to the cell.

2.) Can you elaborate on your reasoning as to why you want to deal with focus propagation inside child elements? We were wondering if the cell could stopPropagation when keyboardNavigationBehavior="tab" and ref.current !== document.activeElement.

3.) We are also still left with the problem of marshaling when relatedTarget === null due to focus restoration from alt+tab or when clicking on a child from outside the collection. This is an existing issue in useSelectableCollection() but we would like to solve it in this PR as well if possible.

As you suggested, elements utilizing the Esc interaction should stopPropagation. We're unsure whether you want to integrate this out of the box for RAC or leave this up to user land.

@nwidynski
Copy link
Contributor

@devongovett Just opened #7277 for a working draft on the requested behavior. Let me know what you guys think!

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.

3 participants