Handle disabled focused tabs with tab JavaScript plugin

[julien-deramond]

Since 135b9cd, disabled tabs with tab JavaScript plugin don't seem to be handled anymore: they can be focused with the keyboard when disabled.
I've read the discussion in #33079 but I was unsure of the current state.

This PR tries to reintegrate this behavior (heavily draft atm) or at least shows the issue.

Note: with this current code, it doesn't work if the first tab is disabled and there are some issues when they are active + disabled.

If we use this PR to fix this use case

• Add tests
• Handle active + disabled
• First tab is disabled
• Only disabled tabs

Live preview

/cc @GeoSot

[julien-deramond]

Some tests were added via 437abdb.

Coverage before when launching only the Tab tests

Statements : 32.53% ( 690/2121 )
Branches : 19.18% ( 199/1037 )
Functions : 21.28% ( 93/437 )
Lines : 32.94% ( 685/2079 )

New coverage when launching only the Tab tests

Statements : 32.62% ( 691/2118 )
Branches : 19.47% ( 202/1037 )
Functions : 21.51% ( 94/437 )
Lines : 33.01% ( 685/2075 )

Proposal: Let's focus on this first fix for the 5.2.0 and try for a 5.2.1 or 5.3.0 to focus on the edge cases: active + disabled, first tab is disabled, only disabled tabs, etc. that I haven't tested yet in details. Please tell me your thoughts about this approach.

Note: You can retrieve 437abdb387dced04d2e8e12c124d6021cd5ac209 in local (just before 639333d) which contains a previous commit allowing you to play with a use case in the documentation.

[GeoSot]

Plus I liked the docs addon (removed by 639333d) 😢

[julien-deramond]

Plus I liked the docs addon (removed by 639333d) 😢

In order to have consistent examples in the documentation, in the same spirit, I've added in 3bdce71 a disabled tab for each example of the "JavaScript behavior" section as a last tab. Always using an example with .disabled and not the attribute.

[GeoSot]

In order to have consistent examples in the documentation, in the same spirit, I've added in 3bdce71 a disabled tab for each example of the "JavaScript behavior" section as a last tab. Always using an example with .disabled and not the attribute.

Yes, much much better. I want a favor though 👼
As a matter of fact, now, the disabled tabs can be placed in any order (except first), so some of them can change positions around the changed examples, can be. right?

[julien-deramond]

Yes, much much better. I want a favor though 👼 As a matter of fact, now, the disabled tabs can be placed in any order (except first), so some of them can change positions around the changed examples, can be. right?

I've changed the position for the vertical example in 3656437. I haven't changed the first 3 examples because IMHO they are working together to explain some concepts; I tried to change it and it seemed to me confusing to have a different tabs order while reading this part of the documentation. But if you still prefer a change of those 3 examples as well to shuffle those disabled tabs, no problem, I can do it.

[julien-deramond]

Probably to tackle in another issue but I'd say this button should only need the disabled attribute as in Buttons > Disabled state. I can't remove .disabled right now because the text becomes gray thanks to that class.

[GeoSot]

probably css ?

[julien-deramond]

Yes. The current rule is:

&.disabled {
    color: var(--#{$prefix}nav-link-disabled-color);
    background-color: transparent;
    border-color: transparent;
}

We could add :disabled as a selector as well for example. But I'm waiting for Patrick's suggestion.

[patrickhlauke]

+1 to using actual disabled attribute on <button>, but also allowing the same to happen with .disabled when author chose to use link or something

[julien-deramond]

Thanks for the feedback Patrick. Following the discussion in this comment, d613fa0 uses disabled attribute on <button> instead of .disabled. CSS rules have been added in _nav.scss to cover this use case.

[GeoSot]

/CC @patrickhlauke have you got any other suggestions here? 😄

[patrickhlauke]

Ok, looking purely at the example https://deploy-preview-36169--twbs-bootstrap.netlify.app/docs/5.1/components/navs-tabs/#javascript-behavior this looks decent and doing what it should. assuming that it handles it correctly if the disabled tab is the second-to-last or something, skipping it and setting focus to the one after it when you navigate through?

And yes I like the addition directly in the docs to have a disabled tab there in the example

[julien-deramond]

assuming that it handles it correctly if the disabled tab is the second-to-last or something, skipping it and setting focus to the one after it when you navigate through?

Yes it is. I chose to show it in the doc only for the vertical pills example to keep a certain consistency between all horizontal examples in the page. This is also covered in the unit tests.

[mdo]

@julien-deramond Let me know if/when this is good to merge for you :)

[julien-deramond]

@julien-deramond Let me know if/when this is good to merge for you :)

This is good to merge. I'll track the remaining edge cases in an issue for the next 5.2.x version.