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.

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

Allow data-toggle="dropdown" and form click events to bubble #32942

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
40a206f
remove stopPropagation from button click event
caseyjhol Jan 6, 2021
9048cf5
add test for click events
caseyjhol Jan 30, 2021
a7c2013
ensure menu can open
caseyjhol Jan 31, 2021
7fc8ee9
test for delegated click events
caseyjhol Jan 31, 2021
160af31
Merge branch 'main' into bubble-click-events
caseyjhol Feb 4, 2021
a87e814
Merge branch 'main' into bubble-click-events
caseyjhol Feb 21, 2021
cdb5f3c
simplify delegated click event bubble test
caseyjhol Feb 21, 2021
1b4d5dd
rearrange test
caseyjhol Feb 21, 2021
65e254e
ensure button children can open menu
caseyjhol Feb 21, 2021
eea7b9c
test to ensure clicking button opens the menu
caseyjhol Feb 21, 2021
cdbd74d
check current element and parents
caseyjhol Feb 22, 2021
4a17f62
Merge branch 'main' into bubble-click-events
caseyjhol Feb 22, 2021
2bd7aea
Merge branch 'main' into bubble-click-events
caseyjhol Feb 25, 2021
80bdc9c
allow dropdown form click events to bubble
caseyjhol Feb 27, 2021
8156318
improve menu open tests
caseyjhol Feb 27, 2021
d5e67c6
Merge branch 'main' into bubble-click-events
caseyjhol Mar 12, 2021
b5f3d91
Merge branch 'main' into bubble-click-events
caseyjhol Mar 18, 2021
f08dae5
refactor/add comments
caseyjhol Mar 18, 2021
4572f81
Merge branch 'main' into bubble-click-events
caseyjhol Mar 19, 2021
9396b1b
remove extra test to check if menu is open
caseyjhol Mar 19, 2021
bbe928b
remove form test
caseyjhol Mar 19, 2021
b12081c
Merge branch 'main' into bubble-click-events
caseyjhol Mar 22, 2021
be87106
remove setTimeouts from tests
caseyjhol Mar 22, 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
24 changes: 14 additions & 10 deletions js/src/dropdown.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ const CLASS_NAME_DROPSTART = 'dropstart'
const CLASS_NAME_NAVBAR = 'navbar'

const SELECTOR_DATA_TOGGLE = '[data-bs-toggle="dropdown"]'
const SELECTOR_FORM_CHILD = '.dropdown form'
const SELECTOR_MENU = '.dropdown-menu'
const SELECTOR_NAVBAR_NAV = '.navbar-nav'
const SELECTOR_VISIBLE_ITEMS = '.dropdown-menu .dropdown-item:not(.disabled):not(:disabled)'
Expand Down Expand Up @@ -253,7 +252,6 @@ class Dropdown extends BaseComponent {
_addEventListeners() {
EventHandler.on(this._element, EVENT_CLICK, event => {
event.preventDefault()
event.stopPropagation()
this.toggle()
})
}
Expand Down Expand Up @@ -402,11 +400,19 @@ class Dropdown extends BaseComponent {
continue
}

if (event && ((event.type === 'click' &&
/input|textarea/i.test(event.target.tagName)) ||
(event.type === 'keyup' && event.key === TAB_KEY)) &&
dropdownMenu.contains(event.target)) {
continue
if (event) {
const dropdownForm = dropdownMenu.querySelector('form')
GeoSot marked this conversation as resolved.
Show resolved Hide resolved

if ([context._element, dropdownForm].some(element => event.composedPath().includes(element))) {
continue
}

if (((event.type === 'click' &&
/input|textarea/i.test(event.target.tagName)) ||
(event.type === 'keyup' && event.key === TAB_KEY)) &&
dropdownMenu.contains(event.target)) {
continue
}
caseyjhol marked this conversation as resolved.
Show resolved Hide resolved
}

const hideEvent = EventHandler.trigger(toggles[i], EVENT_HIDE, relatedTarget)
Expand Down Expand Up @@ -519,10 +525,8 @@ EventHandler.on(document, EVENT_CLICK_DATA_API, Dropdown.clearMenus)
EventHandler.on(document, EVENT_KEYUP_DATA_API, Dropdown.clearMenus)
EventHandler.on(document, EVENT_CLICK_DATA_API, SELECTOR_DATA_TOGGLE, function (event) {
event.preventDefault()
event.stopPropagation()
Dropdown.dropdownInterface(this, 'toggle')
Dropdown.dropdownInterface(this)
})
EventHandler.on(document, EVENT_CLICK_DATA_API, SELECTOR_FORM_CHILD, e => e.stopPropagation())

/**
* ------------------------------------------------------------------------
Expand Down
101 changes: 101 additions & 0 deletions js/tests/unit/dropdown.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1783,4 +1783,105 @@ describe('Dropdown', () => {

triggerDropdown.dispatchEvent(keydown)
})

it('should allow `data-bs-toggle="dropdown"` click events to bubble up', () => {
fixtureEl.innerHTML = [
'<div class="dropdown">',
' <button class="btn dropdown-toggle" data-bs-toggle="dropdown">Dropdown</button>',
' <div class="dropdown-menu">',
' <a class="dropdown-item" href="#">Secondary link</a>',
' </div>',
'</div>'
].join('')

const btnDropdown = fixtureEl.querySelector('[data-bs-toggle="dropdown"]')
const clickListener = jasmine.createSpy('clickListener')
const delegatedClickListener = jasmine.createSpy('delegatedClickListener')

btnDropdown.addEventListener('click', clickListener)
document.addEventListener('click', delegatedClickListener)

btnDropdown.click()

expect(clickListener).toHaveBeenCalled()
expect(delegatedClickListener).toHaveBeenCalled()
})

it('should allow `.dropdown-menu form` click events to bubble up', () => {
fixtureEl.innerHTML = [
'<div class="dropdown">',
' <button class="btn dropdown-toggle" data-bs-toggle="dropdown">Dropdown</button>',
' <div class="dropdown-menu">',
' <form>',
' <input class="form-control" type="text">',
' </form>',
' </div>',
'</div>'
].join('')

const btnDropdown = fixtureEl.querySelector('[data-bs-toggle="dropdown"]')
const form = fixtureEl.querySelector('form')
const formInput = form.querySelector('input')
const formClickListener = jasmine.createSpy('formClickListener')

form.addEventListener('click', formClickListener)

btnDropdown.addEventListener('shown.bs.dropdown', () => {
formInput.click()
form.click()
})

btnDropdown.addEventListener('hidden.bs.dropdown', () => {
throw new Error('should not throw hidden.bs.dropdown event')
})

btnDropdown.click()

expect(formClickListener).toHaveBeenCalledTimes(2)
})

it('should open the dropdown when clicking `data-bs-toggle="dropdown"`', done => {
fixtureEl.innerHTML = [
caseyjhol marked this conversation as resolved.
Show resolved Hide resolved
'<div class="dropdown">',
' <button class="btn dropdown-toggle" data-bs-toggle="dropdown">Dropdown</button>',
' <div class="dropdown-menu">',
' <a class="dropdown-item" href="#subMenu">Sub menu</a>',
' </div>',
'</div>'
].join('')

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

btnDropdown.addEventListener('shown.bs.dropdown', () => setTimeout(() => {
expect(btnDropdown.classList.contains('show')).toEqual(true)
expect(btnDropdown.getAttribute('aria-expanded')).toEqual('true')
done()
}))

btnDropdown.click()
})

it('should open the dropdown when clicking the child element inside `data-bs-toggle="dropdown"`', done => {
fixtureEl.innerHTML = [
'<div class="container">',
' <div class="dropdown">',
' <button class="btn dropdown-toggle" data-bs-toggle="dropdown"><span id="childElement">Dropdown</span></button>',
' <div class="dropdown-menu">',
' <a class="dropdown-item" href="#subMenu">Sub menu</a>',
' </div>',
' </div>',
'</div>'
].join('')

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

btnDropdown.addEventListener('shown.bs.dropdown', () => setTimeout(() => {
expect(btnDropdown.classList.contains('show')).toEqual(true)
expect(btnDropdown.getAttribute('aria-expanded')).toEqual('true')
done()
}))
Copy link
Collaborator

Choose a reason for hiding this comment

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

setTimeout is not required here IMO

Copy link
Contributor Author

@caseyjhol caseyjhol Mar 18, 2021

Choose a reason for hiding this comment

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

The test wasn't working properly for me (locally) without the setTimeout (it was passing when it shouldn't).

Do you want me to remove all code related to checking for the form (with the idea being users should set clickableMenu instead of Bootstrap detecting the presence of a form)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want me to remove all code related to checking for the form

No, that should be handled in the next PR.


childElement.click()
})
})