Closed Bug 707736 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Video/Audio Controls, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: jruderman, Assigned: jan)

References

()

Details

(Whiteboard: [good first bug][mentor=jwein][lang=css][lang=js])

Attachments

(4 files, 1 obsolete file)

No description provided.
Component: DOM → Video/Audio Controls
Product: Core → Toolkit
QA Contact: general → video.audio
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?
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).
OS: Mac OS X → All
Hardware: x86_64 → All
Whiteboard: [good first bug][mentor=jwein][lang=css]
Whiteboard: [good first bug][mentor=jwein][lang=css] → [good first bug][mentor=jwein][lang=css][lang=js]
Assignee: nobody → jan.bambach
Attachment #580073 - Flags: review?(jwein)
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?
Status: NEW → ASSIGNED
(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 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.
Attachment #580073 - Flags: review?(jwein)
Jan: Is there anything that I can do to help you out here?
Yep, I'm trying to figure out how to add the attribute...
(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;
}
...I've tried it several times, but it doesn't really work. :s
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.
(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.
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.
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 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.
Jan: Can you please replace the tabs with spaces?
(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.
Thanks a lot for your response. I hope that this fixes the bug...
Attached patch Another solution for bug 707736 (obsolete) — Splinter Review
Jan: What do you think about this approach? Can you test out this patch and see if it does what we want?
Attachment #583938 - Flags: feedback?(jan.bambach)
It looks good to me. I'll let Firefox recompile over night to apply and test the patch.
I'm pleased to report that your patch works. Except from one little thing that could be my fault: http://screencast.com/t/KmvuQIne
Attachment #583938 - Flags: feedback?(jan.bambach) → feedback+
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 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.
Attachment #583938 - Flags: review?(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.
Attachment #583938 - Flags: review?(dolske) → review-
(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.
Jan: Would you like to make the changes that Justin has recommended?
I renamed the |controlsHidden| class to |hideCursor| since it is not always applied when the controls are hidden.
Attachment #583938 - Attachment is obsolete: true
Attachment #588709 - Flags: review?(dolske)
Attachment #588709 - Flags: feedback?(jan.bambach)
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.
Attachment #588709 - Flags: feedback?(jan.bambach) → feedback+
dolske: review ping?
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. :)
Attachment #588709 - Flags: review?(dolske) → review+
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/11d9ee3f63eb
Whiteboard: [good first bug][mentor=jwein][lang=css][lang=js] → [good first bug][mentor=jwein][lang=css][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/11d9ee3f63eb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=jwein][lang=css][lang=js][fixed-in-fx-team] → [good first bug][mentor=jwein][lang=css][lang=js]
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.