Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .bundlewatch.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
},
{
"path": "./dist/js/bootstrap.bundle.js",
"maxSize": "41.25 kB"
"maxSize": "41.5 kB"
},
{
"path": "./dist/js/bootstrap.bundle.min.js",
Expand All @@ -50,7 +50,7 @@
},
{
"path": "./dist/js/bootstrap.js",
"maxSize": "27.25 kB"
"maxSize": "27.5 kB"
},
{
"path": "./dist/js/bootstrap.min.js",
Expand Down
21 changes: 10 additions & 11 deletions js/src/dropdown.js
Original file line number Diff line number Diff line change
Expand Up @@ -354,18 +354,16 @@ class Dropdown extends BaseComponent {
}
}

_selectMenuItem(event) {
if (![ARROW_UP_KEY, ARROW_DOWN_KEY].includes(event.key)) {
return
}

_selectMenuItem({ key, target }) {
const items = SelectorEngine.find(SELECTOR_VISIBLE_ITEMS, this._menu).filter(isVisible)

if (!items.length) {
return
}

getNextActiveElement(items, event.target, event.key === ARROW_DOWN_KEY, false).focus()
// if target isn't included in items (e.g. when expanding the dropdown)
// allow cycling to get the last item in case key equals ARROW_UP_KEY
getNextActiveElement(items, target, key === ARROW_DOWN_KEY, !items.includes(target)).focus()
}

// Static
Expand Down Expand Up @@ -480,17 +478,18 @@ class Dropdown extends BaseComponent {
return
}

if (!isActive && (event.key === ARROW_UP_KEY || event.key === ARROW_DOWN_KEY)) {
getToggleButton().click()
if (event.key === ARROW_UP_KEY || event.key === ARROW_DOWN_KEY) {
if (!isActive) {
getToggleButton().click()
}

Dropdown.getInstance(getToggleButton())._selectMenuItem(event)
return
}

if (!isActive || event.key === SPACE_KEY) {
Dropdown.clearMenus()
return
}

Dropdown.getInstance(getToggleButton())._selectMenuItem(event)
}
}

Expand Down
4 changes: 2 additions & 2 deletions js/src/util/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,9 @@ const execute = callback => {
const getNextActiveElement = (list, activeElement, shouldGetNext, isCycleAllowed) => {
let index = list.indexOf(activeElement)

// if the element does not exist in the list initialize it as the first element
// if the element does not exist in the list return an element depending on the direction and if cycle is allowed
if (index === -1) {
return list[0]
return list[!shouldGetNext && isCycleAllowed ? list.length - 1 : 0]
Comment on lines +267 to +269
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked with @GeoSot about adding the feature of getting the last item from getNextActiveElement() in case the item is not in the list, we want the previous item and cycling is allowed. (To reflect the case when the dropdown button is the target of the event and the ARROW_UP_KEY is pressed.) He was ok with adding it.

But I'd appreciate to receive some other opinion.

}

const listLength = list.length
Expand Down
41 changes: 33 additions & 8 deletions js/tests/unit/dropdown.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1561,7 +1561,7 @@ describe('Dropdown', () => {
triggerDropdown.click()
})

it('should focus on the first element when using ArrowUp for the first time', done => {
it('should open the dropdown and focus on the last item when using ArrowUp for the first time', done => {
fixtureEl.innerHTML = [
'<div class="dropdown">',
' <button class="btn dropdown-toggle" data-bs-toggle="dropdown">Dropdown</button>',
Expand All @@ -1573,19 +1573,44 @@ describe('Dropdown', () => {
].join('')

const triggerDropdown = fixtureEl.querySelector('[data-bs-toggle="dropdown"]')
const item1 = fixtureEl.querySelector('#item1')
const lastItem = fixtureEl.querySelector('#item2')

triggerDropdown.addEventListener('shown.bs.dropdown', () => {
const keydown = createEvent('keydown')
keydown.key = 'ArrowUp'
setTimeout(() => {
expect(document.activeElement).toEqual(lastItem, 'item2 is focused')
done()
})
})

document.activeElement.dispatchEvent(keydown)
expect(document.activeElement).toEqual(item1, 'item1 is focused')
const keydown = createEvent('keydown')
keydown.key = 'ArrowUp'
triggerDropdown.dispatchEvent(keydown)
})

done()
it('should open the dropdown and focus on the first item when using ArrowDown for the first time', done => {
fixtureEl.innerHTML = [
'<div class="dropdown">',
' <button class="btn dropdown-toggle" data-bs-toggle="dropdown">Dropdown</button>',
' <div class="dropdown-menu">',
' <a id="item1" class="dropdown-item" href="#">A link</a>',
' <a id="item2" class="dropdown-item" href="#">Another link</a>',
' </div>',
'</div>'
].join('')

const triggerDropdown = fixtureEl.querySelector('[data-bs-toggle="dropdown"]')
const firstItem = fixtureEl.querySelector('#item1')

triggerDropdown.addEventListener('shown.bs.dropdown', () => {
setTimeout(() => {
expect(document.activeElement).toEqual(firstItem, 'item1 is focused')
done()
})
})

triggerDropdown.click()
const keydown = createEvent('keydown')
keydown.key = 'ArrowDown'
triggerDropdown.dispatchEvent(keydown)
})

it('should not close the dropdown if the user clicks on a text field within dropdown-menu', done => {
Expand Down
13 changes: 12 additions & 1 deletion js/tests/unit/util/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -661,11 +661,22 @@ describe('Util', () => {
})

describe('getNextActiveElement', () => {
it('should return first element if active not exists or not given', () => {
it('should return first element if active not exists or not given and shouldGetNext is either true, or false with cycling being disabled', () => {
const array = ['a', 'b', 'c', 'd']

expect(Util.getNextActiveElement(array, '', true, true)).toEqual('a')
expect(Util.getNextActiveElement(array, 'g', true, true)).toEqual('a')
expect(Util.getNextActiveElement(array, '', true, false)).toEqual('a')
expect(Util.getNextActiveElement(array, 'g', true, false)).toEqual('a')
expect(Util.getNextActiveElement(array, '', false, false)).toEqual('a')
expect(Util.getNextActiveElement(array, 'g', false, false)).toEqual('a')
})

it('should return last element if active not exists or not given and shouldGetNext is false but cycling is enabled', () => {
const array = ['a', 'b', 'c', 'd']

expect(Util.getNextActiveElement(array, '', false, true)).toEqual('d')
expect(Util.getNextActiveElement(array, 'g', false, true)).toEqual('d')
})

it('should return next element or same if is last', () => {
Expand Down