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

Handle delegated events in bubbling phase wherever possible #40574

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
+63 −72
Open

Handle delegated events in bubbling phase wherever possible #40574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
48f6fb8
WIP bubbling events
nwalters512 Jun 20, 2024
443a8d8
WIP updated tests and implementation
nwalters512 Jun 21, 2024
3973239
Working tests
nwalters512 Jun 21, 2024
36182fe
Remove logs
nwalters512 Jun 21, 2024
2dc80af
Fix bundlewatch values
julien-deramond Jul 19, 2024
eae6f81
Fix bundlewatch values
julien-deramond Jul 19, 2024
2261ae7
Merge branch 'main' into events-bubble
nwalters512 Jul 19, 2024
484f344
Merge branch 'main' into events-bubble
nwalters512 Jul 31, 2024
a084886
Merge branch 'main' into events-bubble
nwalters512 Aug 6, 2024
732493e
Merge branch 'main' into events-bubble
nwalters512 Aug 9, 2024
2c5cf8e
Merge branch 'main' into events-bubble
nwalters512 Aug 14, 2024
4fa651f
Merge branch 'main' into events-bubble
nwalters512 Sep 3, 2024
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: 3 additions & 3 deletions .bundlewatch.config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@
},
{
"path": "./dist/js/bootstrap.bundle.js",
"maxSize": "43.0 kB"
"maxSize": "43.25 kB"
},
{
"path": "./dist/js/bootstrap.bundle.min.js",
"maxSize": "23.5 kB"
},
{
"path": "./dist/js/bootstrap.esm.js",
"maxSize": "28.0 kB"
"maxSize": "28.25 kB"
},
{
"path": "./dist/js/bootstrap.esm.min.js",
Expand All @@ -54,7 +54,7 @@
},
{
"path": "./dist/js/bootstrap.min.js",
"maxSize": "16.25 kB"
"maxSize": "16.5 kB"
}
],
"ci": {
Expand Down
27 changes: 14 additions & 13 deletions js/src/dom/event-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ function normalizeParameters(originalTypeEvent, handler, delegationFunction) {
return [isDelegated, callable, typeEvent]
}

function addHandler(element, originalTypeEvent, handler, delegationFunction, oneOff) {
function addHandler(element, originalTypeEvent, handler, delegationFunction, options) {
Copy link
Contributor Author

@nwalters512 nwalters512 Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is the main change from this PR: whether or not the listener is for the capture phase is now explicit instead of being inferred based on whether this is a delegated handler or not.

I'm very open to changing this API. Suggestions welcome, especially if they mean we can avoid the eslint-disable-next-line max-params later in this file.

if (typeof originalTypeEvent !== 'string' || !element) {
return
}
Expand All @@ -165,7 +165,7 @@ function addHandler(element, originalTypeEvent, handler, delegationFunction, one
const previousFunction = findHandler(handlers, callable, isDelegated ? handler : null)

if (previousFunction) {
previousFunction.oneOff = previousFunction.oneOff && oneOff
previousFunction.oneOff = previousFunction.oneOff && options.oneOff

return
}
Expand All @@ -177,21 +177,22 @@ function addHandler(element, originalTypeEvent, handler, delegationFunction, one

fn.delegationSelector = isDelegated ? handler : null
fn.callable = callable
fn.oneOff = oneOff
fn.oneOff = options.oneOff
fn.uidEvent = uid
handlers[uid] = fn

element.addEventListener(typeEvent, fn, isDelegated)
element.addEventListener(typeEvent, fn, options?.capture ?? false)
}

function removeHandler(element, events, typeEvent, handler, delegationSelector) {
// eslint-disable-next-line max-params
function removeHandler(element, events, typeEvent, handler, delegationSelector, options) {
const fn = findHandler(events[typeEvent], handler, delegationSelector)

if (!fn) {
return
}

element.removeEventListener(typeEvent, fn, Boolean(delegationSelector))
element.removeEventListener(typeEvent, fn, options?.capture ?? false)
delete events[typeEvent][fn.uidEvent]
}

Expand All @@ -212,15 +213,15 @@ function getTypeEvent(event) {
}

const EventHandler = {
on(element, event, handler, delegationFunction) {
addHandler(element, event, handler, delegationFunction, false)
on(element, event, handler, delegationFunction, options) {
addHandler(element, event, handler, delegationFunction, { ...options, oneOff: false })
},

one(element, event, handler, delegationFunction) {
addHandler(element, event, handler, delegationFunction, true)
one(element, event, handler, delegationFunction, options) {
addHandler(element, event, handler, delegationFunction, { ...options, oneOff: true })
},

off(element, originalTypeEvent, handler, delegationFunction) {
off(element, originalTypeEvent, handler, delegationFunction, options) {
if (typeof originalTypeEvent !== 'string' || !element) {
return
}
Expand All @@ -237,7 +238,7 @@ const EventHandler = {
return
}

removeHandler(element, events, typeEvent, callable, isDelegated ? handler : null)
removeHandler(element, events, typeEvent, callable, isDelegated ? handler : null, options)
return
}

Expand All @@ -251,7 +252,7 @@ const EventHandler = {
const handlerKey = keyHandlers.replace(stripUidRegex, '')

if (!inNamespace || originalTypeEvent.includes(handlerKey)) {
removeHandler(element, events, typeEvent, event.callable, event.delegationSelector)
removeHandler(element, events, typeEvent, event.callable, event.delegationSelector, options)
}
}
},
Expand Down
10 changes: 5 additions & 5 deletions js/src/dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,14 +437,14 @@ class Dropdown extends BaseComponent {
* Data API implementation
*/

EventHandler.on(document, EVENT_KEYDOWN_DATA_API, SELECTOR_DATA_TOGGLE, Dropdown.dataApiKeydownHandler)
EventHandler.on(document, EVENT_KEYDOWN_DATA_API, SELECTOR_MENU, Dropdown.dataApiKeydownHandler)
EventHandler.on(document, EVENT_CLICK_DATA_API, Dropdown.clearMenus)
EventHandler.on(document, EVENT_KEYUP_DATA_API, Dropdown.clearMenus)
EventHandler.on(document, EVENT_KEYDOWN_DATA_API, SELECTOR_DATA_TOGGLE, Dropdown.dataApiKeydownHandler, { capture: true })
EventHandler.on(document, EVENT_KEYDOWN_DATA_API, SELECTOR_MENU, Dropdown.dataApiKeydownHandler, { capture: true })
EventHandler.on(document, EVENT_CLICK_DATA_API, Dropdown.clearMenus, { capture: true })
EventHandler.on(document, EVENT_KEYUP_DATA_API, Dropdown.clearMenus, { capture: true })
EventHandler.on(document, EVENT_CLICK_DATA_API, SELECTOR_DATA_TOGGLE, function (event) {
event.preventDefault()
Dropdown.getOrCreateInstance(this).toggle()
})
}, { capture: true })

/**
* jQuery
Expand Down
28 changes: 10 additions & 18 deletions js/tests/unit/collapse.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,28 +514,20 @@ describe('Collapse', () => {

describe('data-api', () => {
it('should prevent url change if click on nested elements', () => {
return new Promise(resolve => {
fixtureEl.innerHTML = [
'<a role="button" data-bs-toggle="collapse" class="collapsed" href="#collapse">',
' <span id="nested"></span>',
'</a>',
'<div id="collapse" class="collapse"></div>'
].join('')
fixtureEl.innerHTML = [
'<a role="button" data-bs-toggle="collapse" class="collapsed" href="#collapse">',
' <span id="nested"></span>',
'</a>',
'<div id="collapse" class="collapse"></div>'
].join('')

const triggerEl = fixtureEl.querySelector('a')
const nestedTriggerEl = fixtureEl.querySelector('#nested')
const nestedTriggerEl = fixtureEl.querySelector('#nested')

const spy = spyOn(Event.prototype, 'preventDefault').and.callThrough()
const spy = spyOn(Event.prototype, 'preventDefault').and.callThrough()

triggerEl.addEventListener('click', event => {
expect(event.target.isEqualNode(nestedTriggerEl)).toBeTrue()
expect(event.delegateTarget.isEqualNode(triggerEl)).toBeTrue()
expect(spy).toHaveBeenCalled()
resolve()
})
nestedTriggerEl.click()

nestedTriggerEl.click()
})
expect(spy).toHaveBeenCalled()
})

it('should show multiple collapsed elements', () => {
Expand Down
40 changes: 19 additions & 21 deletions js/tests/unit/dropdown.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1361,7 +1361,7 @@ describe('Dropdown', () => {
btnDropdown.addEventListener('shown.bs.dropdown', () => {
expect(btnDropdown).toHaveClass('show')

const keyup = createEvent('keyup')
const keyup = createEvent('keyup', { bubbles: true })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and all other createEvent calls I changed are for events that bubble in the browser when they come from keyboard/mouse input. It might be reasonable to change the default for createEvent? I'm not sure. I only changed as many calls as were required to get the tests passing.


keyup.key = 'Tab'
document.dispatchEvent(keyup)
Expand Down Expand Up @@ -1456,7 +1456,7 @@ describe('Dropdown', () => {
expect(triggerDropdownFirst).toHaveClass('show')
expect(fixtureEl.querySelectorAll('.dropdown-menu.show')).toHaveSize(1)

const keyup = createEvent('keyup')
const keyup = createEvent('keyup', { bubbles: true })
keyup.key = 'Tab'

document.dispatchEvent(keyup)
Expand All @@ -1471,7 +1471,7 @@ describe('Dropdown', () => {
expect(triggerDropdownLast).toHaveClass('show')
expect(fixtureEl.querySelectorAll('.dropdown-menu.show')).toHaveSize(1)

const keyup = createEvent('keyup')
const keyup = createEvent('keyup', { bubbles: true })
keyup.key = 'Tab'

document.dispatchEvent(keyup)
Expand Down Expand Up @@ -1570,7 +1570,7 @@ describe('Dropdown', () => {
})

triggerDropdown.addEventListener('shown.bs.dropdown', () => {
const keydown = createEvent('keydown')
const keydown = createEvent('keydown', { bubbles: true })

keydown.key = 'Escape'
triggerDropdown.dispatchEvent(keydown)
Expand Down Expand Up @@ -1637,7 +1637,7 @@ describe('Dropdown', () => {

triggerDropdown.addEventListener('shown.bs.dropdown', () => {
input.focus()
const keydown = createEvent('keydown')
const keydown = createEvent('keydown', { bubbles: true })

keydown.key = 'ArrowUp'
input.dispatchEvent(keydown)
Expand Down Expand Up @@ -1671,7 +1671,7 @@ describe('Dropdown', () => {
const triggerDropdown = fixtureEl.querySelector('[data-bs-toggle="dropdown"]')

triggerDropdown.addEventListener('shown.bs.dropdown', () => {
const keydown = createEvent('keydown')
const keydown = createEvent('keydown', { bubbles: true })
keydown.key = 'ArrowDown'

triggerDropdown.dispatchEvent(keydown)
Expand Down Expand Up @@ -1708,7 +1708,7 @@ describe('Dropdown', () => {
const triggerDropdown = fixtureEl.querySelector('[data-bs-toggle="dropdown"]')

triggerDropdown.addEventListener('shown.bs.dropdown', () => {
const keydown = createEvent('keydown')
const keydown = createEvent('keydown', { bubbles: true })
keydown.key = 'ArrowDown'

triggerDropdown.dispatchEvent(keydown)
Expand Down Expand Up @@ -1741,7 +1741,7 @@ describe('Dropdown', () => {
const item2 = fixtureEl.querySelector('#item2')

triggerDropdown.addEventListener('shown.bs.dropdown', () => {
const keydownArrowDown = createEvent('keydown')
const keydownArrowDown = createEvent('keydown', { bubbles: true })
keydownArrowDown.key = 'ArrowDown'

triggerDropdown.dispatchEvent(keydownArrowDown)
Expand All @@ -1750,7 +1750,7 @@ describe('Dropdown', () => {
document.activeElement.dispatchEvent(keydownArrowDown)
expect(document.activeElement).toEqual(item2, 'item2 is focused')

const keydownArrowUp = createEvent('keydown')
const keydownArrowUp = createEvent('keydown', { bubbles: true })
keydownArrowUp.key = 'ArrowUp'

document.activeElement.dispatchEvent(keydownArrowUp)
Expand Down Expand Up @@ -1785,7 +1785,7 @@ describe('Dropdown', () => {
})
})

const keydown = createEvent('keydown')
const keydown = createEvent('keydown', { bubbles: true })
keydown.key = 'ArrowUp'
triggerDropdown.dispatchEvent(keydown)
})
Expand Down Expand Up @@ -1813,7 +1813,7 @@ describe('Dropdown', () => {
})
})

const keydown = createEvent('keydown')
const keydown = createEvent('keydown', { bubbles: true })
keydown.key = 'ArrowDown'
triggerDropdown.dispatchEvent(keydown)
})
Expand All @@ -1840,7 +1840,7 @@ describe('Dropdown', () => {

triggerDropdown.addEventListener('shown.bs.dropdown', () => {
expect(triggerDropdown).toHaveClass('show')
input.dispatchEvent(createEvent('click'))
input.dispatchEvent(createEvent('click', { bubbles: true }))
})

triggerDropdown.click()
Expand Down Expand Up @@ -1868,7 +1868,7 @@ describe('Dropdown', () => {

triggerDropdown.addEventListener('shown.bs.dropdown', () => {
expect(triggerDropdown).toHaveClass('show')
textarea.dispatchEvent(createEvent('click'))
textarea.dispatchEvent(createEvent('click', { bubbles: true }))
})

triggerDropdown.click()
Expand All @@ -1895,9 +1895,7 @@ describe('Dropdown', () => {
})

triggerDropdown.addEventListener('shown.bs.dropdown', () => {
input.dispatchEvent(createEvent('click', {
bubbles: true
}))
input.dispatchEvent(createEvent('click', { bubbles: true }))
})

triggerDropdown.click()
Expand All @@ -1922,14 +1920,14 @@ describe('Dropdown', () => {
const textarea = fixtureEl.querySelector('textarea')

const test = (eventKey, elementToDispatch) => {
const event = createEvent('keydown')
const event = createEvent('keydown', { bubbles: true })
event.key = eventKey
elementToDispatch.focus()
elementToDispatch.dispatchEvent(event)
expect(document.activeElement).toEqual(elementToDispatch, `${elementToDispatch.tagName} still focused`)
}

const keydownEscape = createEvent('keydown')
const keydownEscape = createEvent('keydown', { bubbles: true })
keydownEscape.key = 'Escape'

triggerDropdown.addEventListener('shown.bs.dropdown', () => {
Expand Down Expand Up @@ -1985,7 +1983,7 @@ describe('Dropdown', () => {
// Key escape
button.focus()
// Key escape
const keydownEscape = createEvent('keydown')
const keydownEscape = createEvent('keydown', { bubbles: true })
keydownEscape.key = 'Escape'
button.dispatchEvent(keydownEscape)

Expand Down Expand Up @@ -2351,10 +2349,10 @@ describe('Dropdown', () => {
const triggerDropdown = fixtureEl.querySelector('[data-bs-toggle="dropdown"]')
const dropdown = fixtureEl.querySelector('.dropdown')

const keydown = createEvent('keydown')
const keydown = createEvent('keydown', { bubbles: true })
keydown.key = 'ArrowDown'

const keyup = createEvent('keyup')
const keyup = createEvent('keyup', { bubbles: true })
keyup.key = 'ArrowUp'

const handleArrowDown = () => {
Expand Down
12 changes: 6 additions & 6 deletions js/tests/unit/toast.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ describe('Toast', () => {
resolve()
})

const mouseOverEvent = createEvent('mouseover')
const mouseOverEvent = createEvent('mouseover', { bubbles: true })
toastEl.dispatchEvent(mouseOverEvent)
}, toast._config.delay / 2)

Expand Down Expand Up @@ -309,7 +309,7 @@ describe('Toast', () => {
})

toastEl.addEventListener('focusin', () => {
const mouseOutEvent = createEvent('mouseout')
const mouseOutEvent = createEvent('mouseout', { bubbles: true })
toastEl.dispatchEvent(mouseOutEvent)
})

Expand All @@ -323,7 +323,7 @@ describe('Toast', () => {
resolve()
})

const mouseOverEvent = createEvent('mouseover')
const mouseOverEvent = createEvent('mouseover', { bubbles: true })
toastEl.dispatchEvent(mouseOverEvent)
}, toast._config.delay / 2)

Expand Down Expand Up @@ -362,7 +362,7 @@ describe('Toast', () => {
resolve()
})

const mouseOverEvent = createEvent('mouseover')
const mouseOverEvent = createEvent('mouseover', { bubbles: true })
toastEl.dispatchEvent(mouseOverEvent)
}, toast._config.delay / 2)

Expand Down Expand Up @@ -392,7 +392,7 @@ describe('Toast', () => {
})

toastEl.addEventListener('focusin', () => {
const mouseOutEvent = createEvent('mouseout')
const mouseOutEvent = createEvent('mouseout', { bubbles: true })
toastEl.dispatchEvent(mouseOutEvent)
})

Expand All @@ -401,7 +401,7 @@ describe('Toast', () => {
resolve()
})

const mouseOverEvent = createEvent('mouseover')
const mouseOverEvent = createEvent('mouseover', { bubbles: true })
toastEl.dispatchEvent(mouseOverEvent)
}, toast._config.delay / 2)

Expand Down
Loading
Loading