449149 - Add support for <audio controls>

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, enhancement)

Description

[Joshua Cranmer [:jcranmer]]

The lack of <audio controls> makes it harder for me to put an <audio> in my blog without surprising people going to planet...

Comment 1

[Justin Dolske [:Dolske]]

The work in bug 448909 should be applicable to this bug, the APIs for <audio> and <video> are basically the same. I wouldn't be surprised if just hooking up the video controls XBL to <audio> Just Worked, although I'm not sure what layout size an <audio> element is supposed to be.

Comment 2

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

We really should have this. We're broken without it.

Comment 3

[Justin Dolske [:Dolske]]

Do <audio> elements default to any particular size (assuming one was not explicitly set)?

Comment 4

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

That's up to us. I plan to make them automatically size to the preferred height of the controls.

Comment 5

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

Attachment: fix

-- Let frame construction create nsVideoFrames for <audio> elements
-- But make <audio> elements without 'controls' be display:none in forms.css
(so an <audio> element without 'controls' can be made visible using author-set 'display'; 449149-2.html tests this)
-- nsVideoFrame shouldn't try to paint video unless it's a video element
-- The intrinsic size of an <audio> element is 300px wide and the preferred height of the controls
-- The controls for an <audio> element should not fade out automatically.
-- Currently videocontrols.xml is relying on auto-fade-in/out to handle dynamic changes to the 'controls' attribute. That won't work for <audio> elements. So I've removed those dynamic checks and I'm using forms.css to set visibility:hidden on the controls when the 'controls' attribute is not set.

Comment 6

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

Comment on attachment 352476 [details] [diff] [review]
fix

Need Justin's review on videocontrols.xml

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 352476 [details] [diff] [review]
fix

>+    // Note that forms.css specifies display:none for audio elements
>+    // without the "controls" attribute.

How come that's in forms.css and not html.css, by the way?  It doesn't seem form-related at all.

>+++ b/layout/generic/nsVideoFrame.cpp
nsVideoFrame::GetMinWidth(nsIRenderingContext *aRenderingContext)
> {
>   // XXX The caller doesn't account for constraints of the height,
>   // min-height, and max-height properties.

I'm not sure what this comment has to do with the intrinsic width getter functions.  I realize it was already here (and in GetPrefWidth()), but I think it should be removed.

>+++ b/layout/style/forms.css

As above, maybe all the audio/video stuff should be in html.css.

>+video:not([controls]) > xul|videocontrols, audio:not([controls]) > xul|videocontrols {
>+  visibility: hidden;
>+}
>+
>+audio:not([controls]) {
>+  display: none;
> }

Should that second rule be !important?  Or do we want to allow sites to bind XBL of their own to <audio> or something?

If it becomes !important, the audio part of the :not([controls]) rule can, imo, be removed (though with comments that it needs readding if the !important is removed).

r+sr=bzbarsky with the nits

Comment 8

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

(In reply to comment #7)
> (From update of attachment 352476 [details] [diff] [review])
> >+    // Note that forms.css specifies display:none for audio elements
> >+    // without the "controls" attribute.
> 
> How come that's in forms.css and not html.css, by the way?  It doesn't seem
> form-related at all.

Yeah, it should be html.css. Want me to move it?

> >+++ b/layout/generic/nsVideoFrame.cpp
> nsVideoFrame::GetMinWidth(nsIRenderingContext *aRenderingContext)
> > {
> >   // XXX The caller doesn't account for constraints of the height,
> >   // min-height, and max-height properties.
> 
> I'm not sure what this comment has to do with the intrinsic width getter
> functions.  I realize it was already here (and in GetPrefWidth()), but I think
> it should be removed.

OK

> >+video:not([controls]) > xul|videocontrols, audio:not([controls]) > xul|videocontrols {
> >+  visibility: hidden;
> >+}
> >+
> >+audio:not([controls]) {
> >+  display: none;
> > }
> 
> Should that second rule be !important?  Or do we want to allow sites to bind
> XBL of their own to <audio> or something?

I think it's reasonable to allow authors to style <audio>. We allow them to style <head> for example. But we could go with !important if you prefer.

> If it becomes !important, the audio part of the :not([controls]) rule can, imo,
> be removed (though with comments that it needs readding if the !important is
> removed).

Sure.

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 352476 [details] [diff] [review]
fix

Looks fine to me, though do note that your patch appears to be on top of the work in bug 461680, which got backed out.

Also, I'm not a toolkit peer, so technically Enn should rs this. (Although it's really just trivial JS changes...)

Comment 10

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

Attachment: fix v2

Updated for comments

Comment 11

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

Comment on attachment 352617 [details] [diff] [review]
fix v2

Better get Enn to sign off on the toolkit change

Comment 12

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

Attachment: fix v3

Oops, I broke things in the move to html.css

Comment 13

[Neil Deakin]

Comment on attachment 352622 [details] [diff] [review]
fix v3

>             // XXX add support for dynamically hiding controls w/o waiting for mouseout?
>-            if (!this.Utils.video.controls)
>-                return;

Why are you removing this part?

Comment 14

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

Because I'm using CSS to automatically hide the controls when the 'controls' attribute is missing. See the end of comment #5. The rule is

+video:not([controls]) > xul|videocontrols,
+audio:not([controls]) > xul|videocontrols {
+  visibility: hidden;
+}

So we don't need or want the controls binding to worry about whether the 'controls' attribute is set. Note that visiblity:hidden makes the controls impervious to events as well.

Comment 15

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

Pushed 4178997e692a to trunk.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d5385f76ce52

Comment 17

[Tony Chung [:tchung]]

dbaron, Roc, 

Are there any examples of sites that have implemented <audio controls> in their videos?   Looking to verify this bug, but can't find any content out there.   Thanks.

Comment 18

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

If you open a .wav file in the browser you'll get an element with controls.

Comment 19

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090420 Minefield/3.6a1pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090420 Shiretoko/3.5b4pre

Comment 20

[juan becerra [:juanb]]

Additional verification: While helping someone in IRC I created a sample html file with the audio tag and the controls attribute, which displays the controls when loaded. Without the controls attribute you don't see the controls.