Closed Bug 521890 Opened 12 years ago Closed 12 years ago

Use CSS Transitions for HTML5 videocontrols


(Toolkit :: Video/Audio Controls, defect)

Not set





(Reporter: Dolske, Assigned: Dolske)


(Depends on 1 open bug, )



(2 files, 4 obsolete files)

The current video controls roll their own animations in JS for the fading of the control bar and the sliding out of the volume controls. We should probably just do these with CSS Transitions.
In which file is the video controls handling currently done? I might take a look at it.
Oh, that changes things. After looking at fullscreen_video.xhtml I was hoping this too was built through HTML, CSS and JavaScript. I don't think I can do this.

But, when using CSS, should we use the opacity, visibility or display property? Display won't "transform", I am not sure if it is supposed to do it, and the other two simply hides it but still leaves it there.

And, for sliding the volume control, I am a bit clueless, since it got to hide behind the other controls.

(I'm sorry, turns out I wasn't much help after all.)
Attached patch Patch v.1 (WIP) (obsolete) — Splinter Review
Mmm, this sure simplifies the code.

One problem, though. Using transitions means that bug 483841 will regress, because the video controls can't set .hidden after fading the opacity to 0. [This also results in clickable controls when we're showing the error overlay, but that's probably fixable other ways. The old issue (bug 463533) of being able to click our controls even when the |controls| attribute is not set was resolved by bug 449149.]

Not sure what the best options is for that. Ideally we would fix the underlying problem, so that animations with opacity=0 don't eat any CPU. I see the Transitions draft spec has events for when transitions complete (but I don't think we implement (yet?)), the controls could listen for those to know when to set .hidden.

A third possibility would be to allow "animating" the CSS |display| property, just to have it hook into the delay timer. Perhaps something like:

  #fadeAndHide {
      -moz-transition-property: opacity, display;
      -moz-transition-duration: 200ms, 0ms;
      -moz-transition-delay:    0ms, 200ms;
  #fadeAndHide[hideme] {
      opacity: 0.0;
      display: none;

[More generally, it might be interesting to allow using transition-delay with any property. Though I can't think of any other usecases for that offhand...]
Being able to animate the display property has been requested a several times, and I actually had a two hour long struggle trying to come up with a suitable replacement for it when I was working on a layout, but failed.

"Fixing the underlying problem" sounds like the best thing to do, but I don't know how much work that would be. For your second proposal, I wonder, wasn't everything related to transitions implemented in bug 435441, including JavaScript events?

So, personally I would like to see the possibility of animating the display property, and using it to hide the video controls is a good solution (at least until transitions are fully implemented).
I haven't implemented transition events yet, but I'm planning to.

It would be pretty easy for us to support transition-delay even on properties that can't be animated.  I wonder what others on www-style would think of that.
It can probably also be fixed with animations of 'visibility' once that's implemented.
Last time I checked, "visibility: hidden" still resulted in CPU being used for a hidden animated image (whereas "display: none" did not). I suppose that's another "underlying problem" to fix...
Depends on: 531585
Depends on: 533352
I've now filed bug 533352, which is about making transition-delay apply to at least some properties which are normally "non-animatable".
Comment on attachment 414899 [details] [diff] [review]
Patch v.1 (WIP)

>diff --git a/toolkit/content/widgets/videocontrols.css b/toolkit/content/widgets/videocontrols.css

>+/* CSS Transitions */
>+.controlBar {
>+    -moz-transition-property: opacity;
>+    -moz-transition-duration: 200ms;
>+.controlBar[fadeout] {
>+    opacity: 0.0;
>+.volumeStack {
>+    -moz-transition-property: opacity, margin-top;
>+    -moz-transition-duration: 200ms, 200ms;
>+.volumeStack[fadeout] {
>+    opacity: 0.0;
>+    margin-top: 0px;
>+.statusOverlay {
>+    -moz-transition-property: opacity;
>+    -moz-transition-duration: 300ms;
>+    -moz-transition-delay: 750ms;
>+.statusOverlay[fadeout] {
>+    opacity: 0.0;

Any reason why this isn't in toolkit/themes/*/global/media/videocontrols.css? Seems to be something that themes should control.
Attached patch Patch v.2 (obsolete) — Splinter Review
Makes use of the just-landed transitionend event. Works fine, except I'm getting two events for each fadeout (for the same element, for the same property). Not sure what's going on there yet.

And yeah, I suppose I should just move the CSS to the theme CSS. This was just easier for testing.
Attachment #414899 - Attachment is obsolete: true
Does the element in question have a :before or :after pseudo-element?
Attached file Double-event testcase (obsolete) —
No before or :after pseudo-elements, but here's a plain HTML testcase that reproduces the problem. Mouse in/out of the grey area to trigger fading of the fake green control bar. [Don't mouse over the green bar, since the simple mousein/out handler gets confused.]

Each time it fades out, 2 transition end events are dispatched. Interestingly, if I toss in a getComputedStyle() before setting style.display to "none", the problem goes away.
Depends on: 537151
I filed the double events (actually double transitions) as bug 537151.
Attached patch Patch v.3 (obsolete) — Splinter Review
The double-event ends up being harmless, so I went ahead and finished up this patch.
Attachment #419077 - Attachment is obsolete: true
Attachment #419267 - Attachment is obsolete: true
Attachment #420848 - Flags: review?(enndeakin)
Attached patch Patch v.4Splinter Review
Oops, meant to s/let/var/ lest that be a problem.
Attachment #420848 - Attachment is obsolete: true
Attachment #420849 - Flags: review?(enndeakin)
Attachment #420848 - Flags: review?(enndeakin)
Will the transitioned event be dispatched if a theme didn't add the transition or a user disabled it (see bug 456620 comment 13)?
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: →
Target Milestone: --- → mozilla1.9.2a1
Version: Trunk → unspecified
Target Milestone: mozilla1.9.2a1 → ---
Attachment #420849 - Flags: review?(enndeakin) → review+
(In reply to comment #17)
> Will the transitioned event be dispatched if a theme didn't add the transition
> or a user disabled it (see bug 456620 comment 13)?

Dolske, ping?

If the binding actually depends on it, the CSS doing the transition shouldn't be in themes/ after all, or there should be a stub transition by default just to trigger the event.
Dao: Hmm, I think things can be problematic either way.

The issue is that if themes want to add a transition of another property, the syntax of -moz-transition-properties doesn't seem to have a way to extend the list of existing properties. Eg, if a previous rule has set "-moz-transition-property: opacity", setting "-moz-transition-property: color" will cause opacity to stop transitioning. You must have a single "-moz-transition-property: opacity, color" rule to do both.

So if we take this CSS out of /themes, a theme wanting to add another transition property (to styles we're transitioning) must ensure they copy our CSS from wherever it's at. Themers likely won't know about this, so will get it wrong or spend time trying to figure out why they've broken it.

If we leave the CSS in /themes, everyone has to include it in their theme and know not to remove it.

I suppose in either case we could add a loud "NOTE TO THEMERS" in our CSS to call out the issue.

Oh, and this means that changing/adding transitions in a 3.7.x update carries risk of breaking themes. [Though I can't think of a good reason why we'd be adding such CSS in an update.]

dbaron: is this problem something the CSSWG has discussed?
Dao: any preference as to which way to proceed?
I'd add a very brief (0.01s? 0.001s? don't know if there's a minimum) transition to the content CSS and let the themes override it. Themes could break this by not including opacity, but I don't see a better option.

You could also define the full transition in the content CSS and not expect themes to override it, but nothing guarantees you they'll honor this, and ultimately I think the transition /should/ be under the theme's control.
Ok, added a brief transition to the /content CSS and overrode it in /skin, per comment 22.

Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
Version: unspecified → Trunk
> Last time I checked, "visibility: hidden" still resulted in CPU being used for
> a hidden animated image (whereas "display: none" did not).

This should be easy to fix.  Was a bug ever filed on that?
Blocks: 558746
(In reply to comment #24)
> > Last time I checked, "visibility: hidden" still resulted in CPU being used for
> > a hidden animated image (whereas "display: none" did not).
> This should be easy to fix.  Was a bug ever filed on that?

Filed bug 560067.
You need to log in before you can comment on or make changes to this bug.