Skip to content

Commit

Permalink
Dynamic tabs: use buttons rather than links (#32630)
Browse files Browse the repository at this point in the history
* Dynamic tabs: use buttons rather than links

- change docs
- add mention that tabs should be <button> elements
- tweak styles to neutralise border and background

* Update js unit and visual test accordingly

- replace links with buttons
- make one specific test that uses links instead of buttons, as we still want to support it despite it being non-semantically appropriate
- Leaving a couple of tests for now. The test for removed tabs should be redone so that tabs are removed programmatically (as the approach of having that close button inside the link is invalid and broken markup). The test for dropdowns should be removed together we actually ripping out the handling for dropdowns in the tab.js code (arguably a breaking change, though we discouraged this for a few versions and effectively "deprecated" it)

* Add isolation:isolate to prevent focus being overlapped

#32630 (comment)
  • Loading branch information
patrickhlauke committed Feb 9, 2021
1 parent c93d754 commit 96be06e
Show file tree
Hide file tree
Showing 4 changed files with 148 additions and 122 deletions.
124 changes: 74 additions & 50 deletions js/tests/unit/tab.spec.js
Expand Up @@ -21,15 +21,39 @@ describe('Tab', () => {
})

describe('show', () => {
it('should activate element by tab id', done => {
it('should activate element by tab id (using buttons, the preferred semantic way)', done => {
fixtureEl.innerHTML = [
'<ul class="nav">',
'<ul class="nav" role="tablist">',
' <li><button type="button" data-bs-target="#home" role="tab">Home</button></li>',
' <li><button type="button" id="triggerProfile" data-bs-target="#profile" role="tab">Profile</button></li>',
'</ul>',
'<ul>',
' <li id="home" role="tabpanel"></li>',
' <li id="profile" role="tabpanel"></li>',
'</ul>'
].join('')

const profileTriggerEl = fixtureEl.querySelector('#triggerProfile')
const tab = new Tab(profileTriggerEl)

profileTriggerEl.addEventListener('shown.bs.tab', () => {
expect(fixtureEl.querySelector('#profile').classList.contains('active')).toEqual(true)
expect(profileTriggerEl.getAttribute('aria-selected')).toEqual('true')
done()
})

tab.show()
})

it('should activate element by tab id (using links for tabs - not ideal, but still supported)', done => {
fixtureEl.innerHTML = [
'<ul class="nav" role="tablist">',
' <li><a href="#home" role="tab">Home</a></li>',
' <li><a id="triggerProfile" role="tab" href="#profile">Profile</a></li>',
' <li><a id="triggerProfile" href="#profile" role="tab">Profile</a></li>',
'</ul>',
'<ul>',
' <li id="home"></li>',
' <li id="profile"></li>',
' <li id="home" role="tabpanel"></li>',
' <li id="profile" role="tabpanel"></li>',
'</ul>'
].join('')

Expand All @@ -48,12 +72,12 @@ describe('Tab', () => {
it('should activate element by tab id in ordered list', done => {
fixtureEl.innerHTML = [
'<ol class="nav nav-pills">',
' <li><a href="#home">Home</a></li>',
' <li><a id="triggerProfile" href="#profile">Profile</a></li>',
' <li><button type="button" data-bs-target="#home" role="tab">Home</button></li>',
' <li><button type="button" id="triggerProfile" href="#profile" role="tab">Profile</button></li>',
'</ol>',
'<ol>',
' <li id="home"></li>',
' <li id="profile"></li>',
' <li id="home" role="tabpanel"></li>',
' <li id="profile" role="tabpanel"></li>',
'</ol>'
].join('')

Expand All @@ -71,10 +95,10 @@ describe('Tab', () => {
it('should activate element by tab id in nav list', done => {
fixtureEl.innerHTML = [
'<nav class="nav">',
' <a href="#home">Home</a>',
' <a id="triggerProfile" href="#profile">Profile</a>',
' <button type="button" data-bs-target="#home" role="tab">Home</button>',
' <button type="button" id="triggerProfile" data-bs-target="#profile" role="tab">Profile</a>',
'</nav>',
'<nav><div id="home"></div><div id="profile"></div></nav>'
'<div><div id="home" role="tabpanel"></div><div id="profile" role="tabpanel"></div></div>'
].join('')

const profileTriggerEl = fixtureEl.querySelector('#triggerProfile')
Expand All @@ -90,11 +114,11 @@ describe('Tab', () => {

it('should activate element by tab id in list group', done => {
fixtureEl.innerHTML = [
'<div class="list-group">',
' <a href="#home">Home</a>',
' <a id="triggerProfile" href="#profile">Profile</a>',
'<div class="list-group" role="tablist">',
' <button type="button" data-bs-target="#home" role="tab">Home</button>',
' <button type="button" id="triggerProfile" data-bs-target="#profile" role="tab">Profile</button>',
'</div>',
'<nav><div id="home"></div><div id="profile"></div></nav>'
'<div><div id="home" role="tabpanel"></div><div id="profile" role="tabpanel"></div></div>'
].join('')

const profileTriggerEl = fixtureEl.querySelector('#triggerProfile')
Expand Down Expand Up @@ -135,16 +159,16 @@ describe('Tab', () => {
it('should not fire shown when tab is already active', done => {
fixtureEl.innerHTML = [
'<ul class="nav nav-tabs" role="tablist">',
' <li class="nav-item" role="presentation"><a href="#home" class="nav-link active" role="tab">Home</a></li>',
' <li class="nav-item" role="presentation"><a href="#profile" class="nav-link" role="tab">Profile</a></li>',
' <li class="nav-item" role="presentation"><button type="button" data-bs-target="#home" class="nav-link active" role="tab" aria-selected="true">Home</button></li>',
' <li class="nav-item" role="presentation"><button type="button" href="#profile" class="nav-link" role="tab">Profile</button></li>',
'</ul>',
'<div class="tab-content">',
' <div class="tab-pane active" id="home" role="tabpanel"></div>',
' <div class="tab-pane" id="profile" role="tabpanel"></div>',
'</div>'
].join('')

const triggerActive = fixtureEl.querySelector('a.active')
const triggerActive = fixtureEl.querySelector('button.active')
const tab = new Tab(triggerActive)

triggerActive.addEventListener('shown.bs.tab', () => {
Expand All @@ -161,16 +185,16 @@ describe('Tab', () => {
it('should not fire shown when tab is disabled', done => {
fixtureEl.innerHTML = [
'<ul class="nav nav-tabs" role="tablist">',
' <li class="nav-item" role="presentation"><a href="#home" class="nav-link active" role="tab">Home</a></li>',
' <li class="nav-item" role="presentation"><a href="#profile" class="nav-link disabled" role="tab">Profile</a></li>',
' <li class="nav-item" role="presentation"><button type="button" data-bs-target="#home" class="nav-link active" role="tab" aria-selected="true">Home</button></li>',
' <li class="nav-item" role="presentation"><button type="button" data-bs-target="#profile" class="nav-link disabled" role="tab">Profile</button></li>',
'</ul>',
'<div class="tab-content">',
' <div class="tab-pane active" id="home" role="tabpanel"></div>',
' <div class="tab-pane" id="profile" role="tabpanel"></div>',
'</div>'
].join('')

const triggerDisabled = fixtureEl.querySelector('a.disabled')
const triggerDisabled = fixtureEl.querySelector('button.disabled')
const tab = new Tab(triggerDisabled)

triggerDisabled.addEventListener('shown.bs.tab', () => {
Expand All @@ -187,8 +211,8 @@ describe('Tab', () => {
it('show and shown events should reference correct relatedTarget', done => {
fixtureEl.innerHTML = [
'<ul class="nav nav-tabs" role="tablist">',
' <li class="nav-item" role="presentation"><a href="#home" class="nav-link active" role="tab">Home</a></li>',
' <li class="nav-item" role="presentation"><a id="triggerProfile" href="#profile" class="nav-link" role="tab">Profile</a></li>',
' <li class="nav-item" role="presentation"><button type="button" data-bs-target="#home" class="nav-link active" role="tab" aria-selected="true">Home</button></li>',
' <li class="nav-item" role="presentation"><button type="button" id="triggerProfile" data-bs-target="#profile" class="nav-link" role="tab">Profile</button></li>',
'</ul>',
'<div class="tab-content">',
' <div class="tab-pane active" id="home" role="tabpanel"></div>',
Expand All @@ -200,13 +224,13 @@ describe('Tab', () => {
const secondTab = new Tab(secondTabTrigger)

secondTabTrigger.addEventListener('show.bs.tab', ev => {
expect(ev.relatedTarget.hash).toEqual('#home')
expect(ev.relatedTarget.getAttribute('data-bs-target')).toEqual('#home')
})

secondTabTrigger.addEventListener('shown.bs.tab', ev => {
expect(ev.relatedTarget.hash).toEqual('#home')
expect(ev.relatedTarget.getAttribute('data-bs-target')).toEqual('#home')
expect(secondTabTrigger.getAttribute('aria-selected')).toEqual('true')
expect(fixtureEl.querySelector('a:not(.active)').getAttribute('aria-selected')).toEqual('false')
expect(fixtureEl.querySelector('button:not(.active)').getAttribute('aria-selected')).toEqual('false')
done()
})

Expand All @@ -215,13 +239,13 @@ describe('Tab', () => {

it('should fire hide and hidden events', done => {
fixtureEl.innerHTML = [
'<ul class="nav">',
' <li><a href="#home">Home</a></li>',
' <li><a href="#profile">Profile</a></li>',
'<ul class="nav" role="tablist">',
' <li><button type="button" data-bs-target="#home" role="tab">Home</button></li>',
' <li><button type="button" data-bs-target="#profile">Profile</button></li>',
'</ul>'
].join('')

const triggerList = fixtureEl.querySelectorAll('a')
const triggerList = fixtureEl.querySelectorAll('button')
const firstTab = new Tab(triggerList[0])
const secondTab = new Tab(triggerList[1])

Expand All @@ -232,12 +256,12 @@ describe('Tab', () => {

triggerList[0].addEventListener('hide.bs.tab', ev => {
hideCalled = true
expect(ev.relatedTarget.hash).toEqual('#profile')
expect(ev.relatedTarget.getAttribute('data-bs-target')).toEqual('#profile')
})

triggerList[0].addEventListener('hidden.bs.tab', ev => {
expect(hideCalled).toEqual(true)
expect(ev.relatedTarget.hash).toEqual('#profile')
expect(ev.relatedTarget.getAttribute('data-bs-target')).toEqual('#profile')
done()
})

Expand All @@ -246,13 +270,13 @@ describe('Tab', () => {

it('should not fire hidden when hide is prevented', done => {
fixtureEl.innerHTML = [
'<ul class="nav">',
' <li><a href="#home">Home</a></li>',
' <li><a href="#profile">Profile</a></li>',
'<ul class="nav" role="tablist">',
' <li><button type="button" data-bs-target="#home" role="tab">Home</button></li>',
' <li><button type="button" data-bs-target="#profile" role="tab">Profile</button></li>',
'</ul>'
].join('')

const triggerList = fixtureEl.querySelectorAll('a')
const triggerList = fixtureEl.querySelectorAll('button')
const firstTab = new Tab(triggerList[0])
const secondTab = new Tab(triggerList[1])
const expectDone = () => {
Expand Down Expand Up @@ -423,8 +447,8 @@ describe('Tab', () => {
it('should create dynamically a tab', done => {
fixtureEl.innerHTML = [
'<ul class="nav nav-tabs" role="tablist">',
' <li class="nav-item" role="presentation"><a href="#home" class="nav-link active" role="tab">Home</a></li>',
' <li class="nav-item" role="presentation"><a id="triggerProfile" data-bs-toggle="tab" href="#profile" class="nav-link" role="tab">Profile</a></li>',
' <li class="nav-item" role="presentation"><button type="button" data-bs-target="#home" class="nav-link active" role="tab" aria-selected="true">Home</button></li>',
' <li class="nav-item" role="presentation"><button type="button" id="triggerProfile" data-bs-toggle="tab" data-bs-target="#profile" class="nav-link" role="tab">Profile</button></li>',
'</ul>',
'<div class="tab-content">',
' <div class="tab-pane active" id="home" role="tabpanel"></div>',
Expand Down Expand Up @@ -469,15 +493,15 @@ describe('Tab', () => {
it('should handle nested tabs', done => {
fixtureEl.innerHTML = [
'<nav class="nav nav-tabs" role="tablist">',
' <a id="tab1" href="#x-tab1" class="nav-link" data-bs-toggle="tab" role="tab" aria-controls="x-tab1">Tab 1</a>',
' <a href="#x-tab2" class="nav-link active" data-bs-toggle="tab" role="tab" aria-controls="x-tab2" aria-selected="true">Tab 2</a>',
' <a href="#x-tab3" class="nav-link" data-bs-toggle="tab" role="tab" aria-controls="x-tab3">Tab 3</a>',
' <button type="button" id="tab1" data-bs-target="#x-tab1" class="nav-link" data-bs-toggle="tab" role="tab" aria-controls="x-tab1">Tab 1</button>',
' <button type="button" data-bs-target="#x-tab2" class="nav-link active" data-bs-toggle="tab" role="tab" aria-controls="x-tab2" aria-selected="true">Tab 2</button>',
' <button type="button" data-bs-target="#x-tab3" class="nav-link" data-bs-toggle="tab" role="tab" aria-controls="x-tab3">Tab 3</button>',
'</nav>',
'<div class="tab-content">',
' <div class="tab-pane" id="x-tab1" role="tabpanel">',
' <nav class="nav nav-tabs" role="tablist">',
' <a href="#nested-tab1" class="nav-link active" data-bs-toggle="tab" role="tab" aria-controls="x-tab1" aria-selected="true">Nested Tab 1</a>',
' <a id="tabNested2" href="#nested-tab2" class="nav-link" data-bs-toggle="tab" role="tab" aria-controls="x-profile">Nested Tab2</a>',
' <button type="button" data-bs-target="#nested-tab1" class="nav-link active" data-bs-toggle="tab" role="tab" aria-controls="x-tab1" aria-selected="true">Nested Tab 1</button>',
' <button type="button" id="tabNested2" data-bs-target="#nested-tab2" class="nav-link" data-bs-toggle="tab" role="tab" aria-controls="x-profile">Nested Tab2</button>',
' </nav>',
' <div class="tab-content">',
' <div class="tab-pane active" id="nested-tab1" role="tabpanel">Nested Tab1 Content</div>',
Expand Down Expand Up @@ -509,8 +533,8 @@ describe('Tab', () => {
it('should not remove fade class if no active pane is present', done => {
fixtureEl.innerHTML = [
'<ul class="nav nav-tabs" role="tablist">',
' <li class="nav-item" role="presentation"><a id="tab-home" href="#home" class="nav-link" data-bs-toggle="tab" role="tab">Home</a></li>',
' <li class="nav-item" role="presentation"><a id="tab-profile" href="#profile" class="nav-link" data-bs-toggle="tab" role="tab">Profile</a></li>',
' <li class="nav-item" role="presentation"><button type="button" id="tab-home" data-bs-target="#home" class="nav-link" data-bs-toggle="tab" role="tab">Home</button></li>',
' <li class="nav-item" role="presentation"><button type="button" id="tab-profile" data-bs-target="#profile" class="nav-link" data-bs-toggle="tab" role="tab">Profile</button></li>',
'</ul>',
'<div class="tab-content">',
' <div class="tab-pane fade" id="home" role="tabpanel"></div>',
Expand Down Expand Up @@ -547,10 +571,10 @@ describe('Tab', () => {
fixtureEl.innerHTML = [
'<ul class="nav nav-tabs" role="tablist">',
' <li class="nav-item" role="presentation">',
' <a class="nav-link nav-tab" href="#home" role="tab" data-bs-toggle="tab">Home</a>',
' <button type="button" class="nav-link nav-tab" data-bs-target="#home" role="tab" data-bs-toggle="tab">Home</button>',
' </li>',
' <li class="nav-item" role="presentation">',
' <a id="secondNav" class="nav-link nav-tab" href="#profile" role="tab" data-bs-toggle="tab">Profile</a>',
' <button type="button" id="secondNav" class="nav-link nav-tab" data-bs-target="#profile" role="tab" data-bs-toggle="tab">Profile</button>',
' </li>',
'</ul>',
'<div class="tab-content">',
Expand All @@ -573,10 +597,10 @@ describe('Tab', () => {
fixtureEl.innerHTML = [
'<ul class="nav nav-tabs" role="tablist">',
' <li class="nav-item" role="presentation">',
' <a class="nav-link nav-tab" href="#home" role="tab" data-bs-toggle="tab">Home</a>',
' <button type="button" class="nav-link nav-tab" data-bs-target="#home" role="tab" data-bs-toggle="tab">Home</button>',
' </li>',
' <li class="nav-item" role="presentation">',
' <a id="secondNav" class="nav-link nav-tab" href="#profile" role="tab" data-bs-toggle="tab">Profile</a>',
' <button type="button" id="secondNav" class="nav-link nav-tab" data-bs-target="#profile" role="tab" data-bs-toggle="tab">Profile</button>',
' </li>',
'</ul>',
'<div class="tab-content">',
Expand Down

0 comments on commit 96be06e

Please sign in to comment.