Closed Bug 689834 Opened 10 years ago Closed 9 years ago
Muted attribute not respected
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()?
W3C's bikeshed on this issue: http://www.w3.org/Bugs/Public/show_bug.cgi?id=10419
Having looked at the code, I suspect I could guide someone through fixing this bug. Chris, please feel free to take my place.
Sounds great Josh.
Added additional check for muted attribute
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
(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.
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
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
Comment on attachment 578087 [details] [diff] [review] Patch v2 I like it, but my blessing is not enough.
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
Do we not need to change UnsetAttr? As in, should/does removing the attribute unmute?
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....
(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.
(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.)
Status: ASSIGNED → RESOLVED
Closed: 9 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 → ---
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?
It sure does and will work on it accordingly.
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+
Changes to nsHTMLContentSink. About testing this, do the tests added in test_defaultMuted.html in bug 706731 not cover this sufficiently?
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+
Flags: in-testsuite? → in-testsuite+
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.