469211 - Video controls should not require initial mouse over to appear

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect, P2)

Description

[cajbir (:cajbir)]

Currently the controls that appear for video require the user to 'mouse over' the video playback area for the controls to appear. Commonly players like YouTube have a big 'Play' icon when the video is not playing, always appearing in the center of the video so users just have to click to play.

A number of users have commented that they didn't know it was a video, or that it was not ready to play, due to the absence of a play icon when the video is stopped. They did not know they had to mouse over. See comments here (requires JavaScript enabled):

http://tinyvid.tv/show/3d198wqepg78m

Actions to reproduce:

1) Load page in URL
2) Video in page has no controls displayed

Expected result:

1) Load page in URL
2) Video contains controls or a play button

Comment 1

[Justin Dolske [:Dolske]]

This came up on IRC earlier today too. When the page loads, we should at least show the controls until the user (or page JS) takes some action (assuming it's not an autoplay video). But I suppose "show controls when not playing" is a more general solution to the problem.

Comment 2

[Christopher Blizzard (:blizzard)]

Yeah, this happens to me all the time with the auto-controls.  "Did it finish loading?  Is it a bug?  Why is it black?"  Showing the controls when you're able to play would be nice.

Comment 3

[Christopher Blizzard (:blizzard)]

Also I can't believe you just rickrolled me with the video tag.

Jerk.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

With this patch, controls start off visible and won't go away until you've moused over them. For videos that autoplay, however, we'll leave the controls hidden by default (as they are currently). I think having them visible by default would be annoying -- the user would have to take some action to get them out of the way.

I considered having the controls stay visible when the video is paused, but didn't do it... That would mean you couldn't pause the video and read subtitles (they'd be obscured by the controls), and it introduces some complicated edge cases (what if page script calls video.play() while your mouse is over the video?).

I'm still open to the idea of doing that -- having an visible "play" target seems compelling. But this would be a safe step to take in the interm, and lets us see if that further optimization is needed or not.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the controls should also stay visible until after the loadeddata event has fired. Before that event, there is no video data to display so the controls can't be obscuring anything.

Comment 6

[Neil Deakin]

Comment on attachment 354367 [details] [diff] [review]
Patch v.1

>-                    if (!this.video.controls && (isMouseOver || !this.controlsVisible))

If video.controls is false, don't we still want to keep them hidden?

>+                    // Nothing to do if we've already faded to this state.
>+                    var directionChange = (this.fadingIn != isMouseOver);
>+                    if (!directionChange)
>                         return;

Can you explain why we don't need to handle this any more?

Also some of the grammar of the comments doesn't make sense. There seems to be missing words.

Comment 7

[Christopher Blizzard (:blizzard)]

What roc said about leaving them visible - yes, agreed.  I really wish that we had some kind of indication that buffering was underway as well.  It's always hard to tell if there's something going on when you press play and nothing changes.

Comment 8

[Justin Dolske [:Dolske]]

(In reply to comment #6)

> If video.controls is false, don't we still want to keep them hidden?

The css in layout/style/html.css takes care of this now. I suppose it might seem a little weird to separate the internal state from what's actually displayed, but I think it simplifies things and should make the controls robust against being enabled/disabled at weird times and getting into an odd state.

> >+                    // Nothing to do if we've already faded to this state.
> >+                    var directionChange = (this.fadingIn != isMouseOver);
> >+                    if (!directionChange)
> >                         return;
> 
> Can you explain why we don't need to handle this any more?

We do, this code just moved up a few lines.

The return (for no direction change) was added so that the first mouseover doesn't trigger an animation. It shouldn't be hit at other times (that would imply were not getting alternating mouseover/mouseout events), but even if it did it should be ok as we'd already be animating towards (or be at) the desired state.  
 
> Also some of the grammar of the comments doesn't make sense. There seems to be
> missing words.

I tend to drop articles (a/an/the), I can try to be more verbose. :)

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 354367 [details] [diff] [review]
Patch v.1

I think I agree with roc's comment 5, will post an updated patch.

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Clarifies some comments, and suppresses fade-out until a |loadeddata| event has been received (except for autoplay videos; since their controls are not initially shown, a mouseover would cause them to get stuck showing until loadeddata had fired).

Comment 11

[Alfred Kayser]

Can the 'visibility:hidden;opacity:0.0' items be moved from the theme part to toolkit or such, so that this doesn't depend on themers to support this?

Comment 12

[Neil Deakin]

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

>+            // (Note: the |controls| attribute already handled via layout/style/html.css)

attribute *is* already handled

>+    /* Bug 449358: hide by default, so we don't show broken controls when script is disabled */
>+    visibility: hidden;
>+    opacity: 0.0;

These should go in html.css if the binding code relies on them being set to these values to start with, as the previous comment suggested.

Comment 13

[Jennifer Morrow [:Boriss] (UX)]

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

looks good

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Adding the rule to html.css didn't seem to work -- I guess it doesn't apply to the anonymous content? In any case, it also seemed a little odd to have details of the binding living over in layout, so I instead created a CSS file that lives next to the binding. That works, and keeps the code closer together.

Comment 15

[Justin Dolske [:Dolske]]

Enn - ping?

Comment 16

[Justin Dolske [:Dolske]]

Using videocontrols.css (instead of html.css) also comes in handy for bug 462113, where we want to add a -moz-binding rule.

Comment 17

[Alfred Kayser]

Functional things like bindings and initial visibility should not be set from .css files from /skin/ but from /content/.

Comment 18

[Justin Dolske [:Dolske]]

Sorry, wasn't clear. In comment 16 I was referring to the new chrome://global/content/bindings/videocontrols.css added by this patch, not chrome://global/skin/media/videocontrols.css

Comment 19

[Alfred Kayser]

Ok, thanks!

Comment 20

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/e611a04fc2b6

Comment 21

[Justin Dolske [:Dolske]]

Pushed to branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2fc471fae7a2

Comment 22

[Marcia Knous [:marcia]]

I just checked this on both the branch and trunk, and it looks good to me as far as the controls showing when the page first loads (I am using http://tinyvid.tv/show/3d198wqepg78m as the test case). But if I pause the video, if I move my mouse away the controls are no longer visible until I mouse over again. Just trying to clarify if that is expected.

Comment 23

[Alfred Kayser]

At least the initial display is there.
But indeed the controls should remain there until the video has really started playing. So, no hiding of controls on mouse-out as long as the video has not started playing... Best to create a new bug for this.

Comment 24

[Marcia Knous [:marcia]]

Bug for issue described in Comment 22 is Bug 474833

Comment 25

[IU]

Should this not have also fixed the issue for audio?

Go to http://urn1350.net/listenlive and click the "Ogg Vorbis" option at the bottom.  The window becomes blank and the audio control is visible only when mousing over the top-left area of the content area.