Closed Bug 890516 Opened 6 years ago Closed 6 years ago

Embedded audio stopped working after installing FF 22


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

22 Branch
Not set



Tracking Status
firefox22 --- wontfix
firefox23 + fixed
firefox24 + fixed
firefox25 --- fixed


(Reporter: jonathan, Assigned: mounir)



(Keywords: regression)


(2 files, 2 obsolete files)

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:

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

Regression window(m-c)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130304 Firefox/22.0 ID:20130304232049
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130305 Firefox/22.0 ID:20130305072450

Regression window(m-i)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130304 Firefox/22.0 ID:20130304214548
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130304 Firefox/22.0 ID:20130305012649

Regressed by:
f73c5ece0869	Mounir Lamouri — Bug 614825 - <embed> should be display:none; when hidden attribute is set. r=bz
Blocks: 614825
Component: Untriaged → DOM: Core & HTML
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
WORKAROUND usrContent.css:

embed {
  display:-moz-box !important;
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...
Flags: needinfo?(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":

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.
Flags: needinfo?(mounir)
Attached file testcase (obsolete) —
IE11 displayed "inline".
The previous patch didn't work on Bugzilla because is in the Compatibility View list.
Attachment #771884 - Attachment is obsolete: true
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".
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → mounir
Attachment #772204 - Flags: review?(bzbarsky)
OS: Windows XP → All
Hardware: x86 → All
Comment on attachment 772204 [details] [diff] [review]

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

You want to go back to the setup we had before bug 614825.
Attachment #772204 - Flags: review?(bzbarsky) → review-
Attached patch PatchSplinter Review
Attachment #772204 - Attachment is obsolete: true
Attachment #773326 - Flags: review?(bzbarsky)
Comment on attachment 773326 [details] [diff] [review]

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

Fix indent, please.

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

And here.

Attachment #773326 - Flags: review?(bzbarsky) → review+
Flags: in-testsuite+
Target Milestone: --- → mozilla25
Comment on attachment 773326 [details] [diff] [review]

[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
Attachment #773326 - Flags: approval-mozilla-beta?
Attachment #773326 - Flags: approval-mozilla-aurora?
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 773326 [details] [diff] [review]

low risk enough and a recent regression in Fx22.Approving on aurora and requesting qa verification
Attachment #773326 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 773326 [details] [diff] [review]

Recent regression in FF22, so approving for Beta as well given where we are in the cycle.
Attachment #773326 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 893377
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!
You need to log in before you can comment on or make changes to this bug.