Muted attribute not respected

RESOLVED FIXED in mozilla11

Status

()

Core
Audio/Video
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: cpearce, Assigned: drexler)

Tracking

Trunk
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=jdm] [lang=c++], URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
A <video volume="0.1" muted> element isn't played at volume=0.1 and it isn't muted. It should be. See test case at URL, it should be muted, and the volume should be 0.1

Maybe we need to add another case to nsHTMLMediaElement::SetAttr()?
(Reporter)

Comment 1

6 years ago
W3C's bikeshed on this issue:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=10419

Comment 2

6 years ago
Having looked at the code, I suspect I could guide someone through fixing this bug. Chris, please feel free to take my place.
Whiteboard: [mentor=jdm]
(Reporter)

Comment 3

6 years ago
Sounds great Josh.

Updated

6 years ago
Whiteboard: [mentor=jdm] → [mentor=jdm] [lang=c++]
(Assignee)

Comment 4

6 years ago
Created attachment 577881 [details] [diff] [review]
Patch

Added additional check for muted attribute
Attachment #577881 - Flags: review?(josh)

Comment 5

6 years ago
Comment on attachment 577881 [details] [diff] [review]
Patch

Great work! There are a couple more changes required, though:

1) you'll also want to add a check in http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXHTMLContentSerializer.cpp#746 for the new muted atom

2) we'll need a new atom for volume as well, and a similar change in SetAttr for it
Attachment #577881 - Flags: review?(josh)
(Assignee)

Comment 6

6 years ago

(In reply to Josh Matthews [:jdm] from comment #5)
> Comment on attachment 577881 [details] [diff] [review] [diff] [details] [review]
> Patch
> 
> Great work! There are a couple more changes required, though:
> 
> 1) you'll also want to add a check in
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsXHTMLContentSerializer.cpp#746 for the new muted atom
> 
> 2) we'll need a new atom for volume as well, and a similar change in SetAttr
> for it

Okay, I missed the nsXHTMLContentSerializer. However, with regards to a new atom for volume, I decided against that since it was superfluous to solving the problem at hand since the ctor of nsHTMLMediaElement had mVolume already preset to 1.0. I will make the necessary adjustments then.
I don't think volume is a content attribute in the spec (unless I am missing something), so we shouldn't treat it like one unless the spec is changed.

Also, it looks like muted may need to be added to parser/html.  I'm not sure how to do that, but if you file a bug like bug 704034 and Henri will sort it out.

Comment 8

6 years ago
Sorry to mislead you Andrew. If you make the nsXHTMLContentSerializer change, and we file a bug about getting muted in to the parser, then this patch should be ready for review.
Summary: Value of volume and media attributes not respected → Muted attribute not respected
(Assignee)

Comment 9

6 years ago
Created attachment 578087 [details] [diff] [review]
Patch v2

No problems Josh. It actually answered a question i had about Google's chrome ignoring the volume attribute even if it was set to "0.0" in the video element. Overall, it's been a learning experience :-)
Attachment #577881 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #578087 - Flags: feedback?(josh)
Comment on attachment 578087 [details] [diff] [review]
Patch v2

I like it, but my blessing is not enough.
Attachment #578087 - Flags: review?(kinetik)
Attachment #578087 - Flags: feedback?(josh)
Attachment #578087 - Flags: feedback+
Comment on attachment 578087 [details] [diff] [review]
Patch v2

This looks good, thanks!

As well as the parser bug, please file a bug about adding support for the defaultMuted IDL attribute.  That bug would make a good follow up bug to work on, if you're interested.
Attachment #578087 - Flags: review?(kinetik) → review+
Assignee: nobody → andrew.quartey
Status: NEW → ASSIGNED
(Assignee)

Comment 12

6 years ago
Filed Bug 706731 and Bug 706735 to address defaultMuted and muted in parser/html respectively.
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Do we not need to change UnsetAttr?  As in, should/does removing the attribute unmute?
(Assignee)

Updated

6 years ago
Blocks: 706731
(Assignee)

Updated

6 years ago
No longer blocks: 706731
http://hg.mozilla.org/integration/mozilla-inbound/rev/47eba8a9a3b8
Keywords: checkin-needed
Target Milestone: --- → mozilla11
Erm.  I should have been clearer in comment 13.  I think this patch is wrong, with my dom peer hat on, and should not have landed....
(Assignee)

Comment 16

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #13)
> Do we not need to change UnsetAttr?  As in, should/does removing the
> attribute unmute?

I don't think a change is required to UnsetAttr since in the case of unmuting a prior muted video, SetMuted is called to handle it.
Called by what?

Again, it looks like this patch causes hysteresis: setting then removing the "muted" attribute changes the element's state, no?  If so, that's buggy.  If not, what am I missing?
Chrome's behaviour here is that removing the muted content attribute results in defaultMuted returning false, but muted remaining true (and as indicated by muted, the media is not unmuted).  The spec is a little unclear to me, but that behaviour seems to be correct.  Let's have this discussion in bug 706731 please, since it interacts with defaultMuted and this patch just implements the basic muted content attribute.
(Assignee)

Comment 19

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #17)
> Called by what?
>
nsHTMLVideoElement::SetMuted() That's as far i could decipher from the call stack shown by VS.
> The spec is a little unclear to me, but that behaviour seems to be correct.

That sounds like a spec bug, if so.  But ok.  Let's take this to bug 706731.

> nsHTMLVideoElement::SetMuted()

Irrelevant here.
So I just looked at the spec.  Per spec, as far as I can tell the "muted" attribute should only have an effect for parser-created elements.  That is, dynamically adding or removing it to an existing element should never have an effect.  Is that how it works in Chrome?
Oh, and I don't think the above interacts with defaultMuted at all.  defaultMuted always just reflects the content attribute, and the spec explicitly says that dynamic changes to defaultMuted don't change muted state.
(We should add tests for both the muted attribute having no effect when set/removed dynamically and for having an effect when set via the parser, btw.)
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/47eba8a9a3b8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(In reply to Boris Zbarsky (:bz) from comment #21)
> So I just looked at the spec.  Per spec, as far as I can tell the "muted"
> attribute should only have an effect for parser-created elements.  That is,
> dynamically adding or removing it to an existing element should never have
> an effect.  Is that how it works in Chrome?

My understanding is that if the element is created via <video muted>, then muted and defaultMuted are true.  If the muted attribute is not present, then muted and defaultMuted are false.  Setting defaultMuted has no effect (and I don't understand why it's not readonly in the spec).  setAttribute("muted", true) or removeAttribute("muted") is reflected by defaultMuted and don't change the state of muted.  So:

|                               | hasAttribute | muted | defaultMuted |         
|-------------------------------+--------------+-------+--------------|         
| <video id=a>                  | false        | false | false        |         
| a.setAttribute("muted", true) | true         | false | true         |
| a.defaultMuted = false        | true         | false | true         |
| <video id=b muted>            | true         | true  | true         |         
| b.removeAttribute("muted")    | false        | true  | false        |
| b.defaultMuted = true         | false        | true  | false        |

This matches Chrome's behaviour, except that setting defaultMuted changes the result of hasAttribute, which is incorrect if I understand the spec correctly.

So I think the patch is fine.  The defaultMuted behaviour will be implemented in bug 706731.
(In reply to Matthew Gregan [:kinetik] from comment #25)
> This matches Chrome's behaviour, except that setting defaultMuted changes
> the result of hasAttribute, which is incorrect if I understand the spec
> correctly.

Er, I misread the spec here and Chrome's behaviour is correct.  So, amending the table above:

| <video id=a>                  | false        | false | false        |         
| a.defaultMuted = true         | true         | false | true         |
| <video id=b muted>            | true         | true  | true         |         
| b.defaultMuted = false        | false        | true  | false        |
Ah, so I guess the obvious thing I missed here is that nsHTMLMediaElement::SetAttr may be called at times other than parsing, and it should not effect the muted status in those situations.  D'oh.

Is there a way to detect SetAttr is being called at parse time, or am I on the wrong path here?  Or should this just use HasAttr at initialization time to get the initial state of muted?

(And sorry for steering you on the wrong path, Andrew.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 706731
Agreed on the table in comment 26 and the analysis in comment 27.  I think we're on the same page now.

The only way to detect whether SetAttr is being called at parse time is to add a boolean member called something like mParserCreating and set it in your constructor to aFromParser != NOT_FROM_PARSER (you'll probably need to change the NS_IMPL_NS_NEW_HTML_ELEMENT for video and audio to NS_IMPL_NS_NEW_HTML_ELEMENT_CHECK_PARSER instead).  Then condition the mMuted set in SetAttr on mParserCreating.  Also add an implementation of DoneCreatingElement that sets mParserCreating to false.  You'll also need to add your tags (audio and video) to the places where DoneCreatingElement calls happen.  Looks like nsHtml5TreeBuilder::elementPopped, txMozillaXMLOutput::endElement, SinkContext::OpenContainer in nsHTMLContentSink (if that's still used), and nsXMLContentSink::HandleStartElement.

I guess you could get away without the boolean flag: just add the DoneCreatingElement implementation and check HasAttr() in that call, and remove the SetAttr chunk...
(In reply to Boris Zbarsky (:bz) from comment #28)
> I guess you could get away without the boolean flag: just add the
> DoneCreatingElement implementation and check HasAttr() in that call, and
> remove the SetAttr chunk...

This sounds like a good approach.  Does this make sense Andrew?  Would you mind doing a new patch that does this?
(Assignee)

Comment 30

6 years ago
It sure does and will work on it accordingly.
(Assignee)

Comment 31

5 years ago
Created attachment 580944 [details] [diff] [review]
Part B v1

Implemented changes to allow for proper setting of muted attribute. A bit unsure about the commit message though.
Attachment #580944 - Flags: review?(kinetik)
Comment on attachment 580944 [details] [diff] [review]
Part B v1

Thanks, this looks good to me, but it's outside the code I can review, so I'll pass this on to bz.

The test you added in bug 706731 should fail without this patch applied, and pass when it is applied.
Attachment #580944 - Flags: review?(kinetik) → review?(bzbarsky)
Comment on attachment 580944 [details] [diff] [review]
Part B v1

In nsHTMLContentSink, just add to the button case instead of adding new cases with copy/pasted code, please.

r=me with that.  And this still needs tests...
Attachment #580944 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 34

5 years ago
Created attachment 581732 [details] [diff] [review]
Part B v2

Changes to nsHTMLContentSink. About testing this, do the tests added in test_defaultMuted.html in bug 706731 not cover this sufficiently?
Attachment #580944 - Attachment is obsolete: true
Attachment #581732 - Flags: review?(bzbarsky)
Comment on attachment 581732 [details] [diff] [review]
Part B v2

> do the tests added in test_defaultMuted.html in bug 706731 not cover this
> sufficiently?

Not quite.  If they added checks that after the defaultMuted changes are done a1.muted and v1.muted are still false, that would be enough, I think.  Might also be good to assert the muted state before the defaultMuted changes.  Assuming you do that in that bug, r=me.
Attachment #581732 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/40cc6c09ffe9
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/40cc6c09ffe9
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 711839
You need to log in before you can comment on or make changes to this bug.