461680 - Improve video control fade in/out animation

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Mockup

The current video controls don't fade in or out very well. It feels slow, and doesn't account for callback timer jitter. Other UI (eg tab strip animations) is better at this.

I was also interested in playing around with the rate at which the controls fade (ie, opacity = f(x) for various functions). Linear fades feel kind of sluggish, other curves feel snappier.

Attached is a mockup that allows controlling the rate and overall time of the fade animation. Power + 250ms feel about right to me.

Comment 1

[Justin Dolske [:Dolske]]

(Oops, you need to edit the time field or else it actually defaults to 450.)

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

Built ontop of the followup patch in bug 448909.

Also fixed an issue here where mousing between the controls and rest of video cause some flicker, because the code was confused (the child node filtering wasn't working right).

This is just using the Math.cos() fade rate for now.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Cleaned up, also fixes bug 463533 by setting .visibility to cause clicks to be ignored.

Comment 4

[Neil Deakin]

Comment on attachment 347495 [details] [diff] [review]
Patch v.2

diff --git a/toolkit/content/widgets/videocontrols.xml b/toolkit/content/widgets/videocontrols.xml
--- a/toolkit/content/widgets/videocontrols.xml
+++ b/toolkit/content/widgets/videocontrols.xml
         <spacer flex="1"/>
-        <hbox id="controlsBackground">
+        <hbox id="controlBar">

You shouldn't be using id in xbl. Two of these in a document would mean two elements with that id. Usually, we use anonid for this, or a class for css purposes.

             <image id="playButton"
                    onclick="if (event.button == 0) this.parentNode.parentNode.Utils.togglePause();"/>
             <box id="controlsMiddle" flex="1"/>
             <image id="muteButton"
                    onclick="if (event.button == 0) this.parentNode.parentNode.Utils.toggleMute();"/>

Why are these images and not buttons? Are they not meant to be keyboard-accessible?

Also, controlsMiddle looks like it should be a <spacer>.  

+                    // Use .visibility to ignore mouse clicks when hidden.
+                    if (self.controlsVisible)
+                        self.controlBar.style.visibility = "visible";
+                    else
+                        self.controlBar.style.visibility = "hidden";

Are you using visibility rather than hidden/collapsed because the layout is affected otherwise? If the controls were buttons, they could just be disabled.
 
-            this.Utils.animationDirection = 1;
-            if (!this.Utils.animationTimer)
-                this.Utils.animationTimer = setInterval(this.Utils.fadeControls, 50, this.Utils);
+            this.Utils.log("Fading in controls...");
+
+            var directionChange = (this.Utils.fadeDirection != this.Utils.FADE_IN);
+            this.Utils.fadeDirection = this.Utils.FADE_IN;

fadeDirection is never set to anything except FADE_IN or FADE_OUT. Why not just check fadeDirection == FADE_OUT?

+            // When switching direction mid-fade, adjust fade time for current fade state.
+            if (directionChange && this.Utils.fadeTime)
+                this.Utils.fadeTime = this.Utils.FADE_TIME_MAX - this.Utils.fadeTime;
+
+            if (!this.Utils.fadeTimer)
+                this.Utils.fadeTimer = setInterval(this.Utils.fadeControls,
+                                                   this.Utils.FADE_TIME_STEP, this.Utils);
             ]]>
         </handler>
 
You could combine the mouseover and mouseout code together into a single function in Utils, since they differ by very little.

Comment 5

[Justin Dolske [:Dolske]]

(In reply to comment #4)

> You shouldn't be using id in xbl. Two of these in a document would mean two
> elements with that id. Usually, we use anonid for this, or a class for css
> purposes.

Yeah, the other Neil caught this. It actually seems to work now (at least, doc.getElementById doesn't pick any of them up, which was his concern), but I agreed we should change them anyway. I'll do this in a followup patch, instead of cluttering this one.

> Why are these images and not buttons?

Hmm, no reason. I guess they should be!

> Also, controlsMiddle looks like it should be a <spacer>.  

This is just a placeholder for bug 462113 anyway.

> +                    // Use .visibility to ignore mouse clicks when hidden.
> +                    if (self.controlsVisible)
> +                        self.controlBar.style.visibility = "visible";
> +                    else
> +                        self.controlBar.style.visibility = "hidden";
> 
> Are you using visibility rather than hidden/collapsed because the layout is
> affected otherwise? If the controls were buttons, they could just be disabled.

.visibility suppressed the mouse clicks, that was the only goal. Hmm, I wonder if that suppresses keyboard input too. Disabling the controls might just be better.

Comment 6

[Justin Dolske [:Dolske]]

(In reply to comment #5)

> > Why are these images and not buttons?
> 
> Hmm, no reason. I guess they should be!

Hmm, *sadface*. If I do that, I get an exception when clicking it. EG:

JavaScript error: , line 0: Permission denied for <http://www.double.co.nz> to call method XULElement.QueryInterface on <http://www.double.co.nz>.

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Hmm. Can't set element.collapsed, or else I get the same exception thrown. I'm not sure if there's a way to workaround that, I was hitting it in the scrubber patch too. :(

Comment 8

[[e-mail]]

Attachment: id removal

FYI

Comment 9

[Neil Deakin]

Comment on attachment 351070 [details] [diff] [review]
Patch v.3


>+                    var directionChange = (isMouseOver == this.fadingIn);

This looks to be reversed. Shouldn't it be !=

Otherwise, this looks OK assuming you file followup bugs on the ids and using buttons instead of images.

Comment 10

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/25228e852b6d

Filed bug 469030 on fixing id/anonid usage.

Will morph bug 464371 to deal with the <button> issue.

Comment 11

[Justin Dolske [:Dolske]]

(In reply to comment #9)
> >+                var directionChange = (isMouseOver == this.fadingIn);
> 
> This looks to be reversed. Shouldn't it be !=

Yeah. I fixed this in the committed changeset.

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Bustage fix v.1

Bah. This broke test_videocontrols.html, because the test is clicking the play button before the first timer-driven call to fadeControls() happens... Now that we set .visibility to suppress clicks when the controls are hidden, the test's click is ignored.

This fix simply immediately enables the controls when fading in.

Comment 13

[Justin Dolske [:Dolske]]

Also pushed a test fix in http://hg.mozilla.org/mozilla-central/rev/741b47107e5e

Test was still failing after the last fix (it never received the "play" event after trying to click the play button), couldn't reproduce locally. I'm thinking it's possible that the test's synthetic mouse click was being generated before the controls received the mouseover event, and so click couldn't be targeted to the play button because it wasn't visible yet.

This changeset adds a mouseover listener to the test, which should avoid that possibility.

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Backout of the above

Backed out due to the test still failing on the Linux tinderbox. Now test 2 isn't getting the mouseover event. *sigh*

Comment 15

[Justin Dolske [:Dolske]]

Test passes on OS X, but apparently leaks?

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228974135.1228978699.17660.gz

Test passes on Windows, but apparently leaks?

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228973979.1228977921.15336.gz

Test fails on Linux. No leaks.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228974682.1228977956.15441.gz

These are all testing revision b883595ba2ce.

Comment 16

[Justin Dolske [:Dolske]]

Leaks were caused by an unrelated checkin, so that's not a problem. They persisted after this backout, and were fixed by backing out the offending patch.

I can't reproduce the Linux failure exactly, but can sometimes get a similar failure in my Linux VM when running valgrind... It seems we sometimes receive a spurious mouseout event, when causes the controls to fade out and become disabled. [We only set .visibility=hidden when the controls are fully transparent, but because the test runs quickly the controls have only faded in one or two steps, and so the mouseout fading only takes one or two steps to become transparent.]

The tinderbox failure was due to the test not receiving the initial mouseover event at all. I'm not sure how that can happen, unless there's code somewhere to consolidate and suppress pending pairs of mouseover + mouseout? I'm also not sure why the extra mouseout is even being generated. Presumably something is noticing that the mouse isn't where our synthetic mouseover implied it was?

I'm thinking the easiest solution would be to add a mozNeverFadeControls attribute, and use that in the tests. It would mean we can't really test fadein/fadeout related issues, but at least we could move forward with testing the rest of it. :(

Comment 17

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

This adds checking for a mozNoDynamicControls attribute, which is set in the test, that suppresses the fade in / fade out of the video controls. This eliminates the need (in tests) to spoof a mouseover to activate the controls, and ensures that any spurious mouseout events are harmless (ala comment 16, or if the mouse is jiggled).

This fits nicely with the code in bug 449149, which suppresses the fading for <audio> elements.

Comment 18

[Neil Deakin]

Comment on attachment 352810 [details] [diff] [review]
Patch v.4

I think a new patch is needed here to update for bug 449149 which does much of the same thing.

> +            if (video.hasAttribute("mozNoDynamicControls"))
> +                this.Utils.dynamicControls = false;

Putting this in the init method won't handle the case where the attribute is changed later.

Comment 19

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Added dynamicControls check to onMouseInOut, and made it a getter so setting and clearing the attribute will work. I think the tests will always have the attribute pre-set in the test HTML (and never clear it), but it's sensible enough to have it work anyway.

Comment 20

[Justin Dolske [:Dolske]]

(In reply to comment #18)
> (From update of attachment 352810 [details] [diff] [review])
> I think a new patch is needed here to update for bug 449149 which does much of
> the same thing.

This patch incorporates the expected videocontrols.xml changes from that bug, since we're both basically doing the same thing.

Comment 21

[Neil Deakin]

Comment on attachment 353264 [details] [diff] [review]
Patch v.5

>+                    // Allow tests to explicitly suppress the fading of controls.
>+                    if (this.video.hasAttribute("mozNoDynamicControls"))
>+                        enabled = false;

Why just 'tests'?

>+                    // If we're fading in, immediately made the controls clickable.

made -> make

Comment 22

[Justin Dolske [:Dolske]]

Attachment: Patch v.6

Patch, as landed.

I changed the test a bit, again, since v.5... The initial mouseclick was sometimes still not being received, and the test would hang. Maybe the video wasn't being loaded and setting up the anonymous content in time? Having the test start from a <video> dispatched event (loadeddata) instead of the document's onload seems to resolve the problem. *sigh*

Comment 23

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/09cfc25615d3

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 353922 [details] [diff] [review]
Patch v.6

a191=beltzner

Comment 25

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b73101a26415

I had to adjust test #1 to use loadedmetadata instead of loadeddata, because it wasn't firing. Looks like bug 462570 adds this, but hasn't landed on 1.9.1 yet. Should be fine, and eventually it should just be a plain old "load" event anyway, once our multiple-load-event bugs get fixed. :)

Comment 26

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090313 Shiretoko/3.1b4pre Ubiquity/0.1.5
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090313 Minefield/3.2a1pre.