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)
Toolkit
Video/Audio Controls
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)
1.45 KB,
patch
|
Details | Diff | Splinter Review | |
1.56 KB,
patch
|
Details | Diff | Splinter Review | |
2.00 KB,
patch
|
Details | Diff | Splinter Review | |
3.24 KB,
patch
|
Dolske
:
review+
jan
:
feedback+
|
Details | Diff | Splinter Review |
No description provided.
Updated•9 years ago
|
Component: DOM → Video/Audio Controls
Product: Core → Toolkit
QA Contact: general → video.audio
Comment 1•9 years ago
|
||
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•9 years ago
|
||
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]
Updated•9 years ago
|
Whiteboard: [good first bug][mentor=jwein][lang=css] → [good first bug][mentor=jwein][lang=css][lang=js]
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jan.bambach
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #580073 -
Flags: review?(jwein)
Comment 4•9 years ago
|
||
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?
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 5•9 years ago
|
||
(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•9 years ago
|
||
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)
Comment 7•9 years ago
|
||
Jan: Is there anything that I can do to help you out here?
Assignee | ||
Comment 8•9 years ago
|
||
Yep, I'm trying to figure out how to add the attribute...
Comment 9•9 years ago
|
||
(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; }
Assignee | ||
Comment 10•9 years ago
|
||
...I've tried it several times, but it doesn't really work. :s
Comment 11•9 years ago
|
||
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•9 years ago
|
||
(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.
Assignee | ||
Comment 13•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
Jan: Can you please replace the tabs with spaces?
Comment 17•9 years ago
|
||
(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.
Assignee | ||
Comment 18•9 years ago
|
||
Thanks a lot for your response. I hope that this fixes the bug...
Comment 19•9 years ago
|
||
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)
Assignee | ||
Comment 20•9 years ago
|
||
It looks good to me. I'll let Firefox recompile over night to apply and test the patch.
Assignee | ||
Comment 21•9 years ago
|
||
I'm pleased to report that your patch works. Except from one little thing that could be my fault: http://screencast.com/t/KmvuQIne
Assignee | ||
Updated•9 years ago
|
Attachment #583938 -
Flags: feedback?(jan.bambach) → feedback+
Comment 22•9 years ago
|
||
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•9 years ago
|
||
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 24•9 years ago
|
||
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-
Comment 25•9 years ago
|
||
This is what you want, I think: https://developer.mozilla.org/en/CSS/%3A-moz-full-screen
Comment 26•9 years ago
|
||
(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•9 years ago
|
||
Jan: Would you like to make the changes that Justin has recommended?
Comment 28•9 years ago
|
||
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)
Assignee | ||
Comment 29•9 years ago
|
||
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+
Comment 30•9 years ago
|
||
dolske: review ping?
Comment 31•9 years ago
|
||
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+
Comment 32•9 years ago
|
||
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]
Comment 33•9 years ago
|
||
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.
Description
•