Use CSS Transitions for HTML5 videocontrols

RESOLVED FIXED in mozilla1.9.3a4

Status

()

Toolkit
Video/Audio Controls
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.9.3a4
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago

Comment 1

8 years ago
In which file is the video controls handling currently done? I might take a look at it.
(Assignee)

Comment 2

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

Comment 3

8 years ago
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.)
(Assignee)

Comment 4

8 years ago
Created attachment 414899 [details] [diff] [review]
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

8 years ago
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.
(Assignee)

Comment 8

8 years ago
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...
(Assignee)

Updated

8 years ago
Depends on: 531585

Updated

8 years ago
Depends on: 533352

Comment 9

8 years ago
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.
(Assignee)

Comment 11

8 years ago
Created attachment 419077 [details] [diff] [review]
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.
Attachment #414899 - Attachment is obsolete: true
Does the element in question have a :before or :after pseudo-element?
(Assignee)

Comment 13

8 years ago
Created attachment 419267 [details]
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.
Depends on: 537151
I filed the double events (actually double transitions) as bug 537151.
(Assignee)

Comment 15

8 years ago
Created attachment 420848 [details] [diff] [review]
Patch v.3

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)
(Assignee)

Comment 16

8 years ago
Created attachment 420849 [details] [diff] [review]
Patch v.4

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)?
(Assignee)

Updated

8 years ago
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Target Milestone: --- → mozilla1.9.2a1
Version: Trunk → unspecified
(Assignee)

Updated

8 years ago
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.
(Assignee)

Comment 19

8 years ago
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?
(Assignee)

Comment 20

8 years ago
Created attachment 427490 [details]
Simple testcase for comment 19
(Assignee)

Comment 21

8 years ago
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.
(Assignee)

Comment 23

7 years ago
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
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
Version: unspecified → Trunk

Comment 24

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

Updated

7 years ago
Blocks: 558746
(Assignee)

Comment 25

7 years ago
(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.
Depends on: 620317
You need to log in before you can comment on or make changes to this bug.