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 handling of transitionend events dispatched by nested elements #33845

Merged
merged 2 commits into from
Jun 3, 2021
Merged

Fix handling of transitionend events dispatched by nested elements #33845

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f12061
test: emulateTransitionEnd should properly handle events from nested …
alpadev May 6, 2021
4901bcc
Merge branch 'main' into fix-transitionend-event-handling
GeoSot Jun 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
16 changes: 3 additions & 13 deletions js/src/base-component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@

import Data from './dom/data'
import {
emulateTransitionEnd,
execute,
getElement,
getTransitionDurationFromElement
executeAfterTransition,
getElement
} from './util/index'
import EventHandler from './dom/event-handler'

Expand Down Expand Up @@ -44,15 +42,7 @@ class BaseComponent {
}

_queueCallback(callback, element, isAnimated = true) {
if (!isAnimated) {
execute(callback)
return
}

const transitionDuration = getTransitionDurationFromElement(element)
EventHandler.one(element, 'transitionend', () => execute(callback))

emulateTransitionEnd(element, transitionDuration)
executeAfterTransition(callback, element, isAnimated)
}

/** Static */
Expand Down
31 changes: 16 additions & 15 deletions js/src/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@

import {
defineJQueryPlugin,
emulateTransitionEnd,
getElementFromSelector,
getTransitionDurationFromElement,
isRTL,
isVisible,
reflow,
Expand Down Expand Up @@ -339,25 +337,28 @@ class Modal extends BaseComponent {
return
}

const isModalOverflowing = this._element.scrollHeight > document.documentElement.clientHeight
const { classList, scrollHeight, style } = this._element
const isModalOverflowing = scrollHeight > document.documentElement.clientHeight

// return if the following background transition hasn't yet completed
if ((!isModalOverflowing && style.overflowY === 'hidden') || classList.contains(CLASS_NAME_STATIC)) {
return
}

if (!isModalOverflowing) {
this._element.style.overflowY = 'hidden'
style.overflowY = 'hidden'
}

this._element.classList.add(CLASS_NAME_STATIC)
const modalTransitionDuration = getTransitionDurationFromElement(this._dialog)
EventHandler.off(this._element, 'transitionend')
EventHandler.one(this._element, 'transitionend', () => {
this._element.classList.remove(CLASS_NAME_STATIC)
classList.add(CLASS_NAME_STATIC)
this._queueCallback(() => {
classList.remove(CLASS_NAME_STATIC)
if (!isModalOverflowing) {
EventHandler.one(this._element, 'transitionend', () => {
this._element.style.overflowY = ''
})
emulateTransitionEnd(this._element, modalTransitionDuration)
this._queueCallback(() => {
style.overflowY = ''
}, this._dialog)
}
})
emulateTransitionEnd(this._element, modalTransitionDuration)
}, this._dialog)

this._element.focus()
}

Expand Down
11 changes: 2 additions & 9 deletions js/src/util/backdrop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import EventHandler from '../dom/event-handler'
import { emulateTransitionEnd, execute, getElement, getTransitionDurationFromElement, reflow, typeCheckConfig } from './index'
import { execute, executeAfterTransition, getElement, reflow, typeCheckConfig } from './index'

const Default = {
isVisible: true, // if false, we use the backdrop helper without adding any element to the dom
Expand Down Expand Up @@ -122,14 +122,7 @@ class Backdrop {
}

_emulateAnimation(callback) {
if (!this._config.isAnimated) {
execute(callback)
return
}

const backdropTransitionDuration = getTransitionDurationFromElement(this._getElement())
EventHandler.one(this._getElement(), 'transitionend', () => execute(callback))
emulateTransitionEnd(this._getElement(), backdropTransitionDuration)
executeAfterTransition(callback, this._getElement(), this._config.isAnimated)
}
}

Expand Down
51 changes: 31 additions & 20 deletions js/src/util/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,24 +126,6 @@ const getElement = obj => {
return null
}

const emulateTransitionEnd = (element, duration) => {
let called = false
const durationPadding = 5
const emulatedDuration = duration + durationPadding

function listener() {
called = true
element.removeEventListener(TRANSITION_END, listener)
}

element.addEventListener(TRANSITION_END, listener)
setTimeout(() => {
if (!called) {
triggerTransitionEnd(element)
}
}, emulatedDuration)
}

const typeCheckConfig = (componentName, config, configTypes) => {
Object.keys(configTypes).forEach(property => {
const expectedTypes = configTypes[property]
Expand Down Expand Up @@ -252,6 +234,35 @@ const execute = callback => {
}
}

const executeAfterTransition = (callback, transitionElement, waitForTransition = true) => {
if (!waitForTransition) {
execute(callback)
return
}

const durationPadding = 5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although the comment was not present here before, 5 seems the magic number here thus it is better to put a nice comment 🙂

Copy link
Contributor Author

@alpadev alpadev May 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure what to comment here.. I tried to find out why it's 5 but I have no idea.. Considering that one frame of a 60fps animation is about 16ms - 5ms seems basically nothing.

AFAICT the idea behind this emulation code was to dispatch a transitionend event if no other transitionend event had been triggered. Likely as a fallback for legacy browsers that don't support them.

I guess the intention was to add some additional duration so the timeout will finish after the native transition event in case the browser dispatch it. That's hard to enforce tho since we're not starting any transition here (or tracking its state) but only adding some listener and a timeout that would trigger after the calculated duration no matter what.

Also getTransitionDurationFromElement only respects the duration of one (the first) CSS transition. If there are multiple transitions defined and one that takes longer (which isn't the first one) setTimeout might finish too early. (This shouldn't be the case with our defaults from what I can tell.)

In general since the timeout triggers a (custom) transitionend event and we don't match or verify them, this could lead to inconsistencies. For example the browser takes longer for the transition, setTimeout will trigger an event, then the browser triggers another event for the same transition.

On a sidenote, native transitionend events in the test environment seem to be somewhat unreliable. No idea if this is some optimization the browser is doing because it's headless or the elements are rendered offscreen (because of the fixture) or what exactly is causing this. setTimeout on the other hand works pretty reliable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

/CC @Johann-S for the opinion ❤️

const emulatedDuration = getTransitionDurationFromElement(transitionElement) + durationPadding

let called = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

This flag variable (called) can not be omitted? @alpadev 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although, there is no test covering the use of the called flag 🙂

Copy link
Contributor Author

@alpadev alpadev May 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure about omitting this. I mean in theory we could, since the event handler unregisters itself, thus the callback is only executed once. But there would be another transitionend event dispatched by the setTimeout which could be a problem.

called is partly covered by
https://github.com/twbs/bootstrap/blob/fd016ac7ba9900f2275163d86c0d554d6a0d9b5a/js/tests/unit/util/index.spec.js#L665 because setTimeout wouldn't trigger an event if called is true.

I added another test that I hope will satisfy your objection. (I can't really verify the state of called since it's scoped to the function.)
https://github.com/twbs/bootstrap/blob/0c1d8fd9849be89f6f933b6b0017dc8f435c31c3/js/tests/unit/util/index.spec.js#L703


const handler = ({ target }) => {
if (target !== transitionElement) {
return
}

called = true
transitionElement.removeEventListener(TRANSITION_END, handler)
execute(callback)
}

transitionElement.addEventListener(TRANSITION_END, handler)
setTimeout(() => {
if (!called) {
triggerTransitionEnd(transitionElement)
}
}, emulatedDuration)
}

/**
* Return the previous/next element of a list.
*
Expand Down Expand Up @@ -288,7 +299,6 @@ export {
getTransitionDurationFromElement,
triggerTransitionEnd,
isElement,
emulateTransitionEnd,
typeCheckConfig,
isVisible,
isDisabled,
Expand All @@ -300,5 +310,6 @@ export {
onDOMContentLoaded,
isRTL,
defineJQueryPlugin,
execute
execute,
executeAfterTransition
}
23 changes: 23 additions & 0 deletions js/tests/unit/modal.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,29 @@ describe('Modal', () => {
modal.show()
})

it('should not queue multiple callbacks when clicking outside of modal-content and backdrop = static', done => {
fixtureEl.innerHTML = '<div class="modal"><div class="modal-dialog" style="transition-duration: 50ms;"></div></div>'

const modalEl = fixtureEl.querySelector('.modal')
const modal = new Modal(modalEl, {
backdrop: 'static'
})

modalEl.addEventListener('shown.bs.modal', () => {
const spy = spyOn(modal, '_queueCallback').and.callThrough()

modalEl.click()
modalEl.click()

setTimeout(() => {
expect(spy).toHaveBeenCalledTimes(1)
done()
}, 20)
})

modal.show()
})

it('should enforce focus', done => {
fixtureEl.innerHTML = '<div class="modal"><div class="modal-dialog"></div></div>'

Expand Down
2 changes: 1 addition & 1 deletion js/tests/unit/tooltip.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ describe('Tooltip', () => {
const tooltip = new Tooltip(tooltipEl)
document.documentElement.ontouchstart = noop

spyOn(EventHandler, 'on')
spyOn(EventHandler, 'on').and.callThrough()
GeoSot marked this conversation as resolved.
Show resolved Hide resolved

tooltipEl.addEventListener('shown.bs.tooltip', () => {
expect(document.querySelector('.tooltip')).not.toBeNull()
Expand Down
Loading