Fix handling of transitionend events dispatched by nested elements #33845

alpadev · 2021-05-06T11:27:28Z

Fixes #33682, fixes #33763, fixes #34066, fixes #31554 and fixes #33826.

Preview: https://deploy-preview-33845--twbs-bootstrap.netlify.app/

mdo · 2021-05-13T04:35:54Z

Is this ready for review and shipping with v5.0.1?

js/tests/unit/tooltip.spec.js

js/src/modal.js

js/src/util/index.js

XhmikosR · 2021-05-13T16:06:09Z

@alpadev would it be OK to move this to 5.0.2? I plan to make the release soon, and I'm unsure if we should land this in such a hurry :)

alpadev · 2021-05-13T16:13:37Z

@alpadev would it be OK to move this to 5.0.2? I plan to make the release soon, and I'm unsure if we should land this in such a hurry :)

Proceed as you wish, I don't mind. Been quite busy and missed the information in slack that we would do a release today. Otherwise I would have tried to focus more on this PR.

GeoSot · 2021-05-14T08:35:11Z

Now that we have time, guys do you consider changing theemulateTransitionEnd arguments is a breaking change or not?
Theoretically yes, but as our utils are not officially documented and exported, I suppose are remaining internal.

I am asking this, cause as Alpa said, the footprint of this change will be less compared with the extra function approach

CC/ @XhmikosR

XhmikosR · 2021-05-18T06:11:55Z

Technically, it could be a BC, but since we are not exposing directly the utils, I'd say we can make the change, if it helps us. At some point, we should expose our utils, but that's for another time.

Merged the emulateTransitionEnd and added some condition so the static background transition isn't triggered. I kindly ask you to review it again.

rohit2sharma95 · 2021-05-24T18:05:37Z

js/src/util/index.js

+    return
+  }
+
+  const durationPadding = 5


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

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.

/CC @Johann-S for the opinion ❤️

rohit2sharma95 · 2021-05-24T18:26:35Z

js/src/util/index.js

+  const durationPadding = 5
+  const emulatedDuration = getTransitionDurationFromElement(transitionElement) + durationPadding
+
+  let called = false


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

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

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

GeoSot · 2021-06-02T09:37:05Z

@alpadev @rohit2sharma95
this pr is being hold for much time. Do you want to proceed with this as it fixes many things and open an issue, for the questions that accrue from past implementation?

…elements fix: emulateTransitionEnd should properly handle events from nested elements fix: BaseComponent._queueCallback should not execute callback if event from nested elements refactor: use _queueCallback for static modal animation refactor: reuse _queueCallback from BaseComponent for backdrop animations feature: add executeAfterTransition utility function test: add spec for excuteAfterTransition function refactor: use executeAfterTransition in base-component and backdrop test: add test if static modal backdrop is clicked multiple times fix: callbacks are queued multiple times when static modal backdrop is clicked more than once test: added another test and updated timing/wording on executeAfterTransition tests test: ensure the functionality of triggerTransitionEnd refactor: merge emulateTransitionEnd with executeAfterTransition test: remove emulateTransitionEnd tests test: add another test for executeAfterTransition This is to assure that setTimeout wouldn't trigger another transitionend event if one had already happened

rohit2sharma95 · 2021-06-02T09:49:38Z

True @GeoSot, everything else LGTM 👍

…bs#33845) Fix handling of transitionend events dispatched by nested elements Properly handle events from nested elements Change `emulateTransitionEnd` to `executeAfterTransition` &&

alpadev mentioned this pull request May 6, 2021

Fix collapse with focused input #33695

Closed

GeoSot force-pushed the fix-transitionend-event-handling branch from 0199be3 to 5d73f20 Compare May 11, 2021 21:47

alpadev force-pushed the fix-transitionend-event-handling branch from 92e64d5 to 349e414 Compare May 13, 2021 12:01

alpadev marked this pull request as ready for review May 13, 2021 12:03

alpadev requested a review from a team as a code owner May 13, 2021 12:03

alpadev commented May 13, 2021

View reviewed changes

js/tests/unit/tooltip.spec.js Show resolved Hide resolved

alpadev commented May 13, 2021

View reviewed changes

js/src/modal.js Outdated Show resolved Hide resolved

GeoSot requested a review from rohit2sharma95 May 13, 2021 12:31

alpadev force-pushed the fix-transitionend-event-handling branch from 349e414 to aead9dd Compare May 13, 2021 12:49

rohit2sharma95 reviewed May 13, 2021

View reviewed changes

js/src/util/index.js Outdated Show resolved Hide resolved

rohit2sharma95 previously approved these changes May 13, 2021

View reviewed changes

GeoSot previously approved these changes May 13, 2021

View reviewed changes

alpadev requested a review from rohit2sharma95 May 13, 2021 15:58

alpadev force-pushed the fix-transitionend-event-handling branch 2 times, most recently from 68cb2cb to 4c3c946 Compare May 13, 2021 16:05

alpadev force-pushed the fix-transitionend-event-handling branch from 4c3c946 to 2064073 Compare May 13, 2021 16:11

alpadev force-pushed the fix-transitionend-event-handling branch 2 times, most recently from 9c4bb8a to 106bbca Compare May 13, 2021 16:27

alpadev requested a review from GeoSot May 13, 2021 16:34

alpadev force-pushed the fix-transitionend-event-handling branch from a0c2f97 to 707ef8e Compare May 13, 2021 17:09

alpadev force-pushed the fix-transitionend-event-handling branch 2 times, most recently from ca844bc to 651dca6 Compare May 20, 2021 11:43

GeoSot approved these changes May 20, 2021

View reviewed changes

alpadev mentioned this pull request May 21, 2021

Navbar collapse transition glitchy after toggling dropdown within #34066

Closed

GeoSot mentioned this pull request May 21, 2021

Collapse animation is not smooth when toggled via a button within the "collapse" element #31554

Closed

XhmikosR added has conflicts js v5 labels May 22, 2021

alpadev force-pushed the fix-transitionend-event-handling branch from 651dca6 to 9bd88b0 Compare May 24, 2021 13:28

rohit2sharma95 removed the has conflicts label May 24, 2021

alpadev force-pushed the fix-transitionend-event-handling branch from 9bd88b0 to fd016ac Compare May 24, 2021 15:16

rohit2sharma95 reviewed May 24, 2021

View reviewed changes

GeoSot mentioned this pull request May 25, 2021

Rename _queueCallback method #34095

Closed

alpadev force-pushed the fix-transitionend-event-handling branch from fd016ac to 0c1d8fd Compare May 26, 2021 02:33

alpadev force-pushed the fix-transitionend-event-handling branch from 2cf6c6f to 4f12061 Compare June 2, 2021 09:46

rohit2sharma95 approved these changes Jun 2, 2021

View reviewed changes

This was referenced Jun 2, 2021

Add an explaining comment to durationPadding for executeAfterTransition. #34168

Open

Review getTransitionDurationFromElement to implement multiple durations #34169

Open

Merge branch 'main' into fix-transitionend-event-handling

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

4901bcc

GeoSot merged commit 4a5029e into twbs:main Jun 3, 2021

alpadev deleted the fix-transitionend-event-handling branch June 3, 2021 22:02

alpadev mentioned this pull request Jun 11, 2021

Bootstrap 5 modal .fade does not finish hide animation if element within it is focused #32993

Closed

GeoSot mentioned this pull request Jun 18, 2021

Text inputs inside collapse - if input is in focus and collapse button is clicked, animation breaks #34293

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Fix handling of transitionend events dispatched by nested elements #33845

Fix handling of transitionend events dispatched by nested elements #33845

alpadev commented May 6, 2021 •

edited

Loading

mdo commented May 13, 2021

XhmikosR commented May 13, 2021

alpadev commented May 13, 2021

GeoSot commented May 14, 2021 •

edited

Loading

XhmikosR commented May 18, 2021

rohit2sharma95 May 24, 2021

alpadev May 26, 2021 •

edited

Loading

rohit2sharma95 May 26, 2021

rohit2sharma95 May 24, 2021

rohit2sharma95 May 25, 2021

alpadev May 26, 2021 •

edited

Loading

GeoSot commented Jun 2, 2021

rohit2sharma95 commented Jun 2, 2021

Fix handling of transitionend events dispatched by nested elements #33845

Fix handling of transitionend events dispatched by nested elements #33845

Conversation

alpadev commented May 6, 2021 • edited Loading

mdo commented May 13, 2021

XhmikosR commented May 13, 2021

alpadev commented May 13, 2021

GeoSot commented May 14, 2021 • edited Loading

XhmikosR commented May 18, 2021

rohit2sharma95 May 24, 2021

Choose a reason for hiding this comment

alpadev May 26, 2021 • edited Loading

Choose a reason for hiding this comment

rohit2sharma95 May 26, 2021

Choose a reason for hiding this comment

rohit2sharma95 May 24, 2021

Choose a reason for hiding this comment

rohit2sharma95 May 25, 2021

Choose a reason for hiding this comment

alpadev May 26, 2021 • edited Loading

Choose a reason for hiding this comment

GeoSot commented Jun 2, 2021

rohit2sharma95 commented Jun 2, 2021

alpadev commented May 6, 2021 •

edited

Loading

GeoSot commented May 14, 2021 •

edited

Loading

alpadev May 26, 2021 •

edited

Loading

alpadev May 26, 2021 •

edited

Loading