613722 - Embedded .mp3 doesn't play when hidden

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Oskar]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7) Gecko/20100101 Firefox/4.0b7
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7) Gecko/20100101 Firefox/4.0b7

On Firefox 4 Beta 7 (German) embaded .mp3 files are not played as long as the hidden tag is active:

<embed src="WeAreYoutube.mp3" autostart="true" hidden="true" />

Like this nothing is played

But if it's deleted like this:

<embed src="WeAreYoutube.mp3" autostart="true" />

There are no problems anymore!

Reproducible: Always

Actual Results:  
Not playing mp3

Expected Results:  
Playing mp3

Comment 1

[Matthias Versen [:Matti]]

I can confirm this with Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101117 Firefox/4.0b8pre SeaMonkey/2.1b2pre

Comment 2

[Johnny Stenback (:jst)]

Is this a windows only problem? Can someone try to see when this regressed, i.e. what's the last version that this worked in?

Comment 3

[Matthias Versen [:Matti]]

Johnny, according to regression.py :
Last good nightly: 2010-08-01 First bad nightly: 2010-08-02

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=070d9d46d8
8b&tochange=a4d86c3a3494

maybe bug 567663 ?
I can only test on win32 and I used QT 7.6.7 as plugin.

Comment 4

[Johnny Stenback (:jst)]

Mounir, can you have a look here? Maybe start with a local backout of bug 567663 and work from there?

Comment 5

[Mounir Lamouri (:mounir)]

The issue is that the hidden attribute is equivalent to "display:none;" and we don't activate embed elements with "display:none;".
It looks like Safari, Chrome and Opera do activate the embed element even with "display:none;" (and with hidden attribute set).

Comment 6

[James McIntosh]

I can confirm that this affects the Mac and Windows versions.

Comment 7

[Johnny Stenback (:jst)]

Mounir, is that something that changed as part of us fixing bug 567663? If so, I think we need to revert the plugin related parts of that bug since we're unable to instantiate plugins without their frame being created, which doesn't happen if we treat it as if the style for the element is display:none;.

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to comment #7)
> Mounir, is that something that changed as part of us fixing bug 567663? If so,
> I think we need to revert the plugin related parts of that bug since we're
> unable to instantiate plugins without their frame being created, which doesn't
> happen if we treat it as if the style for the element is display:none;.

That doesn't seem to be related. On 3.6, <embed> with "display:none;" doesn't run AFAICT.

Comment 9

[Johnny Stenback (:jst)]

(In reply to comment #8)
> That doesn't seem to be related. On 3.6, <embed> with "display:none;" doesn't
> run AFAICT.

Correct, but in 3.6 <embed type=hidden> does work, right?

Comment 10

[Matthias Versen [:Matti]]

>Correct, but in 3.6 <embed type=hidden> does work, right?
The testcase URL works in 3.6.12

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #9)
> (In reply to comment #8)
> > That doesn't seem to be related. On 3.6, <embed> with "display:none;" doesn't
> > run AFAICT.
> 
> Correct, but in 3.6 <embed type=hidden> does work, right?

<embed hidden> is very likely working in 3.6 given that @hidden has no meaning but if it's working, it will show the plugin.
We could probably fix that by making hidden behaving like "visibility:hidden;" or even doing nothing for <embed> but wouldn't it more interesting to not disable plugins when they have no frame?

Comment 12

[Johnny Stenback (:jst)]

Yes, long term we need to make plugins load independently of whether or not they have a frame, and there's a bug on that (bug 90268), but we can not fix that for Firefox 4. So yeah, either making it behave like visibility:hidden or doing nothing for plugins seems like the least bad option atm.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I realized while reading some code in nsObjectFrame.cpp that hidden actually had a meaning for embed. The regression makes more sense now :)

Now, setting hidden on an <embed> should behave exactly like in 3.6 (somewhat hiding the element but with a frame). However, @hidden will make <object> and <applet> being disabled given that, AFAICT, @hidden had no sense for those elements.

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

I forgot to hg add the plugin file.

Comment 15

[Oskar]

So will it be fixed on Firefox 4.0b8? =)

Comment 16

[Mounir Lamouri (:mounir)]

(In reply to comment #15)
> So will it be fixed on Firefox 4.0b8? =)

4.0b8 is going to be released soon so it might be fixed in 4.0b9.

Comment 17

[Oskar]

Okay thank you very much =) ^^

Comment 18

[Johnny Stenback (:jst)]

Patch looks good, but shouldn't we do the same for <object> and <applet> too? Mind testing how they work in 3.6?

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to comment #18)
> Patch looks good, but shouldn't we do the same for <object> and <applet> too?
> Mind testing how they work in 3.6?

<object> and <applet> are ignoring the hidden attribute in 3.6. Only <embed> has a hack to be somewhat hidden but still active in 3.6 when the hidden attribute is set.

According to the specs, <applet> should ignore the display property [1] so we should be able to ignore the hidden attribute on them. It should have no effect currently actually but I'm not able to test (I need a java plugin).
However, for <object>, given that there are no retro-compatibility issues, we can ignore the hidden attribute or apply it. If we ignore it, objects used for non-plugins would not be hidden. If not ignored, objects used for plugins would be disabled. I don't know what should be the worst solution here...
We could also ignore the hidden attribute when <object> is a plugin but we are currently using GetAttributeMappingFunction() and the <object>'s type is a bit more than just an attribute so it might be wrong, isn't it?

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/obsolete.html#the-applet-element

Comment 20

[Boris Zbarsky [:bzbarsky]]

You can condition the return value of GetAttributeMappingFunction on the type (however you define "type") as long as you make sure that a type change triggers a reframe.

Comment 21

[Mounir Lamouri (:mounir)]

(In reply to comment #20)
> You can condition the return value of GetAttributeMappingFunction on the type
> (however you define "type") as long as you make sure that a type change
> triggers a reframe.

Doing that would require to hack in nsObjectLoadingContent which seems a bit scary. And, given the relation between plugins and frames, wouldn't that be risky to reframe on type change?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Well... When we say "type" here, we mean type in the nsObjectLoadingContent::Type() sense, right? 

So if changes to nsObjectLoadingContent::mType always trigger a reframe (which is something to double-check), then we already have the required invariant.

Comment 23

[Mounir Lamouri (:mounir)]

(In reply to comment #22)
> Well... When we say "type" here, we mean type in the
> nsObjectLoadingContent::Type() sense, right? 

Yes.

> So if changes to nsObjectLoadingContent::mType always trigger a reframe (which
> is something to double-check), then we already have the required invariant.

Currently mType change doesn't trigger a reframe so we would have to do that and that seems a bit sensitive or am I just over-scared?
As a reminder, the only reason to do that is to prevent <object> element with the hidden attribute containing plugins to be disabled which is not a regression.

By the way, shouldn't we do that in another bug and push this fix?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Currently mType change doesn't trigger a reframe 

Are you sure?  Are there mType changes that happen without an auto notifier on the stack?  auto notifier will reframe if the type changed, no?

Another bug for that is fine by me, yes.

Comment 25

[Mounir Lamouri (:mounir)]

(In reply to comment #24)
> > Currently mType change doesn't trigger a reframe 
> 
> Are you sure?  Are there mType changes that happen without an auto notifier on
> the stack?  auto notifier will reframe if the type changed, no?

TBH, I don't know. I've checked that GetAttributeMappingFunction is called after the mType change which is not the case. I've had a quick loot at callers of GetAttributeMappingFunction and it doesn't sound obvious that this method is called after a reframe and I understood that from comment 20.

> Another bug for that is fine by me, yes.

Jst, if you can review this patch, I will open a follow-up to have all plugins behaving like <embed> does when the hidden attribute is set.

Comment 26

[Boris Zbarsky [:bzbarsky]]

GetAttributeMappingFunction is called any time we need to restyle as part of the restyling process, fwiw.  We should follow up in this new bug.

Comment 27

[Mounir Lamouri (:mounir)]

I've open bug 615933 to make other plugins active but not visible with @hidden set.

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 493403 [details] [diff] [review]
Patch v1.1

r=jst

Comment 29

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/ef449201d114

Oskar, beta 8 will fix this bug.