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 all 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
31 changes: 19 additions & 12 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 @@ -377,8 +375,14 @@ class Dropdown extends BaseComponent {
}

static clearMenus(event) {
if (event && (event.button === RIGHT_MOUSE_BUTTON || (event.type === 'keyup' && event.key !== TAB_KEY))) {
return
if (event) {
if (event.button === RIGHT_MOUSE_BUTTON || (event.type === 'keyup' && event.key !== TAB_KEY)) {
return
}

if (/input|select|textarea|form/i.test(event.target.tagName)) {
return
}
}

const toggles = SelectorEngine.find(SELECTOR_DATA_TOGGLE)
Expand All @@ -402,11 +406,16 @@ 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) {
// Don't close the menu if the clicked element or one of its parents is the dropdown button
if ([context._element].some(element => event.composedPath().includes(element))) {
continue
}
Comment on lines +411 to +413
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 I'm concerned that removing the setTimeouts might affect the reliability of the tests. If I remove this section of code (preventing the menu from opening), both the "data-api -> should show and hide a dropdown" and "should open the dropdown when clicking the child element inside data-bs-toggle="dropdown"" tests pass. With the setTimeouts, they fail.

In any case, I'm eager to get this merged in, so I've gone ahead and removed the setTimeouts, but I urge you to check into it. If you still feel they're not necessary, if you get a chance, I'd love to know the reasoning. Thanks for all your help getting this feature implemented!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, for smooth release please restore setTimeouts and rebase your branch 🙂


// Tab navigation through the dropdown menu shouldn't close the menu
if (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 +528,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
47 changes: 47 additions & 0 deletions js/tests/unit/dropdown.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1783,4 +1783,51 @@ 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 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', () => {
expect(btnDropdown.classList.contains('show')).toEqual(true)
expect(btnDropdown.getAttribute('aria-expanded')).toEqual('true')
done()
})

childElement.click()
})
})