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

fix: react tabs focus styling #910

Merged
merged 1 commit into from
Dec 21, 2022

Conversation

nataliepina
Copy link
Collaborator

@nataliepina nataliepina commented Dec 19, 2022

Closes D2IQ-94906

Description

Background on the Issue

This initial issue arose from noticing a gray background on Tab Items in the UI after they've been clicked on. After looking into this issue, I discovered this behavior was coming from the :focus style set for the Tabs component upstream. It was an intentional style decision to handle keyboard navigation to display active focus.

:focus vs :focus-visible

After looking into the functionality of the :focus pseudo-class. I discovered :focus-visible which applies the style when elements are interacted with via keyboard and not with pointer-events such as mouse clicks. This seemed like the perfect resolution for our situation. We do not want the gray background on click but we do want it for keyboard navigation to preserve accessibility.

Resource: The :focus-visible Trick

React Tabs

After implementing :focus-visible over :focus I noticed there was inconsistent behavior where it would still apply the gray background on click. I googled around a bit and found a related open issue with react-tabs. I noticed we were a major version behind on react-tabs. I updated that to see if it would resolve the issue, however it persisted. I ended up digging more into the react-tabs documentation and I found there were specific features of the component that were contributing to the unexpected focus behavior.
The main culprit seemed to be focusTabOnClick which is by default set to true.

By default, the tab that is clicked will also be focused in the DOM. If set to false the tab will not be focused anymore.

Setting this property to false seemed to do the trick, and if you click the tab key keyboard navigation can still be initiated again. This change paired with using :focus-visible should prevent the not-so-aesthetically-pleasing gray background for pointer-events while maintaining proper accessibility features with keyboard navigation.

Outline Discoveries

Another change you'll see is where I set outline: 0 instead of outline: none. I was looking into the differences between these two as I've seen both used in the past. Thought I would share what I learned there.

According to MDN:

The CSS outline property is a shorthand property for setting one or more of the individual outline properties outline-style, outline-width and outline-color in a single declaration

More detail on this question on StackOverflow.

The only difference is the outline-width:

When the outline is 0, the outline-width is 0px
When the outline is none, the outline-width is medium
That is the only difference between the two. You can use either one, they will both display the same way (since the outline-style is none, it does not matter how wide the outline is).

Another comment mentioned this:

I opted for outline: 0; and I had to set it in both selector locations to prevent the focus ring outline.

Further reading on outline and focus accessibility: http://www.outlinenone.com/

Which issue(s) does this PR relate to?

Testing

Interact with the Tabs component with keyboard navigation (tabs and arrows) compared to clicking on tabs.

Trade-offs

Screenshots

Tabs with keyboard navigation

tab-keyboard-nav

Tabs with mouse clicks

tabs

Checklist

  • This PR is associated with a JIRA and it is mentioned in the commit message footer ("Closes …")
  • Significant changes have been tested downstream to avoid breaking changes
  • This PR contains breaking changes and states in the commit message body ("BREAKING CHANGE: …")
  • I have reviewed the changes and provided detail to the sections above

@github-actions
Copy link

github-actions bot commented Dec 19, 2022

Uffizzi Preview Environment deployment-9352

☁️ https://app.uffizzi.com//github.com/d2iq/ui-kit/pull/910

📄 View Application Logs etc.

What is Uffizzi? Learn more

Copy link
Contributor

@seialkali seialkali left a comment

Choose a reason for hiding this comment

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

Works perfectly from what I can see! Great work investigating the situation here, and thanks so much for sharing your findings!

@nataliepina nataliepina force-pushed the npina/fix/D2IQ-94906-tabs-focus-style branch from 1b0eccb to 529f226 Compare December 20, 2022 19:41
@clintonmedbery
Copy link
Contributor

Wow! What a write up!

@nataliepina nataliepina merged commit 4ab1735 into main Dec 21, 2022
@nataliepina nataliepina deleted the npina/fix/D2IQ-94906-tabs-focus-style branch December 21, 2022 14:22
@github-actions
Copy link

🎉 This PR is included in version 12.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants