Add Support for disabled state tabs#6261
Conversation
|
PF4 preview: https://patternfly-react-pr-6261.surge.sh |
mcarrano
left a comment
mcarrano
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks @CooperRedhat !
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
LGTM After a11y tests pass
There was a problem hiding this comment.
Looks good! Can you also add an example of when a tab button is an <a> (in the nav and subnav element examples - https://patternfly-react-pr-6261.surge.sh/components/tabs#using-the-nav-element)? Here are the core examples - https://pf4.patternfly.org/components/tabs#using-the-nav-element
|
Note: Nav examples with anchor tags/hrefs were not updating active state, added preventDefault to click handler (which is what jumplinks do), should be working now. |
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
Looks like the examples have some duplicate ids that are causing a11y tests to fail.
| </li> | ||
| ); | ||
| })} | ||
| {filteredChildren} |
There was a problem hiding this comment.
I'm a little confused why this mapping is no longer needed, did the structure of filteredChildren change?
There was a problem hiding this comment.
The tab component used to return null, i.e. render nothing. So filtered children was just an array of objects. I needed a ref on the tab component for tooltips, so I had to forward ref and render an instance to attach a ref to.
Looks like the duplicate IDs came from wrapping a tab in a tooltip, we'll see if it passes. |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
For regular disabled nav links:
- Remove
disabled- An
<a>doesn't support thedisabledattr.
- An
- Add
aria-disabled="true"- Since the browser doesn't disable a link for you, you have to tell screen readers its disabled
- Add
tabindex="-1"- Since its disabled, you don't want users to be able to tab to it (just like tab buttons that are
disabled)
- Since its disabled, you don't want users to be able to tab to it (just like tab buttons that are
For aria-disabled tab links:
- Add
aria-disabled="true"- Same as above
- Remove the
tabindexattr- You want users to be able to tab to it to trigger the popover/tooltip
tlabaj
left a comment
tlabaj
left a comment
There was a problem hiding this comment.
Can you had a demo and some integration test to verify the disables tabs?
jessiehuff
left a comment
jessiehuff
left a comment
There was a problem hiding this comment.
So in this scenario, I don't think we actually want to add a tabIndex="-1" to the tab because it will make the tooltip completely inaccessible by keyboard. The weird thing about aria-disabled elements is they're kind of like a combination of disabled and interactive elements. Adding a tooltip makes it kind of interactive, but the main action of the tab is disabled. However, by playing with the preview, even when I take the tabIndex off the tooltip isn't displaying on focus so I'm not positive what's going on there. I also noticed that the tooltip isn't getting announced in VO. I think we need an aria-describedby="whatever the tooltip's id is". A good example to follow is our aria-disabled button with tooltip examples.
There was a problem hiding this comment.
So I noticed in reviewing that the tooltip still isn't showing up on keyboard focus. I asked Nicole if she could test that aspect too, and she noticed that if you pressed enter the tooltip will appear. That's pretty unexpected that it would appear on hover for mouse users but require interaction to appear for keyboard users. However, we also noticed that this happened with the tooltip on enabled tabs as well. Nicole came up with a good theory that it may have to do with a discrepancy between where the tooltip is put (on the li) and where the focus is (on a button). We could probably create a follow up issue to investigate.
Also, I think we only need the tabIndex="0" if there's something interactive on the disabled tab. We should probably create an issue to add this to our documentation as well.
Update: I created the issues for us. :)
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
8 participants
What: Closes #6162
Additional issues:
Tooltipinaria-disabledcase re: Original Request