689834 - Muted attribute not respected

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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()?

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 2

[Josh Matthews [:jdm]]

Having looked at the code, I suspect I could guide someone through fixing this bug. Chris, please feel free to take my place.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Sounds great Josh.

Comment 4

[Andrew Quartey [:drexler]]

Attachment: Patch

Added additional check for muted attribute

Comment 5

[Josh Matthews [:jdm]]

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

Comment 6

[Andrew Quartey [:drexler]]

(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.

Comment 7

[Matthew Gregan [:kinetik]]

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

[Josh Matthews [:jdm]]

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.

Comment 9

[Andrew Quartey [:drexler]]

Attachment: 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 :-)

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 578087 [details] [diff] [review]
Patch v2

I like it, but my blessing is not enough.

Comment 11

[Matthew Gregan [:kinetik]]

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.

Comment 12

[Andrew Quartey [:drexler]]

Filed Bug 706731 and Bug 706735 to address defaultMuted and muted in parser/html respectively.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Do we not need to change UnsetAttr?  As in, should/does removing the attribute unmute?

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/47eba8a9a3b8

Comment 15

[Boris Zbarsky [:bzbarsky]]

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....

Comment 16

[Andrew Quartey [:drexler]]

(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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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?

Comment 18

[Matthew Gregan [:kinetik]]

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.

Comment 19

[Andrew Quartey [:drexler]]

(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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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?

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

(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.)

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/47eba8a9a3b8

Comment 25

[Matthew Gregan [:kinetik]]

(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.

Comment 26

[Matthew Gregan [:kinetik]]

(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        |

Comment 27

[Matthew Gregan [:kinetik]]

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.)

Comment 28

[Boris Zbarsky [:bzbarsky]]

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...

Comment 29

[Matthew Gregan [:kinetik]]

(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?

Comment 30

[Andrew Quartey [:drexler]]

It sure does and will work on it accordingly.

Comment 31

[Andrew Quartey [:drexler]]

Attachment: Part B v1

Implemented changes to allow for proper setting of muted attribute. A bit unsure about the commit message though.

Comment 32

[Matthew Gregan [: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.

Comment 33

[Boris Zbarsky [: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...

Comment 34

[Andrew Quartey [:drexler]]

Attachment: Part B v2

Changes to nsHTMLContentSink. About testing this, do the tests added in test_defaultMuted.html in bug 706731 not cover this sufficiently?

Comment 35

[Boris Zbarsky [: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.

Comment 36

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40cc6c09ffe9

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/40cc6c09ffe9