Skip to content

Conversation

@cpsievert
Copy link
Contributor

@cpsievert cpsievert commented Sep 14, 2022

Closes #36947 (a regression introduced by #33079, cc @GeoSot)

As the codepen in #36947 demonstrates, when clicking on the 2nd tab in a dropdown, all the tabs within the dropdown are marked with an active class. This happens because, the current logic in _toggleDropdown() toggles the active class on the first tab within the dropdown when it shouldn't be

@cpsievert cpsievert requested a review from a team as a code owner September 14, 2022 15:52
@cpsievert cpsievert force-pushed the fix-dropdown-active-tab branch from 704d222 to 58be58d Compare September 14, 2022 16:01
@GeoSot GeoSot force-pushed the fix-dropdown-active-tab branch from 3d03976 to 7ff0090 Compare September 21, 2022 07:36
@GeoSot
Copy link
Member

GeoSot commented Sep 21, 2022

Looking back the commits history,
it seems that Tab was handling only the removal of active class

bootstrap/js/src/tab.js

Lines 116 to 120 in 63d38b1

const dropdownChild = SelectorEngine.findOne(SELECTOR_DROPDOWN_ACTIVE_CHILD, active.parentNode)
if (dropdownChild) {
dropdownChild.classList.remove(CLASS_NAME_ACTIVE)
}

So if we want to bring back the exact old behavior, I think it would be better to rename the const SELECTOR_DROPDOWN_ITEM, to
const SELECTOR_DROPDOWN_ITEM_ACTIVE = '.dropdown-item.active' and use it, instead of remove the handling

@cpsievert what do you think about this?

@cpsievert
Copy link
Contributor Author

@GeoSot I don't think that's necessary since, in the new implementation, by the time we reach _toggleDropdown(), the .active class will already be added/removed from the tab via

bootstrap/js/src/tab.js

Lines 103 to 108 in 37e2e7e

_activate(element, relatedElem) {
if (!element) {
return
}
element.classList.add(CLASS_NAME_ACTIVE)

and

bootstrap/js/src/tab.js

Lines 129 to 134 in 37e2e7e

_deactivate(element, relatedElem) {
if (!element) {
return
}
element.classList.remove(CLASS_NAME_ACTIVE)

@GeoSot
Copy link
Member

GeoSot commented Sep 21, 2022

I 've checked it and you are right. Thanks, @cpsievert 👍

@GeoSot GeoSot force-pushed the fix-dropdown-active-tab branch from 7ff0090 to 7840191 Compare September 21, 2022 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

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

3 participants