feat: Adds single tab stop util #111
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #111 +/- ##
==========================================
- Coverage 99.23% 93.88% -5.35%
==========================================
Files 27 29 +2
Lines 652 720 +68
Branches 173 193 +20
==========================================
+ Hits 647 676 +29
- Misses 5 44 +39 ☔ View full report in Codecov by Sentry. |
katiegeorge
requested review from
georgylobko and
pan-kot
and removed request for
a team
| focus(): void; | ||
| } | ||
|
|
||
| export default function useForwardFocus( |
There was a problem hiding this comment.
Why this util is needed here? I see it is only used in a button from test page, but not sure why.
There was a problem hiding this comment.
Good call, I copied it over from button group, but seems it is not needed here.
| * @param container Container node | ||
| * @param target Node that is checked to be a descendant of the container | ||
| */ | ||
| export function nodeBelongs(container: Node | null, target: Node | EventTarget | null): boolean { |
There was a problem hiding this comment.
let's take an action to reuse the nodeBelongs, handleKey, isNode, and other utils that are now part of the component-toolkit in the components and, potentially, other repos.
|
|
||
| export default function SingleTabStopKeyboardNav({ | ||
| children, | ||
| matchId, |
There was a problem hiding this comment.
the matchId name is not very telling - it is actually a selector that finds interactive elements that can be focus targets
| @@ -0,0 +1,48 @@ | |||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
Should we call this focus utils instead? This file lives in the focus-lock folder, but we did not carry over the focus lock component to the toolkit.
| function getNextFocusTarget(): null | HTMLElement { | ||
| if (containerObjectRef.current) { | ||
| const buttons: HTMLButtonElement[] = Array.from(containerObjectRef.current.querySelectorAll(matchId)); | ||
| return buttons.find(button => button.dataset.itemid === focusedIdRef.current) ?? buttons[0] ?? null; |
There was a problem hiding this comment.
This integration is rather quite implicit: the interactive elements must have the data-itemid and must be all assigned a selector. I wonder if this can be simplified. For instance, here we can find all interactive elements using getAllFocusables and then instead of matchId expose some matchFocusTarget(focusedItemId, focusableElements): null | Element - this way the matching can be performed on the consumer end and can use any mechanism so not relying on assigning the specific data- attribute.
| import { KeyCode } from '../../../src/internal/keycode'; | ||
| import { getAllFocusables } from '../../../src/internal/focus-lock/utils'; | ||
|
|
||
| export default function SingleTabStopKeyboardNav({ |
There was a problem hiding this comment.
Would it make more sense to introduce this util in the components first to ensure the button group and tabs can actually use it? Meanwhile, a PR to the toolkit can still carry over the necessary internal utils.
There was a problem hiding this comment.
It does work with button group. Tabs is unfortunately a bit more complicated and I'm not sure it would make sense to carry this over there.
We can, alternatively, not pull out the KeyboardNav util and just reuse that portion in both button group and support prompt group for now.
|
Closed in favor of #112 |
Issue #, if available:
Description of changes:
This PR adds two new utils:
SingleTabStop(in the form ofuseSingleTabStopNavigation,SingleTabStopProviderandSingleTabStopNavigationAPI), andSingleTabStopKeyboardNav.SingleTabStop is currently being use in the
componentsrepository inside Tabs, Table (for grid navigation), and Button Group.It needs to be added here instead due to the same utility being needed for the upcoming
SupportPromptGrouprelease. SupportPromptGroup is in thechat-componentsrepository.SingleTabStopKeyboardNavcan be reused inside components which use uni-directional keyboard navigation, currently Tabs and ButtonGroup (and soon, SupportPromptGroup).By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.