521890 - Use CSS Transitions for HTML5 videocontrols

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Justin Dolske [:Dolske]]

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.

Comment 1

[d]

In which file is the video controls handling currently done? I might take a look at it.

Comment 2

[Justin Dolske [:Dolske]]

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml

Comment 3

[d]

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

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

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

Comment 5

[d]

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

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It can probably also be fixed with animations of 'visibility' once that's implemented.

Comment 8

[Justin Dolske [:Dolske]]

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

Comment 9

[d]

I've now filed bug 533352, which is about making transition-delay apply to at least some properties which are normally "non-animatable".

Comment 10

[Dão Gottwald [:dao]]

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.

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does the element in question have a :before or :after pseudo-element?

Comment 13

[Justin Dolske [:Dolske]]

Attachment: Double-event testcase

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.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed the double events (actually double transitions) as bug 537151.

Comment 15

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

The double-event ends up being harmless, so I went ahead and finished up this patch.

Comment 16

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Oops, meant to s/let/var/ lest that be a problem.

Comment 17

[Dão Gottwald [:dao]]

Will the transitioned event be dispatched if a theme didn't add the transition or a user disabled it (see bug 456620 comment 13)?

Comment 18

[Dão Gottwald [:dao]]

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

Comment 19

[Justin Dolske [:Dolske]]

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?

Comment 20

[Justin Dolske [:Dolske]]

Attachment: Simple testcase for comment 19

Comment 21

[Justin Dolske [:Dolske]]

Dao: any preference as to which way to proceed?

Comment 22

[Dão Gottwald [:dao]]

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.

Comment 23

[Justin Dolske [:Dolske]]

Ok, added a brief transition to the /content CSS and overrode it in /skin, per comment 22.

Pushed http://hg.mozilla.org/mozilla-central/rev/c41ee41f58ff

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

Comment 25

[Justin Dolske [:Dolske]]

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