708814 - Should fade out videocontrols even if there's no mouse movement

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

(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"?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yeah, I think that would work.

Comment 4

[Justin Dolske [:Dolske]]

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

Comment 5

[David Seifried (:dseif)]

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

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Thanks David!

Comment 7

[David Seifried (:dseif)]

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.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 9

[David Seifried (:dseif)]

Attachment: Code, no tests

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

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Sounds like this is or is related to bug 708553.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 590984 [details] [diff] [review]
Code, no tests

Review of attachment 590984 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks good to me.

Comment 12

[David Seifried (:dseif)]

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

Comment 13

[David Seifried (:dseif)]

Attachment: Version 2

Sorry, forgot to rebase off master

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 16

[David Seifried (:dseif)]

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

Comment 17

[David Seifried (:dseif)]

Attachment: Version 3

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 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

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...

Comment 19

[Nikhil Johny]

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

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 21

[Sunny [:darkowlzz]]

Attachment: Added fadeout timer inside setFullscreenButtonState.

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 22

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 23

[Sunny [:darkowlzz]]

Attachment: 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.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 25

[Sunny [:darkowlzz]]

Attachment: Introduced a new function "onFullscreenChange" which is called when mozfullscreenchange event takes place and added a timer for hiding videocontrol when fullscreen starts.

Hope this patch does the required job. :)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55662954a5ef

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55662954a5ef

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.