727817 - Video controls do not initialize correctly

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristian Nicolae (:xti)]

Firefox 13.0a1 (2012-02-16)
20120216031231
http://hg.mozilla.org/mozilla-central/rev/a853f4017192
Device: Motorola Droid PRO
OS: Android 2.3.3

Steps to reproduce:
1. Open Fennec
2. Go to http://people.mozilla.com/~nhirata/html_tp/big_buck_bunny_480p.webm (video will start playing automatically)
3. Wait at least 10-15s
4. Tap on screen for video controls

Expected result:
Video controls bar is triggered.

Actual result:
After step 4, nothing happens. Video controls bar might be triggered after 8-10 tries or even more.

Comment 1

[Wesley Johnston (:wesj)]

This is a regression from bug 666306 and bug 470628 (at least). These items weren't added to the touch controls there, and trying to access them in the desktop Utils.init method (at least) causes failures.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

The issue here is that we're missing some elements the desktop video controls have (namely clickToPlay I think), but we're using the same Utils.init method they use. It is failing halfway through its init method and as a result we don't attach listeners to the buttons in the controls.

This just add the clickToPlay box in a hidden box so that it is never shown. I also played with adding the other missing elements (all of the stats pieces and the new error messages), but they don't cause us any problems (yet). I wanted to keep this change small for now.

I also added some fixes to make our slider look correct. We were previously picking up some theme stuff from xul scrollbars that made the scrubber appear funny.

putting mfinkle for review, but I can run this by the toolkit guys if you think we should.

I'm still having issues actually moving the scrubber. If I tap to move it, it seeks but then jumps to the end. Still investigating that and will handle in a separate bug.

Comment 3

[Wesley Johnston (:wesj)]

Updating this summary to better reflect the fix.

Comment 4

[Wesley Johnston (:wesj)]

Oh yeah, I also added some transparency to the controls as well, because they look kinda crappy without it (and that's how I roll apparently). Performance on the test case seems crappy with and without it. Easy to remove if we don't want.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 598455 [details] [diff] [review]
Patch

mobile changes looks ok to me, but we should get a toolkit person to double check the videocontrols.xml change. I assume you have the clicKToPlay box nested in a hidden box show the clickToPlay is never shown, ever, for mobile.

Comment 7

[Wesley Johnston (:wesj)]

Yep to the always hidden. Sometime in the future, I think we actually want this box. We do this "first time shown" trick on mobile to only show the play button the first time the controls are shown. Desktop is now doing a similar thing with this clickToPlay button I think and it would be good if both of us could use the same code.

I haven't looked in to how hard that will be. Dolske might know?

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 598455 [details] [diff] [review]
Patch

Seems reasonable. I've been trying to catch things that would break mobile, but I guess this one snuck through. :(

Would be nice to have a "did the controls init" test, but that's hard with native anon content. Jared and I have talked about it, and may have some ideas though.

Should probably ask madhava what mobile wants to do wrt click-to-play... Could just show a big fat button with no controls (simple, but a bit awkward if you know you want to start from a specific point or mute it), or could just show the big fat mobile controls in lieu of the ctp button, or both, or....

Comment 9

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f0c3fefb520a

click to play only landed in 12, so i'm just marking it affected and will request approval

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 598455 [details] [diff] [review]
Patch

[Approval Request Comment]
Regression caused by (bug #): 666306
User impact if declined: Video controls don't work
Testing completed (on m-c, etc.): Landed on m-c today (2/21)
Risk to taking this patch (and alternatives if risky): Low risk. Mobile only
String changes made by this patch: None.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f0c3fefb520a

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 598455 [details] [diff] [review]
Patch

[Triage Comment]
Mobile only - approving for Aurora 12.

Comment 13

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 13.0a1 (2012-03-05)
20120305031045
http://hg.mozilla.org/mozilla-central/rev/433cfbd2a0da

--
Device: HTC Desire
OS: Android 2.2