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

Event dispatch bug around play/pause? Or am I just confused? #620

Closed
cameront opened this issue Jul 2, 2013 · 5 comments · Fixed by #660
Closed

Event dispatch bug around play/pause? Or am I just confused? #620

cameront opened this issue Jul 2, 2013 · 5 comments · Fixed by #660

Comments

@cameront
Copy link

cameront commented Jul 2, 2013

I've run into a problem trying to play a video from within a pause event handler. This may be a stupid thing to do, but I'd really like to understand why, and what is causing these hundreds/thousands of video pause events to be unleashed. I can't figure out why the video player constantly thinks its being paused.

I'm using Chrome Version 27.0.1453.110 on a Mac.
API Version 4.1.0

I've created a trivial plugin to demonstrate the behavior using a temporary reference to the api:
http://jsbin.com/etewed/8/edit

Warning - my browser (chrome) starts to hang by the end of the video unless I modify the source of the javascript (causing a page-refresh for the output pane).

I initially suspected that this was related to #573, but since I'm not modifying the src of the player, I don't think so anymore.

BTW, I've had a lot of fun playing around with Video.js for the last couple of days -- it's really great. Thanks!

@elad661
Copy link

elad661 commented Jul 2, 2013

I encountered this at work today, also had to do some workarounds.... I tried using chrome's developer tools to find the source but couldn't find it.

Using your example, I got the same issue in Firefox (with flash fallback!), which leads me to believe this is not a browser bug like I first suspected.

@philmosby
Copy link

I am having a similar issue with the timeupdate event in Firefox. Even though the video is not playing, timeupdate is firing repeatedly as if it were. Note that this is ONLY happening in Firefox, other browsers are working as expected.

@cameront
Copy link
Author

cameront commented Jul 3, 2013

UPDATE (2013-07-10): You can save yourself some reading by skipping ahead a few comments. This initial assessment isn't quite right.


I think I figured this out. There are really two issues, but both have to do with the way that events are handled in vjs.trigger().

1 - vjs.trigger() assumes default action will execute synchronously

In that last "else" block of vjs.trigger, the event's target element is examined for a method name matching the event.type. If there is one, and the event hasn't had its default prevented, the method gets called.

This is why the video element's 'play' and 'pause' are getting called multiple times for a single event.

Now, this would still be okay if the target was marked disabled, as the code attempts to do.

// Temporarily disables event dispatching on the target as we have already executed the handler.
targetData.disabled = true;
// Executes the default action.
if (typeof event.target[event.type] === 'function') {
  event.target[event.type]();
}
// Re-enables event dispatching.
targetData.disabled = false;

However, when this snippet executes inside of an event handler (as it always(?) does), the call to play() or pause() generates events that are handled after this handler is finished executing, so the target has been re-enabled before the event gets handled, causing it to get handled again.

I believe this could be solved by:

  1. Making sure default actions never spawn events or do anything else 'asynchronously'
    • On the one hand, this seems kind of sane to me, but really hard to police. You'd be handcuffed by the possibility that somebody could make any one of your class's methods a default action. Therefore, this doesn't seem realistic.
  2. Wrap the call to the default action in its own function inside of a timeout, which would disable the target, execute the call, then re-enable the target, so that it's guaranteed to execute in the correct context.
    • I'm just not sure if it's okay to break up event-processing like this, as it potentially allows other queued events to be processed between the firing of an event and the running of its default action.

I'm no javascript expert, so I'd love some insight from people about these... but on to the second issue.

2 - vjs.trigger() bubbles events up the dom even when event.bubbles === false

The media events triggered from the video element have the 'bubbles' property set to false (at least for me - in chrome). I believe that the 'right' thing to do would be for vjs.trigger to only recursively bubble an event if event.bubbles !== false.

@cameront
Copy link
Author

After some more investigation, I'm not as sure about the causes above.

I think the main issue is that the videojs html5 media component triggers all of the video element's media events on the player's element (usually the div created by player.createEl()).

In looking at the jQuery event handling code (disclaimer: the only other example event-handling code I've really looked at), it looks to me like the first problem I mentioned above (assuming the default action executes immediately) is handled the same way there, leading me to believe that its not the problem I thought it was.

It seems to me that this problem results from vjs proxying all media events without preventing the default action, since every event (even those resulting from a default action) calls trigger(), which in turn can cause a new default action.

I think its still a problem that simple media events bubble at all, but not a very bad one (NOTE: jquery also incorrectly bubbles events with bubbles===false). So I'm going to delete the pull request I've submitted, and propose a simpler fix - just calling preventDefault on media events before calling player.trigger() in the html5 event handler.

@heff
Copy link
Member

heff commented Jul 23, 2013

This is great, thanks for digging into it. The event lib originally came from John Resig's JS Ninja book, so it's probably very similar to jQuery's implementation. I'm surprised it bubble's events with bubbles=false too. Prevent default is a good fix.

heff pushed a commit that referenced this issue Jul 30, 2013
@heff heff closed this as completed in #660 Jul 30, 2013
heff added a commit that referenced this issue Jul 30, 2013
…vents. fixes #573, fixes #620 (duplicate bug)."

This reverts commit 15544c3.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants