The default bug view has changed. See this FAQ.

Add support for <audio controls>

VERIFIED FIXED in mozilla1.9.1b3

Status

()

Toolkit
Video/Audio Controls
--
enhancement
VERIFIED FIXED
9 years ago
7 years ago

People

(Reporter: jcranmer, Assigned: roc)

Tracking

({verified1.9.1})

unspecified
mozilla1.9.1b3
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
The lack of <audio controls> makes it harder for me to put an <audio> in my blog without surprising people going to planet...
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.
OS: Linux → All
Hardware: PC → All
We really should have this. We're broken without it.
Assignee: nobody → roc
Flags: blocking1.9.1+
Do <audio> elements default to any particular size (assuming one was not explicitly set)?
That's up to us. I plan to make them automatically size to the preferred height of the controls.
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.
Attachment #352476 - Flags: superreview?(bzbarsky)
Attachment #352476 - Flags: review?(bzbarsky)
Attachment #352476 - Flags: review?(dolske)
Comment on attachment 352476 [details] [diff] [review]
fix

Need Justin's review on videocontrols.xml
Whiteboard: [needs review]
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
Attachment #352476 - Flags: superreview?(bzbarsky)
Attachment #352476 - Flags: superreview+
Attachment #352476 - Flags: review?(bzbarsky)
Attachment #352476 - Flags: review+
(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 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...)
Attachment #352476 - Flags: review?(dolske) → review+
Whiteboard: [needs review] → [needs landing]
Created attachment 352617 [details] [diff] [review]
fix v2

Updated for comments
Attachment #352476 - Attachment is obsolete: true
Attachment #352617 - Flags: superreview+
Attachment #352617 - Flags: review+
Comment on attachment 352617 [details] [diff] [review]
fix v2

Better get Enn to sign off on the toolkit change
Attachment #352617 - Flags: review?(enndeakin)
Created attachment 352622 [details] [diff] [review]
fix v3

Oops, I broke things in the move to html.css
Attachment #352617 - Attachment is obsolete: true
Attachment #352622 - Flags: superreview+
Attachment #352622 - Flags: review+
Attachment #352617 - Flags: review?(enndeakin)
Attachment #352622 - Flags: review?(enndeakin)
Whiteboard: [needs landing] → [needs review]

Comment 13

8 years ago
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?
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.

Updated

8 years ago
Attachment #352622 - Flags: review?(enndeakin) → review+
Whiteboard: [needs review] → [needs landing]
Pushed 4178997e692a to trunk.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]

Updated

8 years ago
Blocks: 470629
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d5385f76ce52
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Target Milestone: --- → mozilla1.9.1b3
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.
If you open a .wav file in the browser you'll get an element with controls.

Updated

8 years ago
Blocks: 480031
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
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.
Depends on: 538431
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Target Milestone: mozilla1.9.1b3 → ---
Version: Trunk → unspecified
Target Milestone: --- → mozilla1.9.1b3
You need to log in before you can comment on or make changes to this bug.