Fix collapse with focused input #33695
Conversation
| // if event is dispatched by a nested element reschedule the event handler | ||
| if (event.target !== this._element) { | ||
| EventHandler.one(this._element, 'transitionend', complete) | ||
| emulateTransitionEnd(this._element, transitionDuration) |
There was a problem hiding this comment.
From what I can tell, it should work without emulateTransitionEnd but the test did fail in that case. Either we leave this here or dispatch the event manually in the spec. Not sure what is preferred.
There was a problem hiding this comment.
If it's only for our tests, we should handle that there. But only if we are 100% sure it affects the tests only :)
There was a problem hiding this comment.
@XhmikosR appreciate your response. That made me think a little more about this. 👍
So here is the thing. If I understand the purpose of emulateTransitionEnd() correctly, it's some kind of fallback for browsers that don't support transitionend events natively. Since it uses the triggerTransitionEnd() function, which dispatches a custom transitionend event that doesn't bubble, it shouldn't be possible to reach that if-block in those browsers.
IMO event.target !== this._element can only be true if the event is coming from a nested element. In that case calling emulateTransitionEnd() is kind of irrelevant, because if it's a bubbling event it's likely because there is native support.
But there is more to emulateTransitionEnd(). It will queue triggerTransitionEnd(), which is only called after the transition-duration and in case no other transitionend event happened during that duration. Although it doesn't care if the transitionend event came from a nested element. (Maybe it makes sense to change it there too, but this could introduce problems all over the place. 🤔)
To summarize the behavior:
In a browser that supports transitionend natively, there will be two events dispatched by the browser. One from the input (that bubbles) and another from the collapse. The event from the input will be handled by the if-condition and the event from the collapse will run complete like it used to. emulateTransitionEnd() (within the if-block) shouldn't do anything here because it's cancelled by the collapse's event so it wouldn't be necessary to put it there IMO.
In a browser that doesn't support transitionend, there will be no event for the input but only for the collapse dispatched by emulateTransitionEnd(). So the if-condition is never met and complete will just go through.
In our unit tests, we don't rely on the transitions in our CSS but only on those triggered by JS. In this specific case, in the spec, I'm manually dispatching the event (that bubbles like the native ones) on the input. This prevents emulateTransitionEnd() from dispatching a custom transitionend (on the collapse element), so complete would be called only once by the event from the input. This event branches through the if-condition and only because I'm calling emulateTransitionEnd() within the if-block it's queuing another (custom) transitionend event that will call complete a second time. This could be replaced with a manual dispatched event in the spec.
So..
- Either I will leave it like it is..
- Or remove
emulateTransitionEnd()from the if block and dispatch the event manually in the spec.. - Or change
emulateTransitionEnd()to ignore events from nested elements which prevents it from dispatching the customtransitionend. This means I wouldn't have to callemulateTransitionEnd()within the if-block nor manually dispatch an event in the spec.
Your call😊
P.S. I hope it makes sense what I wrote here
|
Closing this in favor of #33845 |
Closes #33682
Background
Our inputs are using transitions for the focus effect. If a nested input got focus right before clicking the collapse button, the input triggers a transitionend event that causes the transition of the collapse module to end prematurely.