890516 - Embedded audio stopped working after installing FF 22

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonathan Allen]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

Clicked on button marked 'Hear Hebrew' in the URL:

http://www.messianictrust.org.uk/parashiyot/mattot-10.php

Same problem can be seen by clicking over any of the Hebrew word graphics in the text or clicking the 'Hear Hebrew (HQ, slower)' button


Actual results:

The sound file did embed, but made no sound


Expected results:

The sound file should have played

Comment 1

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/015da7030aab
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130304 Firefox/22.0 ID:20130304232049
Bad:
http://hg.mozilla.org/mozilla-central/rev/c95439870e05
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130305 Firefox/22.0 ID:20130305072450
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=015da7030aab&tochange=c95439870e05

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c43e96e4a41a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130304 Firefox/22.0 ID:20130304214548
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/f73c5ece0869
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130304 Firefox/22.0 ID:20130305012649
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c43e96e4a41a&tochange=f73c5ece0869

Regressed by:
f73c5ece0869	Mounir Lamouri — Bug 614825 - <embed> should be display:none; when hidden attribute is set. r=bz

Comment 2

[Alice0775 White]

WORKAROUND usrContent.css:

embed {
  display:-moz-box !important;
}

Comment 3

[Boris Zbarsky [:bzbarsky]]

So I just checked, and @hidden on <embed> does NOT make it display:none in WebKit either.

Mounir, maybe this is just not web-compatible at all and the spec needs to be changed here...

Comment 4

[Mounir Lamouri (:mounir)]

Oh boy... So, Webkit and Blink do not make the element display:none indeed. They only put CSS width and height to 0. Not when @hidden is set but when @hidden is set to "yes" or "true":
https://github.com/WebKit/webkit/blob/master/Source/WebCore/html/HTMLEmbedElement.cpp#L84
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp&l=84

I wasn't able to really understand what IE is doing (because I use browserstack and their devtools do not look great or changing @hidden has no effect). Same kind of problem with Opera/Presto: the embed element never gets added to the DOM.

I would be very interested to know what IE is doing. If they have the same behaviour as Webkit and Blink I think we have no choice but to add this to the specs and implement it.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: testcase

IE11 displayed "inline".

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: testcase with the latest document mode forced

The previous patch didn't work on Bugzilla because bugzilla.mozilla.org is in the Compatibility View list.

Comment 7

[Mounir Lamouri (:mounir)]

So, IE9/10 behaviour is the following:
- always display: inline;
- if @hidden is set to the empty string or "true", hide the element.

Webkit/Blink does:
- always display: inline;
- if @hidden is set to "yes" or "true", hide the element.

I think we could safely try to push something where @hidden is followed if set to the empty string, "yes" or "true". We could probably try to follow @hidden if set to anything but "false" or "no".

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Patch

This is building so it is not tested yet but I guess it should work. I will open a follow-up wrt implementing the right thing.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 772204 [details] [diff] [review]
Patch

MapCommonAttributesInto is a static function, so how can it use Tag()?

You want to go back to the setup we had before bug 614825.

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 773326 [details] [diff] [review]
Patch

>+MapAttributesIntoRuleBase(const nsMappedAttributes *aAttributes,
>                       nsRuleData *aData)

Fix indent, please.

>+MapAttributesIntoRuleExceptHidden(const nsMappedAttributes *aAttributes,
>+                      nsRuleData *aData)

And here.

r=me

Comment 12

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6425dcad7f9e

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 773326 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 614825
User impact if declined: some websites don't work
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low, no alternatives
String or IDL/UUID changes made by this patch: no

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6425dcad7f9e

Comment 15

[bhavana bajaj [:bajaj]]

Comment on attachment 773326 [details] [diff] [review]
Patch

low risk enough and a recent regression in Fx22.Approving on aurora and requesting qa verification

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 773326 [details] [diff] [review]
Patch

Recent regression in FF22, so approving for Beta as well given where we are in the cycle.

Comment 18

[Stefan]

Thank you.

This kind of response to issues the main reason I remain a loyal Firefox advocate, besides the fact that FF is the only browser that fully supports MathML (Safari only partially implements it, and Chrome seems to have given up and has removed support..)

Great job all!

Comment 19

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f1cf071add8e
https://hg.mozilla.org/releases/mozilla-beta/rev/c653726e0cc8