Skip to content

Revamp tabs & follow ARIA 1.1 practices #33079

Merged
mdo merged 12 commits into
twbs:mainfrom
GeoSot:make-tabs-aria-compliant
Apr 6, 2022
Merged

Revamp tabs & follow ARIA 1.1 practices #33079
mdo merged 12 commits into
twbs:mainfrom
GeoSot:make-tabs-aria-compliant

Conversation

@GeoSot

@GeoSot GeoSot commented Feb 13, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

@GeoSot

GeoSot commented Feb 15, 2021

Copy link
Copy Markdown
Member Author

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 from a7aa7a4 to 4b5d1fc Compare February 23, 2021 23:54
@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 force-pushed the make-tabs-aria-compliant branch from 9634f91 to ca9a682 Compare March 24, 2021 00:46
@GeoSot GeoSot added the js label Mar 26, 2021
@patrickhlauke

Copy link
Copy Markdown
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
Copy Markdown
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

GeoSot commented Mar 28, 2021

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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 GeoSot force-pushed the make-tabs-aria-compliant branch from f367957 to 7073d83 Compare April 6, 2021 00:08
@GeoSot

GeoSot commented Apr 6, 2021

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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
Copy Markdown
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

GeoSot commented Apr 14, 2021

Copy link
Copy Markdown
Member Author

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 force-pushed the make-tabs-aria-compliant branch from 6691c55 to cbb80c2 Compare April 25, 2021 23:03
@GeoSot GeoSot marked this pull request as draft April 25, 2021 23:06
@GeoSot GeoSot force-pushed the make-tabs-aria-compliant branch from cbb80c2 to d6a5330 Compare April 27, 2021 09:15
@patrickhlauke

Copy link
Copy Markdown
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

GeoSot commented May 5, 2021

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
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
@GeoSot GeoSot force-pushed the make-tabs-aria-compliant branch from 51e9f61 to 70d991d Compare May 18, 2021 23:52
@GeoSot GeoSot force-pushed the make-tabs-aria-compliant branch from 9c33b85 to 91380e3 Compare March 28, 2022 22:02

@mdo mdo left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

@patrickhlauke

patrickhlauke commented Apr 6, 2022

Copy link
Copy Markdown
Member

@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

@patrickhlauke patrickhlauke left a comment

Copy link
Copy Markdown
Member

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

@patrickhlauke

Copy link
Copy Markdown
Member

Lovely, thanks @mdo

@GeoSot

GeoSot commented Apr 6, 2022

Copy link
Copy Markdown
Member Author

Thank you too guys ❤️

@patrickhlauke

Copy link
Copy Markdown
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

None yet

Development

Successfully merging this pull request may close these issues.

Tabs Component Keyboard Navigation [Accessibility] nav tabs : tabpanel focus issues make dynamic tabs actually ARIA 1.1 practices compliant

4 participants