From 4a5029ea29ac75243dfec68153051292fc70f5cf Mon Sep 17 00:00:00 2001 From: alpadev <2838324+alpadev@users.noreply.github.com> Date: Thu, 3 Jun 2021 13:44:16 +0200 Subject: [PATCH] Fix handling of transitionend events dispatched by nested elements(#33845) Fix handling of transitionend events dispatched by nested elements Properly handle events from nested elements Change `emulateTransitionEnd` to `executeAfterTransition` && --- js/src/base-component.js | 16 +--- js/src/modal.js | 33 +++---- js/src/util/backdrop.js | 11 +-- js/src/util/index.js | 51 +++++++---- js/tests/unit/modal.spec.js | 23 +++++ js/tests/unit/tooltip.spec.js | 2 +- js/tests/unit/util/index.spec.js | 153 +++++++++++++++++++++++++------ 7 files changed, 201 insertions(+), 88 deletions(-) diff --git a/js/src/base-component.js b/js/src/base-component.js index a5f1b36a00..368cc99c88 100644 --- a/js/src/base-component.js +++ b/js/src/base-component.js @@ -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' @@ -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 */ diff --git a/js/src/modal.js b/js/src/modal.js index b05fe8de75..e8eee3b4d0 100644 --- a/js/src/modal.js +++ b/js/src/modal.js @@ -7,9 +7,7 @@ import { defineJQueryPlugin, - emulateTransitionEnd, getElementFromSelector, - getTransitionDurationFromElement, isRTL, isVisible, reflow, @@ -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 - if (!isModalOverflowing) { - this._element.style.overflowY = 'hidden' + // return if the following background transition hasn't yet completed + if ((!isModalOverflowing && style.overflowY === 'hidden') || classList.contains(CLASS_NAME_STATIC)) { + return } - 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) + if (!isModalOverflowing) { + style.overflowY = 'hidden' + } + + 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() } diff --git a/js/src/util/backdrop.js b/js/src/util/backdrop.js index 07ad20fab7..028325d118 100644 --- a/js/src/util/backdrop.js +++ b/js/src/util/backdrop.js @@ -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 @@ -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) } } diff --git a/js/src/util/index.js b/js/src/util/index.js index 4d077b21f9..6edfaa580d 100644 --- a/js/src/util/index.js +++ b/js/src/util/index.js @@ -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] @@ -252,6 +234,35 @@ const execute = callback => { } } +const executeAfterTransition = (callback, transitionElement, waitForTransition = true) => { + if (!waitForTransition) { + execute(callback) + return + } + + const durationPadding = 5 + const emulatedDuration = getTransitionDurationFromElement(transitionElement) + durationPadding + + let called = false + + 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. * @@ -288,7 +299,6 @@ export { getTransitionDurationFromElement, triggerTransitionEnd, isElement, - emulateTransitionEnd, typeCheckConfig, isVisible, isDisabled, @@ -300,5 +310,6 @@ export { onDOMContentLoaded, isRTL, defineJQueryPlugin, - execute + execute, + executeAfterTransition } diff --git a/js/tests/unit/modal.spec.js b/js/tests/unit/modal.spec.js index f73ac40b55..2974495cac 100644 --- a/js/tests/unit/modal.spec.js +++ b/js/tests/unit/modal.spec.js @@ -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 = '
' + + 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 = '
' diff --git a/js/tests/unit/tooltip.spec.js b/js/tests/unit/tooltip.spec.js index 27c7a350bc..b5a34bfe0a 100644 --- a/js/tests/unit/tooltip.spec.js +++ b/js/tests/unit/tooltip.spec.js @@ -446,7 +446,7 @@ describe('Tooltip', () => { const tooltip = new Tooltip(tooltipEl) document.documentElement.ontouchstart = noop - spyOn(EventHandler, 'on') + spyOn(EventHandler, 'on').and.callThrough() tooltipEl.addEventListener('shown.bs.tooltip', () => { expect(document.querySelector('.tooltip')).not.toBeNull() diff --git a/js/tests/unit/util/index.spec.js b/js/tests/unit/util/index.spec.js index 737ecacfde..04ad6bf432 100644 --- a/js/tests/unit/util/index.spec.js +++ b/js/tests/unit/util/index.spec.js @@ -157,12 +157,13 @@ describe('Util', () => { describe('triggerTransitionEnd', () => { it('should trigger transitionend event', done => { - fixtureEl.innerHTML = '
' + fixtureEl.innerHTML = '
' const el = fixtureEl.querySelector('div') + const spy = spyOn(el, 'dispatchEvent').and.callThrough() el.addEventListener('transitionend', () => { - expect().nothing() + expect(spy).toHaveBeenCalled() done() }) @@ -226,33 +227,6 @@ describe('Util', () => { }) }) - describe('emulateTransitionEnd', () => { - it('should emulate transition end', () => { - fixtureEl.innerHTML = '
' - - const el = document.querySelector('div') - const spy = spyOn(window, 'setTimeout') - - Util.emulateTransitionEnd(el, 10) - expect(spy).toHaveBeenCalled() - }) - - it('should not emulate transition end if already triggered', done => { - fixtureEl.innerHTML = '
' - - const el = fixtureEl.querySelector('div') - const spy = spyOn(el, 'removeEventListener') - - Util.emulateTransitionEnd(el, 10) - Util.triggerTransitionEnd(el) - - setTimeout(() => { - expect(spy).toHaveBeenCalled() - done() - }, 20) - }) - }) - describe('typeCheckConfig', () => { const namePlugin = 'collapse' @@ -660,6 +634,127 @@ describe('Util', () => { }) }) + describe('executeAfterTransition', () => { + it('should immediately execute a function when waitForTransition parameter is false', () => { + const el = document.createElement('div') + const callbackSpy = jasmine.createSpy('callback spy') + const eventListenerSpy = spyOn(el, 'addEventListener') + + Util.executeAfterTransition(callbackSpy, el, false) + + expect(callbackSpy).toHaveBeenCalled() + expect(eventListenerSpy).not.toHaveBeenCalled() + }) + + it('should execute a function when a transitionend event is dispatched', () => { + const el = document.createElement('div') + const callbackSpy = jasmine.createSpy('callback spy') + + spyOn(window, 'getComputedStyle').and.returnValue({ + transitionDuration: '0.05s', + transitionDelay: '0s' + }) + + Util.executeAfterTransition(callbackSpy, el) + + el.dispatchEvent(new TransitionEvent('transitionend')) + + expect(callbackSpy).toHaveBeenCalled() + }) + + it('should execute a function after a computed CSS transition duration and there was no transitionend event dispatched', done => { + const el = document.createElement('div') + const callbackSpy = jasmine.createSpy('callback spy') + + spyOn(window, 'getComputedStyle').and.returnValue({ + transitionDuration: '0.05s', + transitionDelay: '0s' + }) + + Util.executeAfterTransition(callbackSpy, el) + + setTimeout(() => { + expect(callbackSpy).toHaveBeenCalled() + done() + }, 70) + }) + + it('should not execute a function a second time after a computed CSS transition duration and if a transitionend event has already been dispatched', done => { + const el = document.createElement('div') + const callbackSpy = jasmine.createSpy('callback spy') + + spyOn(window, 'getComputedStyle').and.returnValue({ + transitionDuration: '0.05s', + transitionDelay: '0s' + }) + + Util.executeAfterTransition(callbackSpy, el) + + setTimeout(() => { + el.dispatchEvent(new TransitionEvent('transitionend')) + }, 50) + + setTimeout(() => { + expect(callbackSpy).toHaveBeenCalledTimes(1) + done() + }, 70) + }) + + it('should not trigger a transitionend event if another transitionend event had already happened', done => { + const el = document.createElement('div') + + spyOn(window, 'getComputedStyle').and.returnValue({ + transitionDuration: '0.05s', + transitionDelay: '0s' + }) + + Util.executeAfterTransition(() => {}, el) + + // simulate a event dispatched by the browser + el.dispatchEvent(new TransitionEvent('transitionend')) + + const dispatchSpy = spyOn(el, 'dispatchEvent').and.callThrough() + + setTimeout(() => { + // setTimeout should not have triggered another transitionend event. + expect(dispatchSpy).not.toHaveBeenCalled() + done() + }, 70) + }) + + it('should ignore transitionend events from nested elements', done => { + fixtureEl.innerHTML = [ + '
', + '
', + '
' + ].join('') + + const outer = fixtureEl.querySelector('.outer') + const nested = fixtureEl.querySelector('.nested') + const callbackSpy = jasmine.createSpy('callback spy') + + spyOn(window, 'getComputedStyle').and.returnValue({ + transitionDuration: '0.05s', + transitionDelay: '0s' + }) + + Util.executeAfterTransition(callbackSpy, outer) + + nested.dispatchEvent(new TransitionEvent('transitionend', { + bubbles: true + })) + + setTimeout(() => { + expect(callbackSpy).not.toHaveBeenCalled() + }, 20) + + setTimeout(() => { + expect(callbackSpy).toHaveBeenCalled() + done() + }, 70) + }) + }) + describe('getNextActiveElement', () => { it('should return first element if active not exists or not given and shouldGetNext is either true, or false with cycling being disabled', () => { const array = ['a', 'b', 'c', 'd']