462117 - Add specialized video controls for small-dimension media.

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Justin Dolske [:Dolske]]

Bug 448909 and followups have been adding overlayed controls to <video> elements. These are not well suited to very small videos, which could be smaller than our controls!

Ideas:
* Drop the progress bar for small videos. If it's only a handful of pixels wide, it's already useless (too small to see or seek with)
* Simplify down to a single play/pause button, centered in the element (ala YouTube)?
* Do scalable controls make sense here? (we decided they didn't for normal controls, over in 448909).
* We may want to just display nothing below a certain threshold. EG, there's no way to put useful controls into a 3x3 pixel video.

A complication for the implementation of this is that the video may be dynamically resized (eg, content script changes the height, or it's styled with width=50% and the user resizes the window). Ideally, transitions between the "big" and "tiny" controls would be handled sensibly.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 462117

In my current implementation, I'm hiding the video duration when the video width is under 200px, and hiding the scrubber as well when the video width is under 100px.

If the video height is under 30px or the video width is under 60px, then the full control bar will be removed.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

The feedback? is in reference to what controls I am hiding. Do you think that these are the proper controls to remove when under size constraints?

Comment 3

[Alfred Kayser]

Any specific testpages available, so that we can test this with custom themes?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to comment #3)
> Any specific testpages available, so that we can test this with custom
> themes?

You can test this out by going to http://msu.edu/~weinjare/462117/

This patch only applies to the browsers native media controls. I am not sure what you mean by themes. Can you please explain further?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

I have done a comparison of other browsers to see how they handle this situation.

IE 10:
When the video falls below ~150px in width, the control bar disappears and is replaced by a single play/pause toggle button that is centered vertically and horizontally on the video.

When the video falls below ~40px in height, the single play/pause toggle button disappears.

Chrome 14:
When the video falls below ~190px in width, the duration disappears. There are no other distinguishable changes in the media controls at smaller sizes.

Opera 11.50: There do not appear to be any changes to the media controls as the dimensions fall in size.

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 547585 [details] [diff] [review]
Patch for bug 462117

Review of attachment 547585 [details] [diff] [review]:
-----------------------------------------------------------------

Approach generally looks good, but a few comments before this is review-ready...

::: toolkit/content/widgets/videocontrols.xml
@@ +904,5 @@
> +                adjustControls : function adjustControls() {
> +                    let clientHeight = this.video.clientHeight;
> +                    let clientWidth = this.video.clientWidth;
> +                  
> +                    if (clientHeight < 30 || clientWidth < 60)

Hmm, I wonder if it's worth using getComputedStyle() on the relevant parts of the video controls so these are more than just magic numbers.

@@ +917,5 @@
> +                        
> +                    if (clientWidth < 100)
> +                        this.controlBar.setAttribute("extraSmall", true);
> +                    else
> +                        this.controlBar.removeAttribute("extraSmall");

I'd think it preferable to only have one attribute get set, in total. EG, for the case of clientWidth == 70 you'd be setting both the "small" and "extrasmall" attributes, which is a little confusing.

Also might help simplify to have 1 attribute ("sizemode"?) that takes various values, so that you don't have to cleanup each separately.

@@ +978,5 @@
>                      // Make the <video> element keyboard accessible.
>                      this.video.setAttribute("tabindex", 0);
>                      this.video.addEventListener("keypress", function (e) { self.keyHandler(e) }, false);
> +                    
> +                    window.addEventListener("resize", function (e) { self.adjustControls(); }, false);

terminateEventListeners() should handle removing this.

I sort of wonder if |window| is the right place to do this. Ideally, we'd have a fix for bug 227495 and be able to attach a resize handler directly to the <video>. Using the window is handy for your testcase, but I'd suspect that this would rarely get triggered in the wild... So I'm tempted to just say remove this, and only adjust the control size on init until 227495 is implemented.

Alternatively, it might work well enough to just drive this from a timer (well, better, some common media event) so that the controls adjust semidynamically. A bit hacky, though.

::: toolkit/themes/pinstripe/global/media/videocontrols.css
@@ +205,5 @@
> +.controlBar[small] .durationBox,
> +.controlBar[small] .durationLabel,
> +.controlBar[small] .positionLabel {
> +  display: none;
> +}

Might want to make sure this doesn't break (!) anything... ISTR that display:none will make XBL bindings go away, and the controls could be surprised by properties/methods no longer being available.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 462117 v2

> Hmm, I wonder if it's worth using getComputedStyle() on the relevant parts
> of the video controls so these are more than just magic numbers.

getComputedStyle will return the width as a string with 'px' appended. I've switched to using |clientWidth| instead of the magic numbers.

> I'd think it preferable to only have one attribute get set, in total. EG,
> for the case of clientWidth == 70 you'd be setting both the "small" and
> "extrasmall" attributes, which is a little confusing.
> 
> Also might help simplify to have 1 attribute ("sizemode"?) that takes
> various values, so that you don't have to cleanup each separately.

Agreed. Good idea.

> I sort of wonder if |window| is the right place to do this. Ideally, we'd
> have a fix for bug 227495 and be able to attach a resize handler directly to
> the <video>. Using the window is handy for your testcase, but I'd suspect
> that this would rarely get triggered in the wild... So I'm tempted to just
> say remove this, and only adjust the control size on init until 227495 is
> implemented.
> 
> Alternatively, it might work well enough to just drive this from a timer
> (well, better, some common media event) so that the controls adjust
> semidynamically. A bit hacky, though.
> 

I think using a timer would be too hacky. I'm fine with adding a comment in the code to add this event handler when bug 227495 gets implemented.

> ::: toolkit/themes/pinstripe/global/media/videocontrols.css
> @@ +205,5 @@
> > +.controlBar[small] .durationBox,
> > +.controlBar[small] .durationLabel,
> > +.controlBar[small] .positionLabel {
> > +  display: none;
> > +}
> 
> Might want to make sure this doesn't break (!) anything... ISTR that
> display:none will make XBL bindings go away, and the controls could be
> surprised by properties/methods no longer being available.

The bindings get removed and added back when the |display:none| property is added/removed. This doesn't cause any issues for us, but does require more work if the property gets changed very often. I have switched to using |visibility:collapsed| since this won't incur the extra overhead of the adding/removing of the bindings.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 549852 [details] [diff] [review]
Patch for bug 462117 v2

canceling review as this patch needs a little more work

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 462117 v3

I have decided to remove the case where we hide the duration, since the thumb of the scrubber overlaps with the volume scrubber. We would either need to change the way we draw the thumb, or move to a horizontal scrubber to fix this issue.

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 553181 [details] [diff] [review]
Patch for bug 462117 v3

Review of attachment 553181 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a few small changes, no rereview needed.

::: toolkit/content/widgets/videocontrols.xml
@@ +860,5 @@
>                      if (this.debug)
>                          dump("videoctl: " + msg + "\n");
>                  },
> +                
> +                adjustControls : function adjustControls() {

adjustControlSize() self-describes better.

@@ +867,5 @@
> +
> +                    // The scrubber has |flex=1|, therefore |minimumScrubberWidth|
> +                    // was generated by empirical testing.
> +                    let minimumScrubberWidth = 25;
> +                    let thresholdForHidingDurationAndScrubber = this.playButton.clientWidth +

How about minWidthAllControls as being a bit shorter?

@@ +872,5 @@
> +                                                                minimumScrubberWidth +
> +                                                                this.durationLabel.clientWidth +
> +                                                                this.muteButton.clientWidth;
> +                    let minimumHeightForControlBar = this.controlBar.clientHeight +
> +                                                     this.volumeControl.clientHeight;

Not sure if the volume control height it valid here, but we're removing the fly-out volume slider soon anyway.

@@ +878,5 @@
> +                    let sizeMode = "normal";
> +                    if (videoHeight < minimumHeightForControlBar) {
> +                        sizeMode = "hidden";
> +                    } else {
> +                        if (videoWidth < (this.playButton.clientWidth + minimumScrubberWidth + this.muteButton.clientWidth))

This would read better if you made this a var, ala minWidthAllControls... minWidthOnlyPlayPause?

Why is minimumScrubberWidth needed here?

::: toolkit/themes/pinstripe/global/media/videocontrols.css
@@ +213,5 @@
> +}
> +
> +.controlBar[sizemode="small"] .scrubberStack {
> +  visibility: hidden;
> +}

The changes to the *stripes' videocontrols.css can be moved to the global toolkit/content/widgets/videocontrols.css.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Justin Dolske [:Dolske] from comment #10)
> Comment on attachment 553181 [details] [diff] [review]
> Patch for bug 462117 v3
> 
> Review of attachment 553181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with a few small changes, no rereview needed.
> 
> @@ +878,5 @@
> > +                    let sizeMode = "normal";
> > +                    if (videoHeight < minimumHeightForControlBar) {
> > +                        sizeMode = "hidden";
> > +                    } else {
> > +                        if (videoWidth < (this.playButton.clientWidth + minimumScrubberWidth + this.muteButton.clientWidth))
> 
> This would read better if you made this a var, ala minWidthAllControls...
> minWidthOnlyPlayPause?
> 
> Why is minimumScrubberWidth needed here?
> 

This was being used because I couldn't figure out why the width of the scrubber area wasn't shrinking down to zero. I have figured this out and made the change in the patch that just got pushed (the child elements had margins/paddings set that were keeping the scrubber > 0px in width).

https://hg.mozilla.org/integration/fx-team/rev/6fd779fb622d

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Backout due to /tests/toolkit/content/tests/widgets/test_videocontrols.html timing out everywhere.

http://tinderbox.mozilla.org/showlog.cgi?log=Fx-Team/1314247981.1314248613.5913.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Fx-Team/1314238319.1314239111.9123.gz

etc etc

Comment 13

[Dão Gottwald [:dao]]

"sizemode" should be renamed to something different, since it has an already established meaning for chrome windows.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 462117 v4

I've made some more changes to this patch, enough that I feel it should be re-reviewed.

I've renamed |sizemode| to simply |size|, per Dao's comments. Please let me know if |size| is not a good name.

I've also added the call to adjustControlSize to onloadedmetadata and onMouseInOut since videos that don't explicitly declare a width/height are resized when they start receiving data from the server.

The adjustment for onMouseInOut just seemed like a nice-to-have if the video size is adjusting.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Pushed to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=576ef7803e53

Comment 16

[Mozilla RelEng Bot]

Try run for 576ef7803e53 is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=576ef7803e53
Results (out of 12 total builds):
    success: 12
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-576ef7803e53

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 462117 v5

Pulled in a couple small fixes that were present in the dependent patch.

Comment 18

[Mozilla RelEng Bot]

Try run for 73b85dea5f41 is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=73b85dea5f41
Results (out of 12 total builds):
    success: 12
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-73b85dea5f41

Comment 19

[Justin Dolske [:Dolske]]

Comment on attachment 556682 [details] [diff] [review]
Patch for bug 462117 v5

Review of attachment 556682 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/videocontrols.css
@@ +71,5 @@
> +  padding-left: 0;
> +  padding-right: 0;
> +  margin-left: 0;
> +  margin-right: 0;
> +}

It would be nice if we could just use visibility: hidden on these...

::: toolkit/content/widgets/videocontrols.xml
@@ +654,5 @@
>                      if (!this.firstFrameShown && !isMouseOver &&
>                          !(this.video.autoplay && this.video.mozAutoplayEnabled))
>                          return;
>  
> +                    this.adjustControlSize();

Hmm, we should actually only do this upon mouseout (aka !isMouseOver), otherwise it'll be a bit jarring to have the controls adjust just as you're aiming for something to manipulate.

@@ +879,5 @@
> +                    let size = "normal";
> +                    if (videoHeight < minimumHeightForControlBar) {
> +                        size = "hidden";
> +                    } else {
> +                        var minWidthOnlyPlayPause = this.playButton.clientWidth + this.muteButton.clientWidth;

Might as well group this with the other computations above. And make it "let". :)

And then I think this can just be a |if() { } else if { } else if { }| sequence.

Nit: make everything here "minFoo" instead of mixing "minFoo" and "minimumBar".

@@ +947,5 @@
>                      this.video.setAttribute("tabindex", 0);
>                      this.video.addEventListener("keypress", function (e) { self.keyHandler(e) }, false);
>  
> +                    // An event handler for |onresize| should be added when bug 227495 is fixed.
> +                    this.adjustControlSize();

Move to setupInitialState().

Comment 20

[Justin Dolske [:Dolske]]

Hmm, just tested this out on my Windows box. When in a small mode, when I mouseout from the video I see the hidden control parts briefly flash back into view as the controls fade out.

I was testing on http://www.robwinters.co.uk/lab/html5/video/drag-and-resize.htm

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Justin Dolske [:Dolske] from comment #20)
> Hmm, just tested this out on my Windows box. When in a small mode, when I
> mouseout from the video I see the hidden control parts briefly flash back
> into view as the controls fade out.
> 
> I was testing on
> http://www.robwinters.co.uk/lab/html5/video/drag-and-resize.htm

That issue was because the measurements were recomputed when the controls were hidden and thus the width was computed as 0. This has been changed to just compute the width of the controls in the initial setup.

https://hg.mozilla.org/integration/fx-team/rev/49678dad20f0

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Previous patch was backed out due to a test failure. Repushed to fx-team.

https://tbpl.mozilla.org/?tree=Fx-Team&rev=5a45436b3c18

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 462117

Patch that was pushed.

Comment 24

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/5a45436b3c18

Comment 25

[Simona Badau, Desktop QA]

When trying to verify this fix I encountered a strange behavior of the video controls.

On Firefox 9 beta 3 on videos 30px width and 60px height the video controls fade in and out on hovering the mouse. Even if the audio control is shown it is not functional.

On the latest Nightly the Full Screen button is implemented and the scrubber is still hidden when the video width exceeds 100px. 

Is this intended?

Comment 26

[Simona Badau, Desktop QA]

(In reply to Simona B [QA] from comment #25)
> When trying to verify this fix I encountered a strange behavior of the video
> controls.
> 
> On Firefox 9 beta 3 on videos 30px width and 60px height the video controls
> fade in and out on hovering the mouse. Even if the audio control is shown it
> is not functional.
> 
> On the latest Nightly the Full Screen button is implemented and the scrubber
> is still hidden when the video width exceeds 100px. 
> 
> Is this intended?

Tested on Windows 7
Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 6.1; rv:11.0a1) Gecko/20111129 Firefox/11.0a1

Comment 27

[Simona Badau, Desktop QA]

Attachment: Screenshot with the issue on Firefox 9 beta 3

Comment 28

[Simona Badau, Desktop QA]

Attachment: Screenshot on the latest Nightly

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Simona B [QA] from comment #25)
> When trying to verify this fix I encountered a strange behavior of the video
> controls.
> 
> On Firefox 9 beta 3 on videos 30px width and 60px height the video controls
> fade in and out on hovering the mouse. Even if the audio control is shown it
> is not functional.

The audio control appears outside of the video content and is not usable because of this. This bug is filed as bug 513876.

> On the latest Nightly the Full Screen button is implemented and the scrubber
> is still hidden when the video width exceeds 100px. 
> 
> Is this intended?

We require at least 25px of width to be present before showing the scrubber. Any smaller and it is not granular enough.

Comment 30

[Alfred Kayser]

Why is the scrubber not also collapsed (instead of hidden)?
Now it still uses some room between the play and volume button.

Also why is:
.controlBar[size="small"] .scrubberStack > * {
needed, as
.controlBar[size="small"] .scrubberStack {
already hides the whole thing?

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Alfred Kayser from comment #30)
> Why is the scrubber not also collapsed (instead of hidden)?

Would collapsing keep flex=1 applying? If we use |display:none| (or hidden), then the flex of the scrubber will no longer apply and the volume & fullscreen buttons will be located on the left side of the controlbar. We would like to keep the volume & fullscreen buttons docked to the right side of the controlbar.

> Now it still uses some room between the play and volume button.
> 
> Also why is:
> .controlBar[size="small"] .scrubberStack > * {
> needed, as
> .controlBar[size="small"] .scrubberStack {
> already hides the whole thing?

Please see the patch for bug 689124 which changed these lines.

Comment 32

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Windows NT 6.1; rv:11.0a2) Gecko/20120129 Firefox/11.0a2

When width is reduce under 60 px/height under 30 px, at first mouse hover over the video the controls are still displayed (but at the next ones, not). Should this be addressed in a new bug?

Thank you!

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mihaela Velimiroviciu [QA] from comment #32)
> Mozilla/5.0 (Windows NT 6.1; rv:11.0a2) Gecko/20120129 Firefox/11.0a2
> 
> When width is reduce under 60 px/height under 30 px, at first mouse hover
> over the video the controls are still displayed (but at the next ones, not).
> Should this be addressed in a new bug?

Yes, please file a new bug.

Comment 34

[Mihaela Velimiroviciu (:mihaelav)]

Logged bug #723118(In reply to Jared Wein [:jaws] from comment #33)
> (In reply to Mihaela Velimiroviciu [QA] from comment #32)
> > Mozilla/5.0 (Windows NT 6.1; rv:11.0a2) Gecko/20120129 Firefox/11.0a2
> > 
> > When width is reduce under 60 px/height under 30 px, at first mouse hover
> > over the video the controls are still displayed (but at the next ones, not).
> > Should this be addressed in a new bug?
> 
> Yes, please file a new bug.

Logged bug #723118

Comment 35

[Mihaela Velimiroviciu (:mihaelav)]

This was verified in Firefox versions newer than 9.0 (11.0a2, 12.0b1) and it works fine, with the known issues. Marking as VERIFIED.