atomiks / tippyjs

Tooltip, popover, dropdown, and menu library
https://atomiks.github.io/tippyjs/
MIT License
11.91k stars 518 forks source link

Tippy invokes `removeEventListener` and `addEventListener` with an undefined listener #1027

Closed LazyCompiler closed 2 years ago

LazyCompiler commented 2 years ago

Bug description

The events removeEventListener and addEventListener of transitionend and webkitTransitionEnd are invoked with an undefined listener.

dom-utils.ts / line 117

['transitionend', 'webkitTransitionEnd'].forEach((event) => {
 box[method](event, listener as EventListener);
});

This function does not verify that "listener" is defined after being triggered from (createTippy.ts Line 411), that also didn't although it's an global variable that being defined only after its first run:

updateTransitionEndListener(box, 'remove', currentTransitionEndListener);
updateTransitionEndListener(box, 'add', listener);

currentTransitionEndListener = listener;

currentTransitionEndListener is a global var that is undefined at first load, so first call is always undefined.

Reproduction

CodePen link: https://codepen.io/lazycompiler/pen/rNYxBjQ

atomiks commented 2 years ago

An undefined event listener just gets ignored so I'm not sure what the bug is?

LazyCompiler commented 2 years ago

Because this data is required and not optional (removeEventListener: Source 1, Source 2, addEventListener (Source 1, Source 2).

Here is some background: I'm working on a Chrome Extension, and I decided to use Tippy.js. I found out this website I'm targeting, override window.EventTarget.prototype.removeEventListener, and they access eventHandler.listener without verifying it exists. I immediately thought it was their mistake (to assume it has value), but after reading about it, I found out this information is mandatory.

atomiks commented 2 years ago
Screen Shot 2022-02-01 at 9 25 07 pm

It's required, but the value can be undefined. I'd say it's a problem of the website overriding a built-in function without adhering to the spec (never good practice), but I'm open to changing it to help compatibility in practice.

LazyCompiler commented 2 years ago

I see; indeed, it's not very good practice.

Perhaps to minimize the footprint of Tippy.js, at least we can avoid the first empty call in line 411 before currentTransitionEndListener is defined in line 414.