Bug 699719 added code to fade out the video controls if there's no mouse movement for 2 seconds, which is good, but it appears that this only works if there's mouse movement first. For example: 1. Open http://pearce.org.nz/full-screen/ 2. Without touching the mouse, press the 'v' key, the video will go full-screen. 3. Wait 2 seconds, note that the video controls remain visible. 4. Move the mouse, wait 2 more seconds and note video controls hide. How about we just start the fade out timer whenever the video controls are shown while the mouse is hovering over the video, and reset the timer whenever there's mouse movement? (You could detect whether the mouse is hovering over the video using video.mozMatchesSelector("video:hover")). We don't want to hide the video controls if the mouse doesn't mouseover when the page loads. I think that would handle this case?

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #0) > (You could detect whether the mouse is > hovering over the video using video.mozMatchesSelector("video:hover")). See http://pearce.org.nz/full-screen/ for an example of this.

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #1) > (In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #0) > > (You could detect whether the mouse is > > hovering over the video using video.mozMatchesSelector("video:hover")). > > See http://pearce.org.nz/full-screen/ for an example of this. I'm not sure at what point we could make that check. Within "mozfullscreenchange"?

Yeah, I think that would work.

From bug 699719 comment 15: "2) Related, if you position the mouse strategically, and then use the keyboard to switch to a tab with a <video> under the mouse, what happens?" Now we know! Answer: cpearce files a bug like this. :D

Also willing to attempt this one if someone can assign it to me.

Thanks David!

I took a look at Chris' demo he posted above and I'm definitely seeing the issue. What I'm wondering is if the mouse pointer should disappear with the controls ( when it's fullscreen ) or if that is undesired behaviour. Just a thought.

(In reply to David Seifried (:dseif) from comment #7) > I took a look at Chris' demo he posted above and I'm definitely seeing the > issue. What I'm wondering is if the mouse pointer should disappear with the > controls ( when it's fullscreen ) or if that is undesired behaviour. Yeah, I think what we want is when we see the mozfullscreenchange event, we start the timer to hide the controls. When the controls hide, the mouse cursor will hide.

This is a first pass at the code with no tests. I ran into an interesting issue when testing against the link Chris posted above. When using the keybinding to trigger the video to go into fullscreen ( in Chris' demo its the 'v' key ). What happens is that the first fullscreen check is not fired. This is due to the video.mozMatchesSelector("video:hover")) check that I have. It seems that there is no hover happening after the first time the fullscreen is triggered, but is fine there after. The odd thing is that if you trigger fullscreen by clicking the fullscreen button, this problem doesn't occur. I'm going to keep searching as to what the issue is here and also write some tests, but I figured I would throw this up to get some feedback

Sounds like this is or is related to bug 708553.

Comment on attachment 590984 [details] [diff] [review] Code, no tests Review of attachment 590984 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good to me.

Thanks, I'll add some tests tonight and put it up for review.

Sorry, forgot to rebase off master

I built this patch. It doesn't solve the following cases: 1. Go video fullscreen, leave mouse cursor over video for several seconds (not over controls), move mouse cursor over controls, then move cursor off controls, controls don't hide. 2. Go video fullscreen, leave mouse cursor over controls for several seconds, then move mouse over video, controls don't hide.

(In reply to Chris Pearce (:cpearce) from comment #14) > I built this patch. It doesn't solve the following cases: > > 1. Go video fullscreen, leave mouse cursor over video for several seconds > (not over controls), move mouse cursor over controls, then move cursor off > controls, controls don't hide. > 2. Go video fullscreen, leave mouse cursor over controls for several > seconds, then move mouse over video, controls don't hide. These failures could likely be coming from the patch that got landed in bug 729111.

Alright i'll take a look into it, sorry about that.

Alright I think I fixed everything now. I was hitting a few issues with errors being thrown to console with the way things were currently, so that is why the change to _hideControlsFn and the extra check for this.scrubber.valueChanged !== undefined were necessary. I'm sure there is a better, more efficient way to do what I'm doing without the anonymous functions ( probably using bind or something ), but this works. No errors are thrown to console anymore regarding Utils being undefined ( as it's now a passed in arguement ) and this.scrubber.valueChanged is not a function. Looking forward to review :)

Comment on attachment 613866 [details] [diff] [review] Version 3 Review of attachment 613866 [details] [diff] [review]: ----------------------------------------------------------------- Unfortunately this didn't work for me in the case where I click on a video's fullscreen button and then don't move the mouse. I think that's because of bug 708553, which would cause mouseover not to fire. We'll probably need to wait until that's fixed before this can be fixed. That's a hard bug to fix, but it's on my list of things to do, somewhere near the top...

hi i am new, can you please mention the files that i need to look into to fix this bug?

Hi Nikhil, the code that controls the video controls is in JavaScript, and lives in the files in toolkit/content/widgets/videocontrols.xml i.e.: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml

Hi, I went through the code and made small change which seems to fix things up. Please have a look. It works as expected on my nightly build when tested on the video at http://pearce.org.nz/full-screen/ .

Comment on attachment 728750 [details] [diff] [review] Added fadeout timer inside setFullscreenButtonState. Review of attachment 728750 [details] [diff] [review]: ----------------------------------------------------------------- With this patch applied, moving the mouse over the video causes the cursor to disappear every 2 seconds even though I am still moving my mouse. I didn't have this problem before the patch. Also, I think that you should introduce a new "onFullscreenChange" function that will run this code and then call setFullscreenButtonState.

Hi, Sorry to inform that I am unable to figure out how to solve this bug even after spending a lot of time with it. This patch works same as the previous patch. Just added a new function |onFullscreenChange| which calls |setFullscreenButtonState| when |mozfullscreenchange| event takes place. Thanks to cpearce, I understood the timer creation using |setTimeout| and destroying it using |clearTimeout|. I couldn't figure out why |_hideControlsTimeout| added by me in |onFullscreenChange| was not being cleared by the |clearTimeout| in |onMouseMove|. Would be great if someone can help me out in this.

Comment on attachment 737894 [details] [diff] [review] Introduced a new function "onFullscreenChange" which is called when mozfullscreenchange event takes place (In reply to Sunny [:darkowlzz] from comment #23) > Created attachment 737894 [details] [diff] [review] > Introduced a new function "onFullscreenChange" which is called when > mozfullscreenchange event takes place > > Hi, > Sorry to inform that I am unable to figure out how to solve this bug even > after spending a lot of time with it. > This patch works same as the previous patch. Just added a new function > |onFullscreenChange| which calls |setFullscreenButtonState| when > |mozfullscreenchange| event takes place. > Thanks to cpearce, I understood the timer creation using |setTimeout| and > destroying it using |clearTimeout|. I couldn't figure out why > |_hideControlsTimeout| added by me in |onFullscreenChange| was not being > cleared by the |clearTimeout| in |onMouseMove|. > > Would be great if someone can help me out in this. Hi, I tested out the patch and it seems to work fine for me. I don't understand why I was seeing an issue with the previous patch. > this.fullscreenButton.setAttribute("aria-label", value); > >- if (this.isVideoInFullScreen()) >+ if (this.isVideoInFullScreen()) { > this.fullscreenButton.setAttribute("fullscreened", "true"); >- else >+ } >+ else { > this.fullscreenButton.removeAttribute("fullscreened"); >+ } These changes here aren't needed. You can undo these changes and resubmit the patch for review.

Hope this patch does the required job. :)

Verified fixed in [Mozilla/5.0 (Windows NT 6.2; WOW64; rv:23.0) Gecko/20130418 Firefox/23.0]. Good work Sunny! Thanks for the patch.