465839 - Controls for <video> content are missing

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Viktor Gal]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.4) Gecko/2008111318 Ubuntu/8.10 (intrepid) Firefox/3.0.4
Build Identifier: 1.0a2

When browsing a page with <video> content the video controls are missing.

Reproducible: Always

Steps to Reproduce:
1. go to the given site - or any site with <video> content
2. try to get the video playing, by clicking on the video content

Actual Results:  
It is almost impossible to start playing the video as the video controls does not appear. The on-mouse-over event not handle, which makes sense as Fennec is targeted for touchscreen equiped mobile devices, so a different kind of video control is required.

Expected Results:  
Get the video controls in front somehow; e.g. taping on the video.

Comment 1

[Justin Dolske [:Dolske]]

Wiking noted on IRC that fade-in-on-hover video controls don't work so well on mobile, where you finger doesn't really hover, and made the observation that we could just to (1) tap to play/pause, (2) swipe left/right to change playback position, and (3) swipe up/down to adjust volume. I suppose we could also use swipes to reveal the controls from the <video> frame edge, similar to how Fennec controls reveal from the screen edge. There's also the cheap-and-dirty route of just shrinking the controls to a little widget, but that seems crude in comparison.

Other ideas/comments?

Comment 2

[Jennifer Morrow [:Boriss] (UX)]

A few clarifications:

(In reply to comment #1)
> Wiking noted on IRC that fade-in-on-hover video controls don't work so well on
> mobile, where you finger doesn't really hover, and made the observation that we
> could just to (1) tap to play/pause, 

Is there currently any visual indication that an object is video, such as a small icon on top of it?  

I suppose we could also use
> swipes to reveal the controls from the <video> frame edge, similar to how
> Fennec controls reveal from the screen edge. 

Have the controls available for video on Fennec already been determined?  And if so, what are they - just the same as for Firefox, or for instance, is full-screen working here?

There's also the cheap-and-dirty
> route of just shrinking the controls to a little widget, but that seems crude
> in comparison.

Eh we can do better.  Mobile's all about blunt force trauma UI, it'll be fun. \o/

Comment 3

[Viktor Gal]

(In reply to comment #2)
> (In reply to comment #1)
> > Wiking noted on IRC that fade-in-on-hover video controls don't work so well on
> > mobile, where you finger doesn't really hover, and made the observation that we
> > could just to (1) tap to play/pause, 
> 
> Is there currently any visual indication that an object is video, such as a
> small icon on top of it?  

no, there's none. currently in FF3.1 beta2 it pops up when the mouse cursor gets over the video content. but in case of fennec this is not a working scenario...

> > I suppose we could also use
> > swipes to reveal the controls from the <video> frame edge, similar to how
> > Fennec controls reveal from the screen edge. 
> 
> Have the controls available for video on Fennec already been determined?  And
> if so, what are they - just the same as for Firefox, or for instance, is
> full-screen working here?

AFAIK the same as for Firefox, but i'm not an official developer, so maybe somebody else can answer on this one :p

Comment 5

[Justin Dolske [:Dolske]]

Bug 469211 will help a little bit, and as a short-term fix we could use some of the code already in videocontrols.xml to force the controls to always be visible on mobile.

Comment 6

[Biju]

(In reply to comment #5)
> force the controls to always be visible on mobile.

That will hide content, many times the hidden contents can be even subtitles.
So instead make control appear for 6 seconds after first tap, 
then start a slow fadeout which completes in next 4 seconds.
That will a total of 10 seconds for user to do some action.

Also please consider bug 488003

Comment 7

[Maciej Jaros]

Shouldn't this have status "won't fix" or "fixed"? I'm working on the latest 3.5 preview and cotrols are visible when the "controls" attribute is present and aren't otherwise. If it wouldn't work like that it would be against the specs!

Note that the content provider should be the one to decide does it work. He can provide his own controls and certainly would want browser controls to be visible. After all adding play button is as simple as this:
<a onclick="document.getElementsByTagName('video')[0].play()">&#10704;</a>
Also an end user can control video from the address bar with something like this:
javascript:document.getElementsByTagName('video')[0].play()
javascript:document.getElementsByTagName('video')[0].pause()
...

Comment 8

[Maciej Jaros]

BTW even adding controls by end-user works:
javascript:var v=document.getElementsByTagName('video')[0];var a=document.createAttribute('controls');var xxx=a.nodeValue='controls';var xxx=v.setAttributeNode(a);

(and so extensions to add browser controls for videos is very easy to do)

Comment 9

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

(In reply to comment #7)
> Shouldn't this have status "won't fix" or "fixed"? I'm working on the latest
> 3.5 preview and cotrols are visible when the "controls" attribute is present
> and aren't otherwise. If it wouldn't work like that it would be against the
> specs!

This bug is about the controls not displaying in Fennec (mobile Firefox), not the desktop version. Touchscreen devices don't have the concept of "mouseover" or "hover".

Comment 11

[Wesley Johnston (:wesj)]

Attachment: WIP

This is a WIP patch. The controls will sit around for 5 seconds after the last interaction. Then they'll disappear until the video is 'clicked' again. There are some style changes too, but they should probably be ignored for now.

I've been trying to figure out how to attach this binding. If I store the binding in mobile-browser and apply it, it seems to be forced into a chrome process. Whenever you click, you will see an error when we try to call hasAttribute on the controlBox ("Error: Permission denied to access property 'hasAttribute' from a non-chrome context"). If I move the binding to toolkit/content/widgets/ and then apply it, it works fine. Not sure why that is.

I could just clean the whole thing up, put it in tooklkit, and use :-moz-system-metric(touch-enabled) to determine what to use in layout/styles/html.css. Or I can try and track down what is happening with the bindings. Or I could try to adapt the normal binding to handle both cases... just wanted some feedback before I put more time in. Feedback on the "show on click, hide after a timeout" approach would be nice too.

Comment 12

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

Comment on attachment 471651 [details] [diff] [review]
WIP

Some feedback:
* There is a lot of code duplication, see the Util field for example. Your binding extends the videocontrols.xml#videoControls binding. Why do you need to copy so much source code?
* Your binding says it implements nsISecurityCheckedComponent, but I did not see the impl. See http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml?force=1#229
* Moving this to toolkit is a good idea, but should be able to test it in mobile first.

Before moving to toolkit, we would still want to reduce the code duplication and fixup the nsISecurityCheckedComponent stuff.

The UX behavior it self sounds good too. 

fb+ for the general direction, but let's cleanup the code and test in mobile.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Better WIP patch

A better patch. Works, except that video in Fennec, even on desktop, has regressed with Layers.

Two main problems left to fix:
  1.) Still haven't figured this out, but trying to talk to any of the binding content throws a security exception. This goes away if I move the binding to toolkit.
  2.) .timeThumb has a gray background. Normally it would have a bubble, which I've tried to hide, but no amount of !importants will make it won't go away. With the exact same style in Stylish though, it looks fine on Firefox.

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Mostly done

Background problems came from a collision with thumb styling. Also, transparent background (currently) cause all sorts of problems, so I killed them. Security problems are not fixed by implementing nsISecurityCheckedComponent, but the workaround works until this is moved into toolkit for reals.

Comment 15

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

Justin - Any thoughts about the security problems? See comment 11 about the "Error: Permission denied to access property 'hasAttribute' from a non-chrome context"

It seems to be"fixed" when we move this toolkit/content/widgets/. Should we just try for that?

Comment 16

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

Wes - on IRC Justin pointed out that we might need to add contentaccessible=yes to our Jar.mn file. That is what toolkit does and might have an affect on the permission problem.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Checking on the security trick now. A pic of the current appearance, for ui/ux review.

Comment 18

[Madhava Enros [:madhava]]

This is the layout I'd been thinking.
http://www.flickr.com/photos/madhava_work/4352145250/in/photostream/

I'll pass this by sean for theme advice / icons.

Comment 19

[Wesley Johnston (:wesj)]

And madhava's old mockups, for referrence:

http://www.flickr.com/photos/madhava_work/4352145250/

Comment 20

[Wesley Johnston (:wesj)]

Attachment: Move to MC

This moves this to mc. Concluded that we are seeing a result from:

http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/XPCSystemOnlyWrapper.cpp#214

This implements Madhava's mockups (for the most part at least). Initially we only show the play button, and initially any clicks on the video will start playback (assuming its not autoplay=true). Afterwards, clicks will toggle the controls visibility. There is a timer that runs to hide controls, which resets every time you play/pause or seek the video. I moved the positioning down because I didn't like the popup covering so much of the video (although it covers most of it now anyway).

Needs a little fix for mobile browser, coming.

Comment 21

[Wesley Johnston (:wesj)]

Attachment: MB changes

Mobile browser stuff. I put review on both of these for mfinkle, but I have a feeling dolske or someone is the right choice. Just thought you might have a set of first-pass comments.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 23

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

Comment on attachment 483024 [details] [diff] [review]
Move to MC

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

>+            get visible() {
>+              return !this.Utils.controlBar.hasAttribute('fadeout') || !this.Utils.controlBar.hasAttribute("hidden");

Use " around "fadeout"

>+            video: null,

I think this is in Utils too, so why not just use it from there all the time?

>+            controlsTimer : null,
>+            controlsTimeout : 5000,

We might want a way to control this timeout. Maybe a preference? "layout.video.controls.timeout" ?

>+            toggleControls: function() {
>+              this.log("Toggle control visibility");

You might want to remove some of these entry log messages too (not really for debug)

>+            showControls : function() {

>+                if(!this.visible)

if (

>+            hideControls : function() {

>+              if(this.firstShow)

Same

>+            showPosition : function(aTime) {
>+                this.log("Time: " + aTime);
>+                if (isNaN(aTime))
>+                    return;
>+                var hours = Math.floor(aTime / 3600000);
>+                var mins  = Math.floor(aTime % 3600000 / 60000);
>+                var secs  = Math.floor(aTime % 60000 / 1000);
>+                var timeString;
>+                if (secs < 10)
>+                    secs = "0" + secs;
>+                if (hours) {
>+                    if (mins < 10)
>+                        mins = "0" + mins;
>+                    timeString = hours + ":" + mins + ":" + secs;
>+                } else {
>+                    timeString = mins + ":" + secs;
>+                }
>+                this.positionLabel.setAttribute("value", timeString);

You switched to 4 char indents here

>+            },
>+            handleEvent : function (aEvent) {

Add a blank line before handleEvent

>+              if (this.videoEvents.indexOf(aEvent.type) > -1) {
>+                this.showControls();
>+              }

No {} are needed here

>+            init : function (binding) {
>+
>+              if(!this.video.autoplay)

if (

>+        <handler event="mouseup">
>+            if(event.originalTarget.nodeName == "vbox") {
>+              if (this.TouchUtils.firstShow)
>+                this.Utils.video.play();

See you use Utils.video, not TouchUtils.video


>diff --git a/toolkit/themes/pinstripe/global/media/touchcontrols.css b/toolkit/themes/pinstripe/global/media/touchcontrols.css

>+.controlBar {
>+  font-size: 7mozmm;

I don't think we need to use mozmm here. Just pick a decent fontsize for desktop. I think we will ship a mobile friendly CSS file in mobile-browser anyway, complete with new images. Those don't need to live in toolkit.

>+}
>\ No newline at end of file

Add the newline

>diff --git a/toolkit/themes/winstripe/global/media/touchcontrols.css b/toolkit/themes/winstripe/global/media/touchcontrols.css

Same as above

r+ with nits

Let Justin review too before making changes based solely on my comments. He might have a different opinion.

Comment 24

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

Comment on attachment 483025 [details] [diff] [review]
MB changes

Looks good. We can open a new bug to style the controls with Madhava and Martell

Comment 25

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Tester's note :
short tap to play (if paused)
short tap to show controls (if playing)

bug 590719 is for long tap.

Comment 26

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=13674 test case created but disabled.  Waiting for bug to land.

Comment 27

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note to tester: test bug 590715 when this bug lands.

Comment 28

[Wesley Johnston (:wesj)]

Attachment: Revised fixes

Fixes for mc. This removes all of the themeing junk I left around. We can just override the theme from Fennec, and Firefox can do their own thing if they ever use this for touch enabled PCs.

We can't use prefs for the timeouts here because we don't have access to Components.classes[]. Other than that, I think most of the nits are addressed.

Comment 29

[Wesley Johnston (:wesj)]

Attachment: Mobile browser

Comment 30

[Madhava Enros [:madhava]]

design: http://grab.by/6WJH 
progress bar is white + #5e6166, bg box is (0,0,0,0.5)

glyphs are currently in https://bugzilla.mozilla.org/show_bug.cgi?id=575403#c65

Comment 31

[Sean Martell]

Attachment: video glyphs (mute, unmute, play, pause)

Comment 32

[Justin Dolske [:Dolske]]

(In reply to comment #30)
> design: http://grab.by/6WJH 

The blur is probably going to be prohibitively expensive; I'm doing a similar effect over in the tab-modal prompts bug (bug 59314), and it's kind of chunky even on desktop (haven't tried with video, just animated images).

Also, no volume controls, or affordance for showing that the progress is actually draggable / clickable?

Comment 33

[Justin Dolske [:Dolske]]

Comment on attachment 484165 [details] [diff] [review]
Revised fixes

A few broad comments:

>+++ b/toolkit/content/widgets/videocontrols.xml
...
>+    <xbl:content xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+                 class="mediaControlsFrame">

This is basically duplicating the existing controls, with the small addition of <label class="positionLabel"/>. I think we should just go ahead and pull that label into the main videocontrols (with display:none, and you'll unhide it for mobile).

Not sure exactly how XBL wants <content> expressed when you're extending a binding but not the content the base provides -- can it go away entirely? Or just <content><children/></content>?


Also, similar concern for the CSS. Mark says the mobile theme extends the normal *stripe themes, so it would be good to look at pulling in the existing CSS and then just adding the CSS needed to change the appearance to match what you desire. Not sure of the best method, #ifdef magic, just blindly load another app-supplied CSS file from <resources>, media selectors, etc.

We've talked about wanting similar-looking UI for full-screen mode on desktop, so if we need to make changes to the base <content> to make it more easily styleable that would make sense and be complimentary. OTOH, if you guys think that inherit+extend just creates a different CSS mess (ie, cost of overriding inherited style >= cost of just duplicating it), then we shouldn't do it. I think it'll be a win, but not sure.

>+            toggleControls: function() {
...
>+            showControls : function() {
...
>+            hideControls : function() {

Sigh. I kinda want to find a way to roll some of this into the main binding, but it seems like it would just be awkward. I think you want a bit more of the logic from onMouseInOut(), though.


>+++ b/toolkit/themes/pinstripe/global/media/videocontrols.css
>+++ b/toolkit/themes/winstripe/global/media/videocontrols.css
...
>+.positionLabel {
>+  display: none;
>+}

I don't think these are needed, as-is we're not using your extended binding, so there would never be a .positionLabel anyway. Probably a moot point with the changes above.

Comment 34

[Wesley Johnston (:wesj)]

Attachment: Mozilla Central changes v2

Fixes... for the most part. Moves content stuff into the main binding (and added a few classes up there to help with theming). In one of the older patches here, I did most of the theming by overriding default styles, but I kept it as an override here. I don't think there is much difference in the size of the patch either way.

mfinkle: do you have an opinion about that (overriding the Firefox stylesheet, vs. just mucking around with their styles)?

Biggest other change here is moving to firing mouseover and mouseout events to show and hide the controls rather than doing it on my own, that way we can pick up all of the checks from onMouseInOut.

Left that .positionLabel css in, as now its included in the default binding.

Style changes and a screenshot coming up.

Comment 35

[Wesley Johnston (:wesj)]

Attachment: Mobile Browser

Fixes for styling, updated to use new glyphs, and match (closer) to mockup. I'm using a solid background because from what Stuart has said this week, and from personal experience, it requires a significant amount of power to have transparent controls. I would guess that will get better when OpenGL accelerated stuff lands, but figure we can change it then.

Also, I left the numbers below the progress bar. Moving them up using -moz-box-ordinal-group isn't cooperating with me. I could try to use negative margins if there is a strong desire to have them that way.

Comment 36

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 37

[Wesley Johnston (:wesj)]

Attachment: Mozilla Central changes v3

Oops. Forgot that seeking can trigger a whole lot more than just seek events. This moves those to be based on clicks on the buttons/scale elemtn rather than media events. It also does a better job managing the hide timer.

Comment 38

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

Comment on attachment 484809 [details] [diff] [review]
Mobile Browser

This looks fine to me. Overriding the controls CSS might make it easier in the short term to let Madhava tweak the look and feel too. We don't seem to be duplicating much of the CSS anyway.

The CSS transitions might not work out to well on device. We'll have to test it out.

We still need Madhava to give us an OK on the initial look and feel too.

Comment 39

[Madhava Enros [:madhava]]

Some nits:
http://www.flickr.com/photos/madhava_work/5104729261/sizes/l/

sean adding image and hex colors.

Comment 40

[Sean Martell]

Attachment: scrubber control

Comment 41

[Sean Martell]

for a solid color bg, lets go with #34353a. 

#34353a is the new #000000.

Comment 42

[Wesley Johnston (:wesj)]

Attachment: Mozilla Central changes v4

In order to match the mockups, I had to duplicate the XUl and move the mute buttons position in the box. I left in all of the other elements (i.e. the volume slider), so that the Utils methods that rely on them won't break. Not exactly my dream solution, but CSS to fix this is going to be a pain.

I'm also leaving the positionLabel in both bindings, because I like the idea of updating it at the same time we update the progressbar/scrubber.

Comment 43

[Wesley Johnston (:wesj)]

Attachment: Style changes

Adds additional glyphs, and changes some styles. Modified the unmute image slightly so that the two speakers would overlap.

mfinkle, I'm carrying over the r+, but feel free to look it over.

Comment 44

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 45

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Testers note: please verify theme change : Bug 575403 
when regressing this bug.

Comment 46

[Wesley Johnston (:wesj)]

Attachment: Tweaks

Made the progressbar ends align with numbers and with mute button. Moved text slightly closer to progress bars (but still below scrubber).

Comment 47

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 48

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

Comment on attachment 485354 [details] [diff] [review]
Tweaks

Yeah still looks OK

Comment 49

[Justin Dolske [:Dolske]]

Comment on attachment 485333 [details] [diff] [review]
Mozilla Central changes v4

>+++ b/toolkit/content/widgets/videocontrols.xml
...
>-                    var hours = Math.floor(duration / 3600000);
>+                    let hours = Math.floor(aTime / 3600000);

Hmm, I had been wondering if |let| would work here, since it's a weird case of XBL in content. Guess it does?


>+  <binding id="touchControls" extends="chrome://global/content/bindings/videocontrols.xml#videoControls">
>+
>+    <xbl:content xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" class="mediaControlsFrame">

Sigh. I really with it was easier to position things in CSS/XUL so you didn't have to duplicate this chunk, and could just control it with CSS. (Might still be possible, but a PITA).

>+              this.Utils.scrubber.addEventListener("click", function() { self.showControls(); }, false);
>+              this.Utils.muteButton.addEventListener("click", function() { self.showControls(); }, false);

Hmm, not sure how this is working. When the controls fade out, they normal onTransitionEnd listener will set |hidden=true|, and so the elements shouldn't respond to clicks when hidden. Seems like this should just be calling delayHideControls(), and your <handler> takes care of showing the controls when things are hidden. I suspect the way this is written right now is sending (fake) mouseover events while the controls are visible -- I think we'll handle that ok, but could cause problems.

r+ with fixing / explaining that. :)

Comment 50

[Wesley Johnston (:wesj)]

Attachment: Final Patch

Just to be clear, I'm calling showControls there in order to reset any timers that had been created, not to actually show the controls. But looking at how I've refactored things, a call to delayHideControls should do the same thing, and make mores sense.

I also modified one line here to check for dynamic controls before setting this "firstShown" attribute, so that we get a scrubber on audio elements.

Comment 51

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

pushed m-c:
http://hg.mozilla.org/mozilla-central/rev/d971ad2efec6

pushed m-b:
http://hg.mozilla.org/mobile-browser/rev/cd005f87a866

Comment 52

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101026 Namoroka/4.0b8pre Fennec/4.0b2pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101026 Namoroka/4.0b8pre Fennec/4.0b2pre

Comment 53

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

It looks like the m-c patch caused a Ts regression in Fennec, about 200ms.