37622 - <embed hidden="true"> doesn't really hide

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Open http://bugzilla.mozilla.org/ maximized in background
2. Load http://www.susumama.co.uk/ and scroll to bottom, or load testcase
3. Alt-tab to maximized window and back

Expected results: page looks same before and after alt-tabbing around
Actual results: "nothing displayed" (cf bug 5569) where the plugin would be.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

A "hidden" embed also takes up room in the page layout in mozilla, by the way.

Comment 3

[ekrock's old account (dead)]

Nominating nsbeta2. We're trying to get our Plug-in support robust enough for 
nsbeta2 that plug-in developers will be able to develop and test upgraded 
plug-in binaries without being slowed down by bugs such as this causing basic 
backward incompatibilities vs. how Nav4 handled HTML 3.2-level markup. (Even if 
EMBED wasn't formally part of HTML 3.2, it falls into that category in terms of 
importance.)

Comment 4

[Peter Trudelle]

[nsbeta2+]

Comment 5

[av (gone)]

This page uses JavaScript to start the plugin. LiveConnect is not supported for 
ols school plugins. Probably this one should marked WONT FIX.

Comment 6

[sean echevarria]

Actually it uses javascript to instantiate a plugin and that part works.

The problem is the HIDDEN=TRUE gets completely ignored.
Jesse created a simplified testcase - when viewed in nn4x the plugin is not 
visible.  But in mozilla, it is visible (if you copy the nn4x npaudio.dll 
plugin into the mozilla plugins dir).  

Maybe a problem in nsObjectFrame.cpp - nsPluginInstanceOwner::CreateWidget.  A 
window is always created regardless of the HIDDEN tag.  In fact, 
nsPluginInstanceOwner doesn't have any notion of a hidden plugin.  When HIDDEN 
is true, then the width and height tags (if any) should be ignored as well - 
the plugin should not affect layout in any way.

Comment 7

[av (gone)]

Right. I mislooked the problem. Has nothing to do with LC.

Comment 8

[av (gone)]

Reassigning as per agreement.

Comment 9

[Chris Waterson]

Attachment: one way to fix it

Comment 10

[Chris Waterson]

dbaron: am I dirty for wanting to do this? or should we look for a native 
solution? I still need to figure out how to deal with 22037, so it may be that 
something better falls out of that...

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd say it's low priority, but it *might* be a very easy fix.  I'm not sure if 
changing the attribute mapping function to map HIDDEN=TRUE to 'display: none' 
will do the trick or not.  I've been trying to learn how attribute mapping 
works...

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not sure if that's the right fix or if it's:

embed[hidden="true"] {
  display: none;
}

(I guess you don't *need* to use attribute mapping...)

Comment 13

[Chris Waterson]

"display: none" makes it so that no frame gets instantiated. Apparently, the 
plugin needs a frame to be loaded.

There is plenty of gunk in nsCSSFrameConstructor for special-casing <object>, 
<embed>, and <applet>, so I suppose I could just patch around that.

Thoughts?

Comment 14

[Chris Waterson]

Hrm, never mind. The plugin frame doesn't want to obey the width and height 
properties, so it creates a 100px floated frame. So floating is not useful.

Comment 15

[sean echevarria]

The html.css fix will work if we don't hand the plugin a window.  

In nsObjectFrame.cpp, add a PRBool mVisible member to nsPluginInstanceOwner.  
Check the state in nsPluginInstanceOwner::CreateWidget - at the top add:

  If (!mVisible) 
    return NS_OK;

I set a breakpoint at this line in the debugger.  When it hit, I set my ip to 
the return statement.  Got an invisible plugin!

Now, to set that member, need to strcmp all attributes and parameters.  
nsPluginInstanceOwner::CreateWidget should be an ok place to do that since the 
attributes and parameters should have already been cached by the time it gets 
called.  Just a matter of looping thru the two arrays.

Tried the simple breakpoint proof of concept with both the nn4x audio plugin 
and with my xpcom plugin.

Just looked at nsPluginHostImpl.cpp and see that CreateWidget is called from a 
couple of different places - so might need to rethink how to get and save the 
mVisible state.

Comment 16

[Chris Waterson]

I went back through the Classic codebase and here's what I've found:

- The layout engine "creates" the embed object in lo_FormatEmbedInternal() by
calling FE_GetEmbedSize(), one of the wonderful MWContext macros that ends up
dispatching through the callbacks there.

- The platform-specific GetEmbedSize() callback pulls its information out of the
LO_EmbedStruct's objTag.FE_Data member (objTag is an NPEmbeddedApp struct). If
this is null, then the front-end knows that it's the first time it's creating
the object, and calls NPL_EmbedCreate(). (Otherwise, it goes through gyrations
to reconstitute the object's state from session history.)

- If LO_ELE_HIDDEN is not set, then NPL_EmbedCreate() will call
FE_CreateEmbedWindow(), which is another MWContext-dispatched callback.


I think that's as far as I need to care about: FE_CreateEmbedWindow() is only be
called if the LO_ELE_HIDDEN attribute is not set, and the LO_ELE_HIDDEN
attribute is only set in the case of an <applet> or <embed> tag. In other words,
if I encounter an <applet> or <embed> tag with a hidden="true" attribute (well,
see this code <http://lxr.mozilla.org/classic/source/lib/layout/layembed.c#373>
for the exact logic: looks like hidden set to "no", "false", or "off"), then
we'll simply punt on creating the plugin window.

cc'ing pavlov & blizzard in case they have any objections...

Comment 17

[Chris Waterson]

I need pav to check in fixes for 37477 first.

Comment 18

[Chris Waterson]

Attachment: "beat it with a stick until it screams uncle"

Comment 19

[Chris Waterson]

Ok, so I'm not particularly proud of that patch, but it seems to work on Linux.
I'm going to try it on Mac and Win32 tomorrow.

Comment 20

[sean echevarria]

The patch works on Win32.

The 'kooky' NS_LITERAL_STRING checks in IsHidden() should be case-insensitive.

Why not support the hidden parameter of object tags?

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should they really be case-insensitive?  Don't we normalize all attribute values
in the content model to lowercase?

Comment 22

[sean echevarria]

Normalization of attribute values does not seem to be occurring.  I modified 
the test case and set HIDDEN=FALSE but still got a hidden plugin.

Comment 23

[Chris Waterson]

Yes, I should change that (attribute *values* aren't normalized, just the 
attribute *names*).

As for "why embeds only"? I'm pretty sure that HTML4 doesn't support a "hidden" 
attribute on the <object> tag. (FWIW, I don't think that 4.x did either.)

Comment 24

[sean echevarria]

Right - HIDDEN is not an attribute of OBJECT (was confused because my ActiveX 
control supports HIDDEN as a custom parameter).

Comment 25

[Chris Waterson]

fix checked in.

Comment 26

[sean echevarria]

Missing the nsPluginHostImpl.cpp changes?

Comment 27

[Chris Waterson]

Damn. Checked that in too.

Comment 28

[shrirang khanzode]

verified (build:2000072408)

Comment 29

[Chris Waterson]

Damn damn damn. I completely blanked on this. Just checked in the 
case-insensitive comparison stuff on the tip.