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(TabPanel): Added tabStop boolean prop to TabPanel #2413

Merged
merged 8 commits into from
May 25, 2021

Conversation

elliooo
Copy link
Contributor

@elliooo elliooo commented May 21, 2021

In the new Field Picker experience, a keyboard user has to tab from the Tab to the TabPanel to the first Tree in order to focus on the Tree items and begin field keyboard traversal. Ideally, I think it'd be cleaner to just tab from the Tab directly to the TreeCollection since it is the only element in the TabPanel.

Adding tabIndex as a prop allows devs to pass a non-zero value to TabPanel's tabIndex prop and thus remove it from the tab flow.

Developer Checklist ℹ️

  • ♿️ Accessibility addressed
  • 🌏 Localization & Internationalization addressed
  • 🖼 Image Snapshot coverage
  • 📚 Documentation updated
  • 🧳 Bundle size impact addressed
  • 🏁 Performance impacts addressed
  • 👾 Browsers tested
    • Chrome
    • Firefox
    • Safari
    • IE11

@elliooo
Copy link
Contributor Author

elliooo commented May 24, 2021

There's an argument to add CompatibleHTMLProps<HTMLDivElement> to Tab but I wasn't sure if that was excluded here purposefully.

@ghost ghost self-requested a review May 24, 2021 18:22
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not to ask developers to understand tabIndex and instead offer a more semantic prop. See proposed change.

packages/components/src/Tabs/TabPanel.tsx Outdated Show resolved Hide resolved
@ghost ghost requested a review from jhardy May 24, 2021 21:05
Elliot Park and others added 2 commits May 24, 2021 14:16
Co-authored-by: Luke Bowerman <34253496+lukelooker@users.noreply.github.com>
@elliooo elliooo requested a review from a user May 24, 2021 21:25
@elliooo elliooo changed the title feat(TabPanel): Added tabIndex prop to TabPanel feat(TabPanel): Added tabStop boolean prop to TabPanel May 24, 2021
ghost
ghost previously approved these changes May 24, 2021
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming @jhardy's research supports this I'm happy with it :)

@jhardy
Copy link
Contributor

jhardy commented May 25, 2021

I dug in a bit more and this seems to align closely with some updated guidance from the w3c aria best practices. This is not published on their official docs just yet, but the new guidance is for the tab key is

  • When focus moves into the tab list, places focus on the active tab element.
  • When the tab list contains the focus, moves focus to the next element in the page tab sequence outside the tablist, which is the tabpanel unless the first element containing meaningful content inside the tabpanel is focusable.

I am wondering about the prop name tabStop, that feels a bit too generic as first glance. I don't dislike it enough to block the PR. I am wondering if a more descriptive prop name but could be something we reuse elsewhere if we need to give the option to programmatically add focus.

<TabPanel makeFocusable>
<TabPanel includeInTabSequence>
<TabPanel addAsTabStop>
<TabPanel canReceiveFocus>

Not sure if those are any better or too verbose, but just a few early morning thoughts.

@ghost
Copy link

ghost commented May 25, 2021

Great point on naming... what about isTabStop? (fits with our other isX conventions)

@jhardy
Copy link
Contributor

jhardy commented May 25, 2021

that works for me!

@elliooo elliooo dismissed ghost ’s stale review via 08aca39 May 25, 2021 17:44
@elliooo elliooo requested a review from a user May 25, 2021 17:45
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@elliooo elliooo merged commit 70d6e1f into main May 25, 2021
@elliooo elliooo deleted the elliot/tab-panel-tab-index branch May 25, 2021 18:48
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.

2 participants