707736 - Hide the cursor when it's motionless and over a full-screen video

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, enhancement)

Comment 1

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

Maybe we could implement this by having the code which bug 699719 uses to add/remove an attribute on the video which then matches a style rule to hide the mouse cursor. Or maybe there's a better way?

Comment 2

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

We'll likely just need to add |cursor:none;| to the |.controlBar[fadeout]| rules in:

/toolkit/themes/winstripe/global/media/videocontrols.css
/toolkit/themes/pinstripe/global/media/videocontrols.css

as well as do something similar for the |.controlsSpacer| (the attribute will probably need to be added to the controlsSpacer).

Comment 3

[Jan Bambach [:Jan]]

Attachment: This patch should fix Bug 707736.

Comment 4

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

Comment on attachment 580073 [details] [diff] [review]
This patch should fix Bug 707736.

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

I applied this patch and it didn't seem to have any effect?

Comment 5

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jwein and :jaws] from comment #2)
> We'll likely just need to add |cursor:none;| to the |.controlBar[fadeout]|
> rules.

I'm sorry, I didn't think clearly about this. Adding |cursor:none;| to the |.controlBar[fadeout]| rules won't work because the control bar doesn't fade out if the cursor is on top of it.

We'll need to add this rule to the .controlsSpacer but only when the controls are faded out. This will involve updating /toolkit/content/widgets/videocontrols.xml in the |startFade| function to add an attribute to the controlsSpacer saying that the controls have been faded out.

Comment 6

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 580073 [details] [diff] [review]
This patch should fix Bug 707736.

Clearing review since I gave poor explanation of what would be necessary to fix this. Please see comment #5 for more details.

Comment 7

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

Jan: Is there anything that I can do to help you out here?

Comment 8

[Jan Bambach [:Jan]]

Yep, I'm trying to figure out how to add the attribute...

Comment 9

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jan Bambach [:Jan] from comment #8)
> Yep, I'm trying to figure out how to add the attribute...

You could probably add the attribute here, but only if the element.className == "controlBar".
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#878

For example in videocontrols.xml around line 878:
if (fadeIn) {
  if (element.className == "controlBar")
    this.controlsSpacer.removeAttribute("controlsHidden");
} else {
  if (element.className == "controlBar")
    this.controlsSpacer.setAttribute("controlsHidden", true);
}

And in videocontrols.css:
.controlsSpacer[controlsHidden] {
 cursor: none;
}

Comment 10

[Jan Bambach [:Jan]]

Attachment: This patch somehow doesn't fix this bug...

...I've tried it several times, but it doesn't really work. :s

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 583814 [details] [diff] [review]
This patch somehow doesn't fix this bug...

>+                        if (element.className == "controlBar")
>+					this.controlsSpacer.removeAttribute("controlsHidden");
>+					} else {

I'm pretty sure you're missing an opening { here.

Comment 12

[Tobias (:Tobbi) Markus]

(In reply to Josh Matthews [:jdm] from comment #11)
> Comment on attachment 583814 [details] [diff] [review]
> This patch somehow doesn't fix this bug...
> 
> >+                        if (element.className == "controlBar")
> >+					this.controlsSpacer.removeAttribute("controlsHidden");
> >+					} else {
> 
> I'm pretty sure you're missing an opening { here.

I thought so too, at first, but that last closing bracket actually belongs to the fadeIn opening bracket, if I read the code correctly.

Comment 13

[Jan Bambach [:Jan]]

Just so you know: I've tested it and it doesn't fix the bug. However, it disables the control bar permanently and it makes all videos appear... pink.

Comment 14

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

Running this code gives this JavaScript error, and counting the curly brackets also shows that it is missing one.

> JavaScript error: chrome://global/content/bindings/videocontrols.xml, line 890: missing } after property list
> JavaScript error: , line 0: uncaught exception: An error occurred throwing an exception

Comment 15

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 583814 [details] [diff] [review]
This patch somehow doesn't fix this bug...

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

::: toolkit/content/widgets/videocontrols.xml
@@ +884,5 @@
> +                        if (element.className == "controlBar")
> +						this.controlsSpacer.removeAttribute("controlsHidden");
> +					} else {
> +						if (element.className == "controlBar")
> +							this.controlsSpacer.setAttribute("controlsHidden", true);

This should only be run when |!fadeIn|. Please move this down to the |else| branch.

Comment 16

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

Jan: Can you please replace the tabs with spaces?

Comment 17

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jwein and :jaws] from comment #15)
> Comment on attachment 583814 [details] [diff] [review]
> This patch somehow doesn't fix this bug...
> 
> Review of attachment 583814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/widgets/videocontrols.xml
> @@ +884,5 @@
> > +                        if (element.className == "controlBar")
> > +						this.controlsSpacer.removeAttribute("controlsHidden");
> > +					} else {
> > +						if (element.className == "controlBar")
> > +							this.controlsSpacer.setAttribute("controlsHidden", true);
> 
> This should only be run when |!fadeIn|. Please move this down to the |else|
> branch.

This comment is ambiguous. I will try to reword it:

Leave the section of the code that removes the attribute in the |if (fadeIn)| branch. Move the section of code that adds the attribute down to the |if (!fadeIn)| branch (in this case, the |else| branch).

I hope that helps.

Comment 18

[Jan Bambach [:Jan]]

Attachment: There's no need for a description I guess...

Thanks a lot for your response. I hope that this fixes the bug...

Comment 19

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

Attachment: Another solution for bug 707736

Jan: What do you think about this approach? Can you test out this patch and see if it does what we want?

Comment 20

[Jan Bambach [:Jan]]

It looks good to me. I'll let Firefox recompile over night to apply and test the patch.

Comment 21

[Jan Bambach [:Jan]]

I'm pleased to report that your patch works. Except from one little thing that could be my fault: http://screencast.com/t/KmvuQIne

Comment 22

[alexhultman]

Will this work for the youtube html5 player as well?

(In reply to Tobias Markus (:Tobbi) from comment #12)
> (In reply to Josh Matthews [:jdm] from comment #11)
> > Comment on attachment 583814 [details] [diff] [review]
> > This patch somehow doesn't fix this bug...
> > 
> > >+                        if (element.className == "controlBar")
> > >+					this.controlsSpacer.removeAttribute("controlsHidden");
> > >+					} else {
> > 
> > I'm pretty sure you're missing an opening { here.
> 
> I thought so too, at first, but that last closing bracket actually belongs
> to the fadeIn opening bracket, if I read the code correctly.

Then this code has wrong indentation. Inner code should be to the right of enclosing bracket..

Comment 23

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 583938 [details] [diff] [review]
Another solution for bug 707736

(In reply to Jan Bambach [:Jan] from comment #21)
> I'm pleased to report that your patch works. Except from one little thing
> that could be my fault: http://screencast.com/t/KmvuQIne

I'm not seeing any pink videos on my end. I think it could be something that was changed on your end. You can do "hg diff" to see all the changes that have been made in your tree.

Comment 24

[Justin Dolske [:Dolske]]

Comment on attachment 583938 [details] [diff] [review]
Another solution for bug 707736

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

Unless I'm missing something, this hides the cursor over the video whenever the controls are hidden, even if you're not in full-screen mode. I don't think it makes sense to do this _unless_ you're in full-screen mode.

Need to:

1) Do that (istr there are CSS pseudoselectors to control that)
2) reshow the cursor in FS mode when an error occurs.

Comment 25

[Justin Dolske [:Dolske]]

This is what you want, I think:

https://developer.mozilla.org/en/CSS/%3A-moz-full-screen

Comment 26

[Justin Dolske [:Dolske]]

(Sorry for the spam)

I suppose there's also the oddball case of:

<div>blah blah content blah blah <video/></div>

When the div is in DOM FS mode, the pointer probably should still stay visible (unless the page itself wants to hide it). 

So you'd probably also want to check document.mozFullScreenElement to see if it's just the video that's fullscreen.

Comment 27

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

Jan: Would you like to make the changes that Justin has recommended?

Comment 28

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 707736

I renamed the |controlsHidden| class to |hideCursor| since it is not always applied when the controls are hidden.

Comment 29

[Jan Bambach [:Jan]]

Comment on attachment 588709 [details] [diff] [review]
Patch for bug 707736

This patch looks good to me. So... feedback=+ :)

(In reply to Jared Wein [:jaws] from comment #27)
> Jan: Would you like to make the changes that Justin has recommended?
I'm really sorry for not answering your question before, I was busy with doing school-related stuff.

Comment 30

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

dolske: review ping?

Comment 31

[Justin Dolske [:Dolske]]

Comment on attachment 588709 [details] [diff] [review]
Patch for bug 707736

Hiding the cursor via controlsSpacer is a little odd, but meh.

I started to take a look at if it would look better to hide the cursor in onTransitionEnd (ie, when the bar finishes fading instead of starts fading), but it looks good as-is. Which is also a little odd, because YouTube also seems to hide the cursor at the same point, but there's a distinct blink-blink effect. Maybe the start hiding the bar a few milliseconds after the cursor goes away? Or it's just a different transition curve? Who knows.

Looks fine.

Oh, I also realized we could probably fix my last comment via some CSS like  like:

 *:-moz-full-screen-ancestor > video:-moz-full-screen { cursor: none; }

But it's probably clearer doing this via code. And we have a working patch. :)

Comment 32

[(Away until Sept 26) Jared Wein [:jaws] (please needinfo? me)]

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/11d9ee3f63eb

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/11d9ee3f63eb