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.

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

fix: make EventHandler better handle mouseenter/mouseleave events #33310

Merged
merged 3 commits into from Apr 13, 2021

Conversation

alpadev
Copy link
Contributor

@alpadev alpadev commented Mar 9, 2021

This PR makes the EventHandler handle mouseenter and mouseleave more like their native counterparts.

Background

Currently the EventHandler is implemented to simply map mouseenter to mouseover and mouseleave to mouseout events - see this comment #31646 (comment)

The problem is that mouseenter and mouseleave behave different to some extent but this wasn't respected yet. As such there are situations e.g. within the tooltip plugin where one does expect nested elements not to trigger the bound event handlers.

With the PR #33289 by @RyanBerliner this is addressed although in my opinion (and I assume Ryan agrees to some degree) some general fix to the EventHandler module would be more appropriate.

TBD

With this implementation the targets are not rewritten. In some situations e.g. when the window loses focus and some nested element is hovered when it receives back focus, the targets differ between mouseenter and mouseover.

Also like asked here #33220 - are there any reasons as to why mouseover/mouseout was used in the first place?

js/src/dom/event-handler.js Outdated Show resolved Hide resolved
@RyanBerliner
Copy link
Contributor

tl;dr I think that the custom event mapping can be removed altogether. Otherwise if there is a reason it exists, it would be beneficial to add documentation inline as to why.


I think it should be understood why these 2 custom events were replaced in the first place. I've done some digging and found the origin of this event swapping to be this commit, which is converting the carousel from jquery to vanilla js. I'm assuming here that the reason these events were treated as "custom" were because of jquerys claims about implementing these events themselves. This is an assumption, so there's a good chance I'm wrong, though reading this on jquery's website is the only thing I can find that would explain to me why its treated as "custom" in bs5.

The mouseenter JavaScript event is proprietary to Internet Explorer. Because of the event's general utility, jQuery simulates this event so that it can be used regardless of browser.

Again, I'm assuming that this is what caused the reaction to treat these as custom events. I'd bet that the carousel was the first component with these mouseenter/mouseleave events to be converted to vanilla js, so the event swapping was set up at this point. Then, the fact that the actual behavior was never implemented correctly wasn't caught because in the case of the carousel, it doesn't actually matter! It still works no matter what mouse event is used.

From what I can tell jquery's claim about these events are no longer correct (at least for the browsers that bs5 supports), so I don't think any custom event mapping is necessary, and this PR can simply be removing the mapping altogether.

@alpadev
Copy link
Contributor Author

alpadev commented Apr 1, 2021

@RyanBerliner nice investigation there! And your conclusion about the why seems reasonable. 👍

Initially, my naive me had the same thoughts.😊

But there is a problem though by simply removing the event mapping. With capturing listeners, that we use for our delegated handlers, the mouseenter and mouseleave events behave a bit different - like a mixture of mouseover/mouseout and mouseenter/mouseleave. (Some codepen to check for yourself.)

So they need some sort of special treatment I guess. I can try to optimize it a bit, so to use the native events and only fix the behaviour in the delegated version.

WDYT?

@RyanBerliner
Copy link
Contributor

RyanBerliner commented Apr 2, 2021

Gotcha - I hadn't considered delegated mouseenter/mouseleave events (in any event phase really, capturing or not). I agree that if you want delegated mouseenter/mouseleave handlers to act in a meaningfully different way than mouseover/mouseout, you can't rely on the events native implementation alone. Disregard my previous notes! And if my understanding of the goal here is correct, I think adding a short message like this near the customEvents = {} assignment would provide a lot of clarity.

If we want delegated mouseenter/mouseleave handlers to act in a meaningful way, we can't rely on their native implementation alone.

EDIT: Said in another way - If we want to leverage event delegation for mouseenter/mouseleave events, we must force the handlers to attach to the capturing phase. Although, as I write this, I'm thinking maybe that's all that's needed? You should be able to use the events at they are and just force useCapture?

EDIT AGAIN: But using event delegation for native mouseenter/mouseleave events on the capturing phase is pretty much identical to use mouseover/mouseout events, but with performance issues since native mouseover seems to be more performant than a mouseenter being added on an element with many children. I've successfully talked myself into exactly what you've done here 😆

@XhmikosR XhmikosR added this to Inbox in v5.0.0 via automation Apr 3, 2021
@XhmikosR XhmikosR moved this from Inbox to Review in v5.0.0 Apr 9, 2021
@alpadev alpadev marked this pull request as ready for review April 11, 2021 20:12
@alpadev alpadev requested a review from a team as a code owner April 11, 2021 20:12
@@ -113,7 +114,7 @@ function bootstrapDelegationHandler(element, selector, fn) {

if (handler.oneOff) {
// eslint-disable-next-line unicorn/consistent-destructuring
EventHandler.off(element, event.type, fn)
EventHandler.off(element, event.type, selector, fn)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewers, this fixes a bug with the one time delegated event handlers. Added a test to the specs too.

@@ -22,6 +22,7 @@ const customEvents = {
mouseenter: 'mouseover',
mouseleave: 'mouseout'
}
const customEventsRegex = /^(mouseenter|mouseleave)/i
Copy link
Member

Choose a reason for hiding this comment

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

childish comment: better leave it as string inline, instead of use a var. Is very specific (either way is ok)

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't matter, though, since the minifier should already inline it :)

v5.0.0 automation moved this from Review to Approved Apr 12, 2021
@XhmikosR XhmikosR merged commit db32b23 into twbs:main Apr 13, 2021
v5.0.0 automation moved this from Approved to Done Apr 13, 2021
alpadev added a commit to alpadev/bootstrap that referenced this pull request Apr 18, 2021
there is a regression introduced by twbs#33310 - this would have catched that
alpadev added a commit to alpadev/bootstrap that referenced this pull request Apr 18, 2021
…wbs#33310

the old logic only worked for parent-child movement since it checked for the relatedTarget to contain the delegateTarget - this should be fixed with this
XhmikosR added a commit that referenced this pull request Apr 19, 2021
…ed by #33310 (#33679)

* test: update spec for sibling adjacent mouseenter/mouseleave events

there is a regression introduced by #33310 - this would have catched that

* fix: fixup regression for mouseenter/mouseleave events introduced by #33310

the old logic only worked for parent-child movement since it checked for the relatedTarget to contain the delegateTarget - this should be fixed with this

Co-authored-by: XhmikosR <xhmikosr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants