Player.mouseover/mouseout events handlers will fire multiple times if play button is clicked multiple times #173

Closed
ghost opened this Issue Apr 18, 2012 · 4 comments

Comments

Projects
None yet
2 participants
@ghost

ghost commented Apr 18, 2012

In file video.js, version 3.2.0, line 753, you have this:

player.addEvent("play", this.proxy(function(){
  this.fadeIn();
  this.player.addEvent("mouseover", this.proxy(this.fadeIn));
  this.player.addEvent("mouseout", this.proxy(this.fadeOut));
}));

This is probably a code smell. Each time the player fires the play event (e.g. user clicking play button) an extra handler is attached to mouseover/mouseout events. I verified this by adding console.log() statements inside fadeIn() and fadeOut(). I clicked the play/pause button about, say, 8 times. After that, every time I hovered the mouse over and out of the video I noticed 8 log messages in console.

Secondly, the above mentioned lines will cause the controls to appear and remain stuck on the screen if video is set to autoplay. To un-stick, mouse over-and-out of the player. This is annoying; I don't think everyone can figure out how to get rid of video controls.

On youtube for example, if controls are set to autohide they will do so automatically in a few seconds (see http://jsfiddle.net/FBeRn/1/). Depending on how you work around the first problem, you might be able to address this issue at the same time.

Owner

heff commented Apr 18, 2012

Woah, good catch. On it. -heff

On Apr 17, 2012, at 11:28 PM, schwarzenneger wrote:

In file video.js, version 3.2.0, line 753, you have this:

player.addEvent("play", this.proxy(function(){
this.fadeIn();
this.player.addEvent("mouseover", this.proxy(this.fadeIn));
this.player.addEvent("mouseout", this.proxy(this.fadeOut));
}));

This is probably a code smell. Each time the player fires the play event (e.g. user clicking play button) an extra handler is attached to mouseover/mouseout events. I verified this by adding console.log() statements inside fadeIn() and fadeOut(). I clicked the play/pause button about, say, 8 times. After that, every time I hovered the mouse over and out of the video I noticed 8 log messages in console.

Secondly, the above mentioned lines will cause the controls to appear and remain stuck on the screen if video is set to autoplay. To un-stick, mouse over-and-out of the player. This is annoying; I don't think everyone can figure out how to get rid of video controls.

On youtube for example, if controls are set to autohide they will do so automatically in a few seconds (see http://jsfiddle.net/FBeRn/1/). Depending on how you work around the first problem, you might be able to address this issue at the same time.


Reply to this email directly or view it on GitHub:
zencoder#173

@ghost

ghost commented Apr 18, 2012

I was trying a few things myself in the meantime. See if you find this patch useful:

753,757c753,758
<     player.addEvent("play", this.proxy(function(){
<       this.fadeIn();
<       this.player.addEvent("mouseover", this.proxy(this.fadeIn));
<       this.player.addEvent("mouseout", this.proxy(this.fadeOut));
<     }));
---
>     //player.addEvent("play", this.proxy(function(){ // event fire cases (i) by big button for manual-play videos (ii) automatically for auto-play videos (iii) other ways
>     //  this.fadeIn();                               // show controls when playback starts; good for case (i) -- debatable for case (ii) 
>     //  setTimeout(this.proxy(this.fadeOut), 2000);  // hide controls after 2 sec; good for case (ii) if we chose to call fadeIn -- bad for case (i) mouse might still be over the video after the timeout
>     //}));
>     player.addEvent("mouseover", this.proxy(this.fadeIn));
>     player.addEvent("mouseout", this.proxy(this.fadeOut));
768,769c769,772
<     this._super();
<     this.player.triggerEvent("controlsvisible");
---
>     if(this.player.bigPlayButton.el.style.display == "none"){ // if there is a better way to check if video (a) has not started or (b) has ended then use that check instead
>       this._super();
>       this.player.triggerEvent("controlsvisible");
>     }

Any reason something like this won't solve the problem?

player.addEvent("play", this.proxy(function(){
  this.fadeIn();
  if (!player.hasAddedPlayEvents) {
    this.player.addEvent("mouseover", this.proxy(this.fadeIn));
    this.player.addEvent("mouseout", this.proxy(this.fadeOut));
    player.hasAddedPlayEvents = true;
  }
}));

@heff heff closed this in a787b5c May 3, 2012

Owner

heff commented May 3, 2012

I just used the 'one' method, which binds a listener only once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment