36997 - nested EMBED tag incorrectly placed if align=top on parent OBJECT

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Max Hammond]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; m15)
BuildID:    2000041805

On http://www.beme.com the flash content plays fine, but on following the link
to "View with Flash 4.0" there's nothing to see, except for some motion in a
very thin band just under the location bar. Perhaps the movie is being displayed
off the screen?

Reproducible: Always
Steps to Reproduce:
1. View URL


Actual Results:  Content not properly displayed	

Expected Results:  Show the movie

Comment 1

[shrirang khanzode]

I see the behaviour you mentioned. Seems like the ' align="left" ' tag is not 
being understood by the browser properly. Copying the source code locally and 
changing the value to 'align=left' works and the movie loads and plays fine.

Comment 2

[shrirang khanzode]

oops..the original tag is "align=top". changing it to "align=left" works.

Comment 3

[shrirang khanzode]

Even removing the "align=top" tag works, but I am wondering why we have to do 
this...in order for the movie to play.

Comment 4

[Max Hammond]

Changing the attribute to align=texttop makes it work.
http://www.w3.org/TR/WD-object-970218 explains somewhat. Seems like it's
(technically) a problem with the page rather than the browser, but this doesn't
explain why it's mucking things up so badly.
 setting align=randomgarbage doesn't make it break, so it must be actively
getting it wrong :) (and "other browsers" can handle align=top) oddly,
http://www.w3.org/TR/html4/struct/objects.html#h-13.7.4 suggests that align=top
should be okay...

Comment 5

[shrirang khanzode]

Thanks for the info. I believe this is not a plugin problem since the plugin 
plays properly. Reassigning to HTML element component. 

Comment 6

[Chris Petersen]

I can reproduce the problem with the April 21 build. I noticed that the url 
contains a object with a nested EMBED. After some research, it looks like the 
object element is not able to render the flash animation so I passes to the EMBED 
element. If you comment out the EMBED element, the document will not rendered the 
flash src. The align="top" attribute is supported in object and is used to 
vertically align the top of object with the top current of the text line. I have 
a test case that works with align=top at http://mozilla.org/quality/browser/
standards/html/object_align_top.html.

Comment 7

[rickg]

Andrei -- please take a look.

Comment 8

[ekrock's old account (dead)]

nsbeta2 6/1-. Marking flash. Shouldn't this be reassigned to whoever's 
fixing HTMLElement cmpn bugs? Who is that?

Comment 9

[Peter Trudelle]

[nsbeta2-] 

Comment 10

[av (gone)]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another 
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration. 

Comment 11

[gerardok]

massive update for QA contact.

Comment 12

[ekrock's old account (dead)]

Raising priority to P2--high profile backward compatibility for Flash content on 
the web. Clearing Future to -- and nominating nsbeta3.

Comment 13

[Chris Petersen]

Removing the align=top from the object element (which contains the embed element) 
will cause the flash anaimation to be rendered.

Comment 14

[karnaze (gone)]

Marking nsbeta3+ and cc-ing attinasi, peterl. 

Comment 15

[karnaze (gone)]

Adding rtm+.

Comment 16

[Phil Peterson]

PDT marking [rtm-] because there are workarounds

Comment 17

[ekrock's old account (dead)]

Flash crasher or backward incompatibility bug. Nominating for nsbeta1 as
high-quality plug-in support is a priority for embedded applications.

Comment 18

[Loco]

REMOVING nsbeta2/3 and replacing with nsbeta1 KW.  Also clearing 
Status Whiteboard of nsbeta2/3 result.  We really should get Flash 
working.  Not that I like it, but other people do:).

Comment 19

[Peter Lubczynski]

Attachment: testcase

Comment 20

[Peter Lubczynski]

This is caused by the align=top on the OBJECT tag. It's a floater but a 
placeholder isn't generated for it. 

Changing summary from: Flash movie doesn't play properly (wrong position?)

Sending it over to buster and nominating...

Comment 21

[Hixie (not reading bugmail)]

Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.

Comment 22

[karnaze (gone)]

Moving to Mozilla0.9

Comment 23

[bsharma]

qa contact updated.

Comment 24

[Chris Waterson]

taking

Comment 25

[Heikki Toivonen (remove -bugzilla when emailing directly)]

SPAM. HTML Element component deprecated, changing component to Layout. See bug
88132 for details.

Comment 26

[av (gone)]

*** Bug 64645 has been marked as a duplicate of this bug. ***

Comment 27

[Jonathan S Abrams]

Changing platform and OS to reflect the conditions of bug 64645, which has been
deemed a duplicate of this bug.

Comment 28

[Peter Lubczynski]

*** Bug 105944 has been marked as a duplicate of this bug. ***

Comment 29

[Peter Lubczynski]

back to me....still happening....

Comment 30

[Peter Lubczynski]

*** Bug 103438 has been marked as a duplicate of this bug. ***

Comment 31

[Peter Lubczynski]

Happens on PBS which I think is top100.

http://www.pbs.org/newshour/video/index.html


Hey Marc or Chris, do you have any tips on what could be causing this? Thanks!

Comment 32

[Chris Waterson]

Is the attribute mapping is working correctly (i.e.,
nsIContent::GetMappedAttributeImpact)? Setting |align={left|right}| are supposed
to float the tag (per GetImageAlignAttributeImpact). What is |align=top|
supposed to do?

Comment 33

[Peter Lubczynski]

Well, nsHTMLObjectElement.cpp calls nsGenericHTMLElement::MapAlignAttributeInto
which looks like it only deals with left and right. I don't see where
|align=top| would be even be mapped in for the IMG tag. According to W3C, this
should work for Objects like it does for Images? Does |align=top| work for images?

Comment 34

[Peter Lubczynski]

Attachment: testcase showing OBJECT and IMG align=TOP working!

Comment 35

[Peter Lubczynski]

Well, the orrignal web site, http://www.beme.com, is gone! Can someone attach a
(non)-working testcase?

I don't think the problem is with the mapping anymore. With the last attached
testcase, it seems to work just fine.

I think the problem maybe when the OBJECT tag specified a align=top but NOT the
EMBED tag. Since we render the EMBED tag instead of the OBJECT in the nested case.

Should the EMBED check it's parent OBJECT for allignment?

Comment 36

[av (gone)]

How about http://bugzilla.mozilla.org/showattachment.cgi?attach_id=50773 ?

Comment 37

[Peter Lubczynski]

Oh yeah, that's a good testcase. It shows the problem with the OBJECT tag having
align=top and not the EMBED.

Comment 38

[Peter Lubczynski]

This is very strange. How does align=top work? It looks like the "line" above
the BODY is getting a bogus y value of "-3255" which is causing everything else
to be wrong. However, the actual BODY looks like it has the right y value. I'm
attaching a frame dump of the tescase with a normal dump without align=top to
compare. 

Comment 39

[Peter Lubczynski]

Attachment: frame dump of first testcase

Comment 40

[Marc Attinasi]

Here is a reduced testcase that does not require any plugins:

<object align="top">
 <img src="http://www.mozilla.org/images/mozilla-banner.gif">
</object>

Comment 41

[Peter Lubczynski]

Attachment: attachment form of Marc's simple non-plugin testcase

Comment 42

[Peter Lubczynski]

Removing the align=top on the OBJECT tag and looking in Viewer with frame
borders on, it looks like the line is the wrong height and cuts through the
middle of the image. In the debugger, I can see the height of the inline being
set to only the font metrics size:
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsInlineFrame.cpp#611
There is comment there that says "the height of children do not affect our
height" and that code is pretty old so I'm not even sure I'm looking in the
right place.

cc:ing dbaron for some advice...

Comment 43

[David Baron :dbaron:]

Well, that looks like the correct behavior for 'vertical-align: text-top' and
perhaps also 'vertical-align: top' (although I would argue it isn't) on an
inline element.  Maybe we shouldn't be mapping the ALIGN attribute on the OBJECT
element to 'vertical-align' when the element isn't replaced, though that seems
tricky.  Ian, any ideas?

Comment 44

[Peter Lubczynski]

Even though it's deprecated, 4.01 spec does mention align=top:
http://www.w3.org/TR/html4/struct/objects.html#h-13.7.4

Should align=top be ignored on the object tag if the contents get rendered?

Comment 45

[Stephen Moehle]

I may be dense, but I do not see why align=top is supposed to work the way it
currently is.  Section 13.7.4 quoted by Peter says for top: "top: means that the
top of the object should be vertically aligned with the top of the current text
line."  I cannot fathom any way in which "top of the object should be vertically
aligned with the top of the current text line" means to push the object way
beyond the top of the page as I see nothing that would say that the top of the
current text line is above the top of the page.  If you add some text next to
the image in Marc's example, the text appears fine so it seems that the top of
the current text line is well within the boundaries of the page.

Comment 46

[David Baron :dbaron:]

...because the image sticks out of the top of the inline frame for the object.

However, one can argue about what 'vertical-align: top' should do in such a case
-- see my proposal at http://www.people.fas.harvard.edu/~dbaron/css/2000/01/dibm

Comment 47

[shrirang khanzode]

this is clearly visible on the shockwave site(hence it's important to be fixed 
since a lot of users will notice it )
 
http://www.shockwave.com/sw/games/ 

The football image at the bottom right is peeking outside the table border.

Comment 48

[Peter Lubczynski]

After looking at this again this weekend, I think I agree with dbaron. Maybe for
replaced object elements, we should drop the 'vertical-align:' style? IE 6 seems
to ignore it in this situation but we try to support it (try 'sub' or a length).
Could one of the layout guys maybe suggest a strategy? I was thinking of setting
a style rule in nsCSSFrameConstructor::CantRenderReplaceElement(), but I'm not
quite sure how to set it?

Comment 49

[Peter Lubczynski]

Attachment: patch to remove align attribute on replaced object frames

We have problems with other "align attributes like "right" on the object frame
when it's replaced as well. This patch removes any align attribute before
calling CantRenderReplaceElement() to match our behavior with IE.

This hack isn't really a "fix" for this bug, but it should make top100 sites
that show this problem look better. I'm wondering if I can get reviews for this
patch and then move this bug to post-1.0 for a better fix?

Comment 50

[David Baron :dbaron:]

That seems like a bit too much of a hack.  Would it be possible to change the
MapAttributesIntoRule in content/html/content/src/nsHTMLObjectElement.cpp so
that it doesn't call nsGenericHTMLElement::MapAlignAttributeInto if the object
element is replaced?  Or does the information about whether the object is
replaced live only on the frame (that would seem bad)?

Comment 51

[Peter Lubczynski]

We don't know we are going to be replaced until the end of reflow, at which time
we post a CantRenderReplaceElement(). I can't think of a way the content knows
that the frame has been replaced. I guess I can try to put in a hook however, I
don't see any call to MapAttributesIntoRule for the TextReset after the point I
know I need to be replaced. Will that be a problem or should re-resolving style
and a reflow fix that?

Comment 52

[Peter Lubczynski]

Attachment: slightly less hacky approach of resetting vertical align on replaced objects

I tried to change |MapAttributesIntoRule| to block the calling of
|nsGenericHTMLElement::MapAlignAttributeInto| but since this static mapping
function isn't a member of the content class, I didn't have anything to work
with. I need to at least get at the content so I can call |GetPrimaryFrameFor|.
Suggestions?

So anyway, here a patch with a slightly different approach. I just reset the
vertical align style back to the default before we start to replace the object
frame. Please reivew.

Comment 53

[David Baron :dbaron:]

There's an API for doing that that won't break things like inherit bits.  You
want to call nsIStyleContext::GetUniqueStyleData (and you'll need to extend
nsStyleContext::GetUniqueStyleData for the TextReset struct), and then modify
that struct.

Comment 54

[Peter Lubczynski]

Attachment: patch using GetUniqueStyleData

This seems to work. Thanks for the tip David, could you r= this patch?

Comment 55

[David Baron :dbaron:]

Comment on attachment 61210 [details] [diff] [review]
patch using GetUniqueStyleData

>+    nsStyleTextReset* text = 
>+      (nsStyleTextReset*)mStyleContext->GetUniqueStyleData(mPresContext,  eStyleStruct_TextReset);

r=dbaron if you change the above to a new-style cast:

nsStyleTextReset* text = NS_STATIC_CAST(nsStyleTextReset*,
  mStyleContext->GetUniqueStyleData(mPresContext, eStyleStruct_TextReset));

Comment 56

[Peter Lubczynski]

Attachment: correct patch

Comment 57

[Marc Attinasi]

Comment on attachment 61213 [details] [diff] [review]
correct patch

I think that 
NS_STYLE_INHERIT_TEXT

should be NS_STYLE_INHERIT_TEXT_RESET
Check that out. sr=attinasi if you do.

Comment 58

[Peter Lubczynski]

Attachment: really corrected patch

Comment 59

[Brendan Eich [:brendan]]

Comment on attachment 61550 [details] [diff] [review]
really corrected patch

+    const nsStyleTextReset* reset = (const
nsStyleTextReset*)GetStyleData(aSID);

This could be new-style (NS_STATIC_CAST), too.

a=[e-mail] for 0.9.7 in any case.

/be

Comment 60

[Peter Lubczynski]

Patch in trunk, marking FIXED.

Comment 61

[Stephen Moehle]

The problem I reported with pbs.org (NewsHour and Washington Week video) is now
fixed with Mozilla trunk 2001-12-14-03 on Win2000.

Comment 62

[Peter Lubczynski]

Embedding may want this because it effects top100 sites, nominating...

Comment 63

[Judson Valeski]

removing URL that is now dead.

Comment 64

[Peter Lubczynski]

not quite fixed yet....this causes crashing in some other major plugin sites so
I just backed myself out until there can be a better fix... :(

Comment 65

[Peter Lubczynski]

Attachment: check for null

okay, this patch seems to be better, now checking for null....

Comment 66

[Peter Lubczynski]

Comment on attachment 61550 [details] [diff] [review]
really corrected patch

okay, I'm an idiot....this patch is fine. The crash I was seeing was because I
didn't re-link the content DLL so naturally it didn't pick-up my changes and
returned null. In fact, Stephen in comment #61 says this patch is good so I'll
try to land it again tonight. Sorry for the trouble....

Comment 67

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.7

Comment 68

[Arun Ranganathan]

Nominating edt0.9.4

Comment 69

[Peter Lubczynski]

fixed0.9.7

Comment 70

[Michael Dunn]

Per shrir comment:
http://bugzilla.mozilla.org/show_bug.cgi?id=36997#c47
I suggest we include for 094. I would like to see it verified fixed on trunk/097.

Comment 71

[shrirang khanzode]

Yes, this is fixed on trunk/0.9.7 branch build 1220. I tried all testcases/ 
duped bugs and the shockwave site as well. Looks good.

Comment 72

[Michael Dunn]

Plused for 094

Comment 73

[Peter Lubczynski]

fixed0.9.4

Comment 74

[shrirang khanzode]

verif fixed on 0101 0.9.4 branch build.