Closed Bug 890516 Opened 11 years ago Closed 11 years ago

Embedded audio stopped working after installing FF 22

Categories

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

22 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox22 --- wontfix
firefox23 + fixed
firefox24 + fixed
firefox25 --- fixed

People

(Reporter: jonathan, Assigned: mounir)

References

Details

(Keywords: regression)

Attachments

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

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

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
Blocks: 614825
Status: UNCONFIRMED → NEW
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":
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.
Flags: needinfo?(mounir)
Attached file testcase (obsolete) —
IE11 displayed "inline".
The previous patch didn't work on Bugzilla because bugzilla.mozilla.org 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
Status: NEW → ASSIGNED
Attachment #772204 - Flags: review?(bzbarsky)
OS: Windows XP → All
Hardware: x86 → All
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.
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]
Patch

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

Fix indent, please.

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

And here.

r=me
Attachment #773326 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6425dcad7f9e
Flags: in-testsuite+
Target Milestone: --- → mozilla25
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
Attachment #773326 - Flags: approval-mozilla-beta?
Attachment #773326 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/6425dcad7f9e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 773326 [details] [diff] [review]
Patch

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]
Patch

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

Attachment

General

Created:
Updated:
Size: