Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix dropdown keys to open menu #32750

Merged
merged 10 commits into from
Feb 3, 2021
Merged

Fix dropdown keys to open menu #32750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
af77129
Fixes for:
sijusamson Jan 10, 2021
b375998
Changes for event key
sijusamson Jan 10, 2021
5e95edd
Added "Active" Case condition
sijusamson Jan 10, 2021
939f930
Fixes EsLint issues
sijusamson Jan 10, 2021
ac6e2c4
Event Triggered only for "Up" and "Down" Key
sijusamson Jan 16, 2021
56cf7aa
Added a test-case for Up/Down key press in dropdown.
sijusamson Jan 17, 2021
991432f
Added a testcase for ArrowUp button
sijusamson Jan 30, 2021
7927a8f
changes for 'Arrow Down function'
sijusamson Jan 31, 2021
1e8db5b
Merge branch 'main' into dropdown_keys_no_longer_open_menu
XhmikosR Feb 1, 2021
783955a
Merge branch 'main' into dropdown_keys_no_longer_open_menu
XhmikosR Feb 3, 2021
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
6 changes: 6 additions & 0 deletions js/src/dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,12 @@ class Dropdown extends BaseComponent {
return
}

if (!isActive && (event.key === ARROW_UP_KEY || event.key === ARROW_DOWN_KEY)) {
const button = this.matches(SELECTOR_DATA_TOGGLE) ? this : SelectorEngine.prev(this, SELECTOR_DATA_TOGGLE)[0]
button.click()
return
}

Comment on lines +471 to +476
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now the method dataApiKeydownHandler has a linting warning:
Static method 'dataApiKeydownHandler' has a complexity of 23. Maximum allowed is 20.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rohit2sharma95 Thank you for your suggestion, let me try to figure out the best possible solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per my findings, this linting warning can be removed if we split the function dataApiKeydownHandler. We can create a new static method, and pass on some functionality there.
But should we do that ?
@rohit2sharma95 @patrickhlauke @XhmikosR

Copy link
Member

Choose a reason for hiding this comment

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

I'd say ignore it for now and tackle it later in another PR.

if (!isActive || event.key === SPACE_KEY) {
Dropdown.clearMenus()
return
Expand Down
48 changes: 48 additions & 0 deletions js/tests/unit/dropdown.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1626,4 +1626,52 @@ describe('Dropdown', () => {
expect(Dropdown.getInstance(div)).toEqual(null)
})
})

it('should open dropdown when pressing keydown or keyup', done => {
fixtureEl.innerHTML = [
'<div class="dropdown">',
' <button class="btn dropdown-toggle" data-bs-toggle="dropdown">Dropdown</button>',
' <div class="dropdown-menu">',
' <a class="dropdown-item disabled" href="#sub1">Submenu 1</a>',
' <button class="dropdown-item" type="button" disabled>Disabled button</button>',
' <a id="item1" class="dropdown-item" href="#">Another link</a>',
' </div>',
'</div>'
].join('')

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

const keydown = createEvent('keydown')
keydown.key = 'ArrowDown'
sijusamson marked this conversation as resolved.
Show resolved Hide resolved

const keyup = createEvent('keyup')
keyup.key = 'ArrowUp'

const handleArrowDown = () => {
expect(triggerDropdown.classList.contains('show')).toEqual(true)
expect(triggerDropdown.getAttribute('aria-expanded')).toEqual('true')
setTimeout(() => {
dropdown.hide()
keydown.key = 'ArrowUp'
triggerDropdown.dispatchEvent(keyup)
}, 20)
}

const handleArrowUp = () => {
expect(triggerDropdown.classList.contains('show')).toEqual(true)
expect(triggerDropdown.getAttribute('aria-expanded')).toEqual('true')
done()
}

dropdown.addEventListener('shown.bs.dropdown', event => {
if (event.target.key === 'ArrowDown') {
handleArrowDown()
} else {
handleArrowUp()
}
})

triggerDropdown.dispatchEvent(keydown)
})
})