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

Revamp tabs & follow ARIA 1.1 practices #33079

Merged
merged 12 commits into from Apr 6, 2022
Merged

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Feb 13, 2021

It may be a bit late to, apply it. It is almost re-written to focus on parent element in order to help in siblings filtering

P.S; The first commit was initialized on parent, which I found less demanding (and I prefer it), but I don't know if it is time for such major change

Closes #28918, closes #31345, closes #34814

Preview: https://deploy-preview-33079--twbs-bootstrap.netlify.app/docs/5.1/components/navs-tabs/#javascript-behavior

TODO:

  • Tab static

@XhmikosR
Copy link
Member

Thanks for the PR! Personally, I don't think we should land any breaking changes anymore.

@GeoSot
Copy link
Member Author

GeoSot commented Feb 15, 2021

I can only agree on this. I 've cross check the tests, and it is on bootstrap team decision

In any case, it misses test for keyDown event

@GeoSot GeoSot marked this pull request as ready for review February 15, 2021 10:22
@GeoSot GeoSot requested a review from a team as a code owner February 15, 2021 10:22
@GeoSot GeoSot force-pushed the make-tabs-aria-compliant branch 2 times, most recently from b3ff26e to 9634f91 Compare March 18, 2021 21:58
@GeoSot GeoSot added the js label Mar 26, 2021
@patrickhlauke
Copy link
Member

Just getting around to checking this https://deploy-preview-33079--twbs-bootstrap.netlify.app/docs/5.0/components/navs-tabs/#javascript-behavior

This may need some additional initialisation, as currently the "make only the active tab focusable / allow cursor keys to change between them" only works after the first time a user changes the tabs

@patrickhlauke
Copy link
Member

heads-up: ideally, not initialising on focus, but on page load itself. that was the part that stumped me (with our current JS approach). either that or we say authors need to explicitly initialise them when they're using them (same as with tooltips)

@GeoSot
Copy link
Member Author

GeoSot commented Mar 28, 2021

We have an opened issue for explicit initialization of tooltips/popover. So I 've added an auto-initialization on window load, to experiment with it, and if we proceed, we may discuss it a bit more

@patrickhlauke
Copy link
Member

going to https://deploy-preview-33079--twbs-bootstrap.netlify.app/docs/5.0/components/navs-tabs/#javascript-behavior still has all tabs by default focusable (if you start off Tab-bing through the page). After initialisation, only the currently active tab should be focusable.

If you set focus to one of the tabs, and then use cursor keys, the other tabs you land on also become non-focusable by default. but the ones you don't get to still can receive focus.

only after you go to a tabbed interface and cursor through all the tabs does it end up behaving as expected (a single tab, the active one, receives focus in the tablist)

current focus behavior

@GeoSot
Copy link
Member Author

GeoSot commented Apr 6, 2021

So,

  • initialize only active tabs on window load
  • only active tabs are focus-able
  • on keyboard right/left cycle them
  • many aria attrs addons (line 225 to 264)

@patrickhlauke
Copy link
Member

will check in a bit, but yeah basically, this is a good exemplar of how tabs should behave https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html

@patrickhlauke
Copy link
Member

The intialisation now looks good, and I see you've also made the content panels themselves focusable now (which addresses #31345)

Worth checking if it also handles disabled tabs correctly (i.e. if a tab is disabled, it should not be focusable). And thinking how to handle edge cases like a tab panel where no tab is initially visible (which is not really a sensible thing, but the script should be able to handle it somehow, as otherwise there's a risk none of the tabs would then be focusable and the whole thing would become unusable).

@GeoSot
Copy link
Member Author

GeoSot commented Apr 14, 2021

The edge case that we have a tab active & disabled, I think is out of our scope.
For now if a tab is invisible, gets the aria attributes, so when it become visible, will have the proper functionality.
The case that may need to be fix is t the keyboard navigation, when we have disabled elements.

@GeoSot GeoSot marked this pull request as draft April 25, 2021 23:06
@patrickhlauke
Copy link
Member

I lost sight of this... what's the state of play here @GeoSot ? are there outstanding parts / things that still need to be explored?

@GeoSot GeoSot force-pushed the make-tabs-aria-compliant branch from 140eb22 to 51e9f61 Compare May 5, 2021 20:54
@GeoSot
Copy link
Member Author

GeoSot commented May 5, 2021

@patrickhlauke we are almost on the same spot.

The edge case that we have a tab active & disabled, I think is out of our scope.
For now if a tab is invisible, gets the aria attributes, so when it become visible, will have the proper functionality.
The case that may need to be fix is t the keyboard navigation, when we have disabled elements.

The only difference are some changes after a help session with @alpadev , where I've changed some selectors and the way the component searches for nested selectors, to cover the co-existent dropdown with an inner tab element.

PS: IMO dropdown messes a bit the mark-up,. You can see and example on our tests, where the tab trigger is two levels deeper than the other tab-triggers

@patrickhlauke
Copy link
Member

PS: IMO dropdown messes a bit the mark-up,. You can see and example on our tests, where the tab trigger is two levels deeper than the other tab-triggers

tabs with dropdowns were always an abomination. we already strongly suggest that authors SHOULD NOT use them and we removed any examples to that effect https://getbootstrap.com/docs/5.0/components/navs-tabs/#javascript-behavior ... i guess it's too late to completely gut out the code that supports them (as that code simply can't be made to work with proper ARIA tab model that we're implementing here) @XhmikosR @mdo

@GeoSot GeoSot changed the title Make dynamic tabs follow ARIA 1.1 practices Revamp tabs & follow ARIA 1.1 practices May 11, 2021
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

Looks good to me, but would love @patrickhlauke's eyes one more time.

@patrickhlauke
Copy link
Member

Looks good to me, but would love @patrickhlauke's eyes one more time.

works as it should (mirrors structure/behaviour of https://www.w3.org/TR/wai-aria-practices-1.2/examples/tabs/tabs-1/tabs.html). if @mdo is happy with the (slightly wordy) additions to the documentation I made, this should be good to go.

v5.2.0 automation moved this from Review in progress to Reviewer approved Apr 6, 2022
@patrickhlauke
Copy link
Member

patrickhlauke commented Apr 6, 2022

@mdo your latest change to the docs is confusing. the accessibility information only relates to the javascript behaviour/plugin (when you're actually doing dynamic tabs, not just using the look/feel of tabs for otherwise basic navigation/links that go to different pages). so it should be a subsection of THAT, rather than coming before it

Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

The new "Accessibility" bit is only relevant to the plugin, so should come as a subsection of that

v5.2.0 automation moved this from Reviewer approved to Review in progress Apr 6, 2022
v5.2.0 automation moved this from Review in progress to Reviewer approved Apr 6, 2022
@patrickhlauke
Copy link
Member

Lovely, thanks @mdo

@GeoSot
Copy link
Member Author

GeoSot commented Apr 6, 2022

Thank you too guys ❤️

@patrickhlauke
Copy link
Member

Closing the loop, I filed some observations back to ARIA Practices w3c/aria-practices#2281

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.2.0
  
Done
4 participants