Duplicate Events at window

[DanWilkerson]

Expected behaviour

A single 'play' event should be capture-able at the window when an HTML5 player is interacted with.

Actual behaviour

Plyr emits a duplicate CustomEvent with a special property pointing to the Plyr instance.

Environment

• Browser:
• Version:
• Operating System:
• Version:

Steps to reproduce

• Add a capturing listener window.addEventListener('play', console.log, true);
• Click the Play button

[DanWilkerson]

I'm writing a library that listens for 'play', 'pause', and 'timeupdate' events at the window level via capture, and Plyr causes my listener to fire twice.

I could put logic in place to determine if the event is "synthetic", but it seemed to me that this behavior might be an unexpected surprise for others, too. It seems this is in place to allow setting a plyr property without messing with the original event, which the developer can later use to retrieve a reference to the Plyr player that the element is associated with.

I'd propose that Plyr not create the synthetic event and instead let the constructor act as a getter (or create a getter method). YouTube's API behaves in this manner. Then the user could use the target of the Event to retrieve the Plyr reference. I'm certain I don't have all the facts, so if this doesn't make sense I will close this issue, but I'm happy to put together a PR if this is a good idea.

[friday]

I also like adding delegated ("ajax safe") event listeners and think this should be working.

I tried understanding why this is happening but couldn't quite follow the logic in stack traces and listeners.js, which is actually quite complex. With play, it's happening in these two places:

        // Run default and custom handlers
        const proxy = (event, defaultHandler, customHandlerKey) => {
            const customHandler = this.player.config.listeners[customHandlerKey];
            const hasCustomHandler = utils.is.function(customHandler);
            let returned = true;

            // Execute custom handler
            if (hasCustomHandler) {
                returned = customHandler.call(this.player, event);
            }

            // Only call default handler if not prevented in custom handler
            if (returned && utils.is.function(defaultHandler)) {
                defaultHandler.call(this.player, event);
            }
        };

        // Proxy events to container
        // Bubble up key events for Edge
        utils.on(this.player.media, this.player.config.events.concat([
            'keyup',
            'keydown',
        ]).join(' '), event => {
            let detail = {};

            // Get error details from media
            if (event.type === 'error') {
                detail = this.player.media.error;
            }

            utils.dispatchEvent.call(this.player, this.player.elements.container, event.type, true, detail);
        });

According to stack traces pause (which is also duplicated) doesn't go through the proxy.

keyup and keydown doesn't bubble at all in my tests, which isn't what this particular part of the code reads like to me.

[sampotts]

I'm not a big fan of the listeners object and my preference would be to just rely on events with event.preventDefault() where required as this whole proxy thing is a pain to maintain. I'd also prefer to not have to catch and re-throw events but if I remove them, people whine and I think they have their uses :) The problem is the events on media elements such as <video> and <audio> don't bubble by default so I have to catch and re-throw them...

[friday]

I see. So I guess this in turn means useCapture won't behave as expected, but also that it isn't needed to catch the the events from the root element. I don't like it either, but I can certainly live with it. :/

[DanWilkerson]

Alternatively, namespacing the event would also solve the issue in my mind - 'plyr:play' instead of 'play'. That, or those who are using the bubbling event could simply add an event listener to the audio or video element directly.

I'm clearly short on details on a lot of the internals here, so apologies if these are truly awful suggestions; hard to know from the outside looking in 😄

[friday]

Prefixing would break current listeners. I think the easiest thing that could be done without breaking or introducing a new setting (and the added complexity, maintenance and testing that comes with it) is to flag the CustomEvent so you could filter them out (if needed).

For example event.syntheticBubble = true.

Since this is primarily in your interest, perhaps you could create a PR?

(Sam is the maintainer and calls the shots. I just like to keep the ball rolling)

[sampotts]

Isn't it possible to namespace events using a . and the event type will appear the same if you listened for play for example and we fire play.plyr or something. That's how bootstrap does it with jQuery but not sure if it's possible with good old fashioned JavaScript.

[sampotts]

What I don't get though, is media events don't bubble so you should only get events that Plyr has fired. For example:

https://developer.mozilla.org/en-US/docs/Web/Events/play

Bubbles: No

Unless I'm completely missing something :)

[friday]

I'm pretty sure namespaces were added by jquery's event handling, but not how.

If I understand it correctly useCapture isn't affected by whether or not events "bubbles". Since capture goes in the opposite direction this is considered a different thing.

[DanWilkerson]

Yes, it would be a breaking change, which would mean a version bump, but those listeners could then fix themselves.

Media events don't bubble, but they do capture; Plyr events do both. That's how I'm catching two - the original event, which captures down, then the Plyr event, which captures and then bubbles.

[DanWilkerson]

Oh, also, there's an easy fix to check the event - Event.isTrusted will be false if the Event is not natively triggered. That's only useful for folks who are aware of the issue, though, and I'm concerned about those who aren't.

[friday]

I suggest you go with that then. At least for now people have this issue to help them in case others run into the same problem, but since it's not been brought up before it's probably not a common use case.

I think this should make the v4 wishlist/candidate roadmap however (though there probably isn't any).

[DanWilkerson]

Ok, fair enough. Thanks for your time!

[friday]

No, Thank you! It's a valid concern imo (again, my opinions are not official).

[sampotts]

I'll still take a look and see if I can resolve this another way.

[sampotts]

I'll leave it open as a reminder