swup / preload-plugin

A swup plugin for preloading pages to speed up navigation 🚀
https://swup.js.org/plugins/preload-plugin
MIT License
14 stars 14 forks source link

`mouseover` is triggered multiple times on links with complex (nested) markup #49

Closed hirasso closed 1 year ago

hirasso commented 1 year ago

Describe your issue: Right now, since mouseover bubbles, it triggers multiple times on links with complex markup:

<a href="/my-path/">
  <span>Link Text</span>
</a>

Here swup.on('hoverLink') will be triggered as for the <a> as well as for the <span>

A consumer-side workaround for that is to disable pointer events on the link's children:

a * {
  pointer-events: none;
}

But maybe we can find a better solution that doesn't depend on the consumer-side implementation? mouseenter wouldn't work with delegation, since it doesn't bubble.

daun commented 1 year ago

That's a bit surprising. Doesn't that also mean that swup's normal click handler will fire multiple times in similar cases of a span nested inside a div? I assumed the delegate library would handle that by just passing the actual element as delegateTarget...

hirasso commented 1 year ago

It was suprising to me, too. Turns out my description is a bit wrong, MDN has a better one. It's not due to bubbling, but due to the fact that mouseover fires for the link itself as well as for all children of the link if the mouse moves over them.

I quickly hacked something together that reduces the fired event to 1 for each hover (tested):

onMouseOver = (event) => {
    const target = event.delegateTarget;
    // Return early if the mouse already is on the link
    if (target.hasAttribute('data-swup-mouse-over')) {
        return;
    }
    // Set the data attribute
    target.setAttribute('data-swup-mouse-over', '');
    // Remove the data attribute on mouseleave
    target.addEventListener(
        'mouseleave',
        () => {
            target.removeAttribute('data-swup-mouse-over');
        },
        { once: true }
    );
        // Will only fire once per mouseover
    console.log('mouseOver');

    this.swup.triggerEvent('hoverLink', event);
    this.preloadLink(event.delegateTarget);
};

A bit nasty, I know... But the best I could come up with until now. Maybe you can find a better approach? Another alternative could be to augment the element itself (dummy code):

if (event.delegateTarget.__swupMouseOver) return;
// ...
hirasso commented 1 year ago

Actually, I was wrong again. The delegation of mouseenter actually also works, if using the capture phase:

swup.delegatedListeners.mouseover = delegate(
    document.body,
    swup.options.linkSelector,
    'mouseenter',
    this.onMouseOver.bind(this),
    { capture: true } // << this!
);

...and the handler here is also fired multiple times for the children! 🤦‍♂️

Something weird with the delegation library maybe? Anyhow, the above code will also work in this scenario.

daun commented 1 year ago

For mouseenter, at least we could compare event.target and event.delegateTarget and ignore it if they aren't the same element, no? Not sure how that holds up for children that cover the link completely vs. children that don't.

daun commented 1 year ago

So this seems to do the trick when using mouseenter:

swup.delegatedListeners.mouseenter = delegate(
  document.body,
  swup.options.linkSelector,
  'mouseenter',
  this.onMouseEnter.bind(this),
  { capture: true } 
);
onMouseEnter = (event) => {
  if (event.target !== event.delegateTarget) {
    return;
  }
  // Return early on devices that don't support hover
  if (!this.deviceSupportsHover()) return;
  this.swup.triggerEvent('hoverLink', event);
  this.preloadLink(event.delegateTarget);
};
daun commented 1 year ago

Interestingly, event.stopPropagation() wouldn't help here, but that's as clean as it gets I reckon.

hirasso commented 1 year ago

Did you try the same check with the original listener, namely mouseover and capture: false?

daun commented 1 year ago

Yes, that fires twice, but that's to be expected from what I read. Once when entering the link, and once when returning from the inner span to the link. There's probably no way around that since the event targets always match.

Is there a reason not to go with the mouseenter version above? I've read somewhere it's much better for performance as well since it triggers less often.

hirasso commented 1 year ago

Nice! Let's switch to mouseenter then. I just asked out of curiosity :)