bootstrap icon indicating copy to clipboard operation
bootstrap copied to clipboard

Tab active class isn't removed correctly when inside drop-down menu (5.2 regression)

Open cpsievert opened this issue 1 year ago • 16 comments

Prerequisites

Describe the issue

In the codepen link below, first click on the "C" tab (inside the dropdown "Menu"), then click on the "A" tab. Content for both the "A" tab and the "C" tab are still shown (the content for the "C" tab should not be shown).

It appears this bug was introduced in 5.2.0 (by changing the CDN links to 5.1.3 in the codepen, the issue goes away).

Reduced test cases

https://codepen.io/cpsievert/pen/OJvoYJb

What operating system(s) are you seeing the problem on?

macOS

What browser(s) are you seeing the problem on?

Chrome

What version of Bootstrap are you using?

v5.2.0

cpsievert avatar Aug 12 '22 18:08 cpsievert

you miss data-bs-target and aria-selected attributes

WinterSilence avatar Aug 14 '22 16:08 WinterSilence

Thank you for believing in me.

On Sunday, August 14, 2022, Anton @.***> wrote:

you miss data-bs-target attribute

— Reply to this email directly, view it on GitHub https://github.com/twbs/bootstrap/issues/36947#issuecomment-1214411900, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHORFCZ5QISJ7JJDFSKKADTVZEOONANCNFSM56MRRAHA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

tungphan995 avatar Aug 17 '22 16:08 tungphan995

@GeoSot has this issue actually been fixed (it doesn't appear to be)?

cpsievert avatar Sep 01 '22 16:09 cpsievert

As you didn't respond to @WinterSilence answer after 17 days, I supposed it was closed.

Please feel free to reopen it, if his answer doesn't solve your issue

GeoSot avatar Sep 01 '22 23:09 GeoSot

@WinterSilence's comment does not resolve the issue and I cannot reopen it.

cpsievert avatar Sep 04 '22 03:09 cpsievert

@patrickhlauke when you have the time, you may take a look at this. I don't think it was documented, neither seems a valid behavior to me 😕 Do you believe we should handle it?

GeoSot avatar Sep 05 '22 22:09 GeoSot

The tab JavaScript plugin does not support tabbed interfaces that contain dropdown menus, as these cause both usability and accessibility issues. From a usability perspective, the fact that the currently displayed tab’s trigger element is not immediately visible (as it’s inside the closed dropdown menu) can cause confusion. From an accessibility point of view, there is currently no sensible way to map this sort of construct to a standard WAI ARIA pattern, meaning that it cannot be easily made understandable to users of assistive technologies.

https://getbootstrap.com/docs/5.2/components/navs-tabs/#accessibility

patrickhlauke avatar Sep 05 '22 22:09 patrickhlauke

Unless I'm missing a piece of the history, it seems we explicitly didn't include tabs with dropdowns with the 5.0.0 release, then this "regression" was patched back in, despite the documentation already suggesting it shouldn't be done. Docs were then later strengthens to make it clear that it's not something that should be done. In hindsight, the regression patch should not have been merged.

patrickhlauke avatar Sep 05 '22 22:09 patrickhlauke

Note how the accessibility tree for the codepen example is borked, since role="tab" elements must be directly owned by a role="tablist" element, and you can't have links and lists like that inside a tablist.

accessibility tree representation of the codepen example, showing that the tablist has extraneous child elements that are not allowed

Basically, you can either have a proper, compliant ARIA tabbed interface (in which case you can't use unorthodox constructs like tabs with dropdowns), or you have to abandon ARIA tabs altogether. We opted for the former, as it covers the vast majority of use cases. https://www.w3.org/WAI/ARIA/apg/patterns/tabpanel/

patrickhlauke avatar Sep 05 '22 23:09 patrickhlauke

Docs were then later strengthens to make it clear that it's not something that should be done.

As a heavy user and follower of Bootstrap, I wasn't aware of this. To make it more clear, the section dedicated to using dropdowns should at least link to that accessibility note https://getbootstrap.com/docs/5.2/components/navs-tabs/#using-dropdowns

Basically, you can either have a proper, compliant ARIA tabbed interface (in which case you can't use unorthodox constructs like tabs with dropdowns), or you have to abandon ARIA tabs altogether. We opted for the former, as it covers the vast majority of use cases. https://www.w3.org/WAI/ARIA/apg/patterns/tabpanel/

I understand wanting to push for ARIA compliance, especially in terms of documentation and examples, but given the precedence for tabs within dropdowns in previous versions of Bootstrap, it doesn't seem like justification for breaking/dropping such a widely used feature. I know, for example, many users of Shiny/R Markdown (both of which builds on Bootstrap) will be surprised/upset to see this feature no longer available to them. Would you be willing to entertain a PR if I send a fix for this particular issue?

cpsievert avatar Sep 12 '22 16:09 cpsievert

Would you be willing to entertain a PR if I send a fix for this particular issue?

I'd prefer seeing this as a 3rd party extension/plugin, not a core feature.

patrickhlauke avatar Sep 12 '22 17:09 patrickhlauke

breaking/dropping such a widely used feature

personally, I'd argue it's a horrendous franken-pattern (is it a tab? is it a menu? yes!), even leaving aside accessibility concerns. I've not seen it in wide use in the wild.

patrickhlauke avatar Sep 12 '22 17:09 patrickhlauke

I'd prefer seeing this as a 3rd party extension/plugin, not a core feature.

Then Bootstrap should be deprecating it first and intentionally removing it in a future major release. The current state is misleading since that the 1st tab in a dropdown does work, but clicking on any other tab breaks in a surprising way. Also, having a look at the code and #33079, it looks as though this particular bug was in oversight in #33079 and can be fixed with one line change here:

https://github.com/twbs/bootstrap/blob/a1b1e43ddc32e8aba848968023ca0e4e11fb3a55/js/src/tab.js#L235

I'll start a PR shortly with the proposed change and why that particular line of code is wrong (along with unit tests)

cpsievert avatar Sep 13 '22 17:09 cpsievert

Then Bootstrap should be deprecating it first and intentionally removing it in a future major release

it was removed as part of the switch to 5.0.0

patrickhlauke avatar Sep 13 '22 17:09 patrickhlauke

it was removed as part of the switch to 5.0.0

Is there any record/documentation of this? Also, in that case, why is there still this logic to toggle active class on dropdown items?

https://github.com/twbs/bootstrap/blob/a1b1e43ddc32e8aba848968023ca0e4e11fb3a55/js/src/tab.js#L235

(cc contributors/reviewers on https://github.com/twbs/bootstrap/pull/33079 @GeoSot @mdo @XhmikosR)

cpsievert avatar Sep 13 '22 17:09 cpsievert

why is there still this logic to toggle active class on dropdown items

because shockingly enough we're only human and that slipped through?

patrickhlauke avatar Sep 13 '22 17:09 patrickhlauke