Last Comment Bug 449149 - Add support for <audio controls>
: Add support for <audio controls>
Status: VERIFIED FIXED
: verified1.9.1
Product: Toolkit
Classification: Components
Component: Video/Audio Controls (show other bugs)
: unspecified
: All All
: -- enhancement with 1 vote (vote)
: mozilla1.9.1b3
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
:
Mentors:
Depends on: 538431
Blocks: TrackAVUI 480031
  Show dependency treegraph
 
Reported: 2008-08-04 18:57 PDT by Joshua Cranmer [:jcranmer]
Modified: 2010-01-25 17:41 PST (History)
11 users (show)
roc: blocking1.9.1+
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (15.81 KB, patch)
2008-12-10 20:46 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
bzbarsky: review+
dolske: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
fix v2 (15.25 KB, patch)
2008-12-11 14:40 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
roc: review+
roc: superreview+
Details | Diff | Splinter Review
fix v3 (15.85 KB, patch)
2008-12-11 15:13 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
roc: review+
enndeakin: review+
roc: superreview+
Details | Diff | Splinter Review

Description Joshua Cranmer [:jcranmer] 2008-08-04 18:57:49 PDT
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] 2008-09-17 15:28:07 PDT
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) 2008-12-10 17:52:27 PST
We really should have this. We're broken without it.
Comment 3 Justin Dolske [:Dolske] 2008-12-10 18:26:05 PST
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) 2008-12-10 18:34:36 PST
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) 2008-12-10 20:46:18 PST
Created attachment 352476 [details] [diff] [review]
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) 2008-12-10 20:46:50 PST
Comment on attachment 352476 [details] [diff] [review]
fix

Need Justin's review on videocontrols.xml
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2008-12-11 09:35:59 PST
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) 2008-12-11 12:48:13 PST
(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] 2008-12-11 13:30:33 PST
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) 2008-12-11 14:40:57 PST
Created attachment 352617 [details] [diff] [review]
fix v2

Updated for comments
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-11 14:49:08 PST
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) 2008-12-11 15:13:06 PST
Created attachment 352622 [details] [diff] [review]
fix v3

Oops, I broke things in the move to html.css
Comment 13 Neil Deakin (away until Oct 3) 2008-12-15 12:09:21 PST
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) 2008-12-15 12:40:01 PST
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) 2008-12-16 23:53:04 PST
Pushed 4178997e692a to trunk.
Comment 16 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-12-26 13:31:29 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d5385f76ce52
Comment 17 Tony Chung [:tchung] 2009-02-24 11:10:43 PST
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) 2009-02-24 12:21:15 PST
If you open a .wav file in the browser you'll get an element with controls.
Comment 19 Tony Chung [:tchung] 2009-04-21 01:17:23 PDT
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] 2009-04-23 16:06:51 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.