feat(tabs): traverse tabs with arrow keys

[weronikaolejniczak]

Summary

Both the WCAG and MDN specifications for tabs specify that they should be within a tab group and traversed via arrow keys.

Screen.Recording.2024-11-05.at.12.41.35.mov

I've noticed there are 2 ways the tabs are used:

1. either with EuiTabbedContent (that provides a tabpanel; EuiTabs and EuiTab accessible from the same level),
2. or using EuiTabs + EuiTab directly (composition through children prop).

I decided to put the logic inside EuiTabs as its the common point between the 2 use cases and it makes sense for that wrapper to traverse the nodes within and manipulate the DOM (as opposed to traversing them inside EuiTab from the parentElement or handling in EuiTabbedContent).

closes #8005

QA

I tested the changes in the Storybook, the docs and against Kibana (the provided use case: Main Menu > Management > Dev Tools) with VoiceOver.

General checklist

• Browser QA
  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Checked for accessibility including keyboard-only and screenreader modes
• Docs site QA
  • Added documentation
  • Props have proper autodocs (using @default if default values are missing) and playground toggles
  • Checked Code Sandbox works for any docs examples
• Code quality checklist
  • Added or updated jest and cypress tests
  • Updated visual regression tests (not applicable)
• Release checklist
  • A changelog entry exists and is marked appropriately.
  • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
• Designer checklist
  • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

[cee-chen]

Suggested change

	if (!tabs) return;
	if (!tabs.length) return;

querySelectorAll will return an empty list/array which passes a truthy check, so this current early return won't work unless we check the length

[weronikaolejniczak]

You're correct! I was focusing on safe-guarding against elementRef.current being null but didn't think of an empty array. Thanks, Cee! 🙌🏻 I'll do:

Suggested change

	if (!tabs) return;
	if (!tabs?.length) return;

[cee-chen]

I actually really like querying for the tab role (as opposed to using our tabbable library. It's possible for consumers to insert wildcard children into <EuiTabs> (although it's not accessibly correct to do so), so this query selector ensures that only the correct tab children receive arrow key navigation.

That being said, I'd consider modifying this selector to exclude disabled and inert tabs, so they're not included in arrow key navigation:

Suggested change

	const tabs = currentEl?.querySelectorAll('[role="tab"]');
	const tabs = currentEl?.querySelectorAll('[role="tab"]:not(:disabled, [inert])');

The tabbable library additionally checks for visibility: hidden or display: none, but IMO we don't need to go that far here for those edge cases.

[cee-chen]

Oh, also worth noting you can avoid the as casting below by passing a generic to querySelectorAll:

Suggested change

	const tabs = currentEl?.querySelectorAll('[role="tab"]');
	const tabs = currentEl?.querySelectorAll<HTMLButtonElement>('[role="tab"]:not(:disabled, [inert])');

(prettier will probably have Opinions about splitting this into multiple lines at that point, so deliberately leaving the indentation wrong on this suggestion)

[weronikaolejniczak]

@cee-chen oh, good to know about the tabbable library, thanks, Cee! 🙌🏻

Skipping navigation for the disabled and inert elements makes total sense 👌🏻

[cee-chen]

Why add this as a useEffect as opposed to adding an onKeyDown={handleKeyDown} onto the div?

[weronikaolejniczak]

@cee-chen no idea, honestly. Might've been too much in the vanilla JS context after reading MDN docs. Event handler is a much cleaner solution, I'll do that 👌🏻 Thanks, Cee!

[cee-chen]

[microperf] We should memoize this in a useCallback so it's not reinstantiated every rerender

Suggested change

	const handleKeyDown = (event: KeyboardEvent) => {
	const handleKeyDown = useCallback((event: KeyboardEvent) => {

[weronikaolejniczak]

@cee-chen handleKeyDown doesn't perform any heavy calculations. It's not used in the useEffect's dependency array and it's not passed to a memo-ed component. Re-rendering a bunch of tabs doesn't seem too expensive either. And memoizing a function comes at a cost.

That being said, I'll test the performance in Profiler with and without the useCallback, share the results and we can decide together whether to go for it or skip it. Unless you're strong on this? Or I won't, I don't know what's up with React DevTools recently but Profiler tab doesn't show for me 😢

[cee-chen]

Can we add a Cypress test checking that this still works if consumers completely change their tabs dynamically? Or what happens if they dynamically disable some tabs?

[weronikaolejniczak]

@cee-chen that's a very good idea! I didn't even think of a dynamic tab use case. Could you share an example from Kibana? Or maybe you have an idea of what that test could look like?

[cee-chen]

I've never seen this shortcut math for looping back around to the first/last item in the array before 🤯 Can we add a quick inline code comment here explaining what's happening? e.g.,

Suggested change

	const previousIndex = (currentIndex - 1 + tabs.length) % tabs.length;
	const previousIndex = (currentIndex - 1 + tabs.length) % tabs.length; // % loops back to the last item in the array

+ same for the nextIndex % logic

[weronikaolejniczak]

@cee-chen A neat math trick ✨ but you're right, it's not too clear. We could just do:

const previousIndex = (currentIndex === 0 ? tabs.length : currentIndex) - 1;

and

const nextIndex = currentIndex === tabs.length ? 0 : currentIndex + 1

What do you think?

[kibanamachine]

Preview staging links for this PR:

• Docs site: https://eui.elastic.co/pr_8116/
• Storybook: https://eui.elastic.co/pr_8116/storybook
• New docs: https://eui.elastic.co/pr_8116/new-docs

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: a0cb507

History

• 💚 Build #3028 succeeded 63b23a6
• 💚 Build #3018 succeeded bb63b1c
• 💔 Build #3014 failed 6d1b7f0
• 💔 Build #3013 failed e4511ae
• 💔 Build #3011 failed fa2af92