83159 - HTML Embed region is not painted correctly

Status: VERIFIED FIXED

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

Description

[Chris Petersen]

Build: 2001052904
Platform: Windows and Mac 9(QT plugin not avaiable for Linux)
Expected Results: Embed area should be painted with the bgcolor of the page.
What I got: The Embed area (outside of the QT movie) is displayed as grey
Steps to reproduce: 
1) Install QT plugin
2) Go to Apple's url
3) Embed region is displayed grey.

Comment 1

[Chris Petersen]

Attachment: reduced test case of issue

Comment 2

[Peter Lubczynski]

-->peterl

Comment 3

[Peter Lubczynski]

nominating mozilla0.9.2

Comment 4

[Peter Lubczynski]

This is probably in the Paint() method. Over to Chris Karnaze....

Comment 5

[karnaze (gone)]

Moving to m0.9.3

Comment 6

[Peter Lubczynski]

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

Comment 7

[Blake Ross]

Missed 0.9.3.

Comment 8

[karnaze (gone)]

reassigning to m0.9.6

Comment 9

[Peter Lubczynski]

my bug

Comment 10

[Peter Lubczynski]

Attachment: windows fix

Comment 11

[Peter Lubczynski]

Attachment: windows fix

Comment 12

[av (gone)]

Peter, do you this problem on windows?

Comment 13

[av (gone)]

Attachment: Ah... Slightly modified test case to reveal the bug on my Windows machine

Comment 14

[av (gone)]

Comment on attachment 56192 [details] [diff] [review]
windows fix

This patch does not fix it for me on Windows. 
GetStyleData rerurns 0xffffff for the color which I beleive is white.

Comment 15

[Peter Lubczynski]

Okay, I think I know what's going on. There are several problems here. Andrei
shows that we are not mapping background style color correctly in content. I see
where I think I can fix this in nsHTMLObjectElement.cpp and
nsHTMLAppletElement.cpp, but what about the EMBED tag? cc:ing Jonny as I recall
it was removed recently in bug 107453.

On Mac, RGBBackColor() does do the trick and needs to be called with the
background color from style. It needs to be called before a paint update. For
some reason, it doesn't always work on the first update so I thought it wasn't
working at all :-/ This could be because of the above background mapping
problem, I wasn't calling it on the right port, or maybe Quicktime also expects
it to be set in NPP_SetWindow() where we also pass the port.

Comment 16

[Johnny Stenback (:jst)]

The code for <embed> now lives in nsHTMLSharedLeafElement.cpp, check that
mNodeInfo->Equals(nsHTMLAtoms::embed) before doing embed specific things.

Comment 17

[Peter Lubczynski]

Attachment: window fix

Comment 18

[Peter Lubczynski]

I found one more problem: turns out that since the body background color doesn't
get inherited up to the OBJECT tag, I have to walk up the frame tree to find the
first non-transparent parent and use his background color.

That last patch should fix this problem on all platforms that use child windows.
Since Mac doesn't....I still need to fix it up.

Comment 19

[av (gone)]

Can scx->GetStyleData fail or return null |color|? If it can there should be 
some sort of check for the condition. Also, in such a case it would be better to 
have |ResolveBackgroundColor()| return nsresult and pass the actual color via an 
argument.

Comment 20

[Johnny Stenback (:jst)]

The changes to nsHTMLSharedLeafElement.cpp should be specific to embed, no? If
so then, again, you need to check that mNodeInfo->Equals(nsHTMLAtoms::embed)
before doing embed specific things.

Comment 21

[Peter Lubczynski]

Attachment: better patch

Comment 22

[Peter Lubczynski]

It turns out the XP code does correctly set the background color of the widget
on Mac! That last patch works well, please review.

Sticking points:
* Changes in nsHTMLSharedLeafElement.cpp are isolated to embed tags. The mapping
in |EmbedMapAttributesIntoRule()| should only be called by embed tags. The
impact check is also inside a |mNodeInfo->Equals(nsHTMLAtoms::embed|.
* I couldn't find any code that checks the return of |GetStyleData| before using
it and I don't feel like starting a trend.

Comment 23

[av (gone)]

Comment on attachment 56720 [details] [diff] [review]
better patch

This patch works neatly. r=av

Comment 24

[Johnny Stenback (:jst)]

Hmm, after thinking about this some more, why do we need the changes in the
content code? The attributes "background" and "bgcolor" mean nothing on embed,
applet or object, at least not according to the spec. Are those changes for
compatibility issues?

Comment 25

[Peter Lubczynski]

Jonny is right. The spec doesn't say anything about bgcolor or background
attributes on these tags even if some authors use them. New patch without
mappings coming up...

Comment 26

[Peter Lubczynski]

Attachment: new patch

Comment 27

[Johnny Stenback (:jst)]

Someone more familiar with layout-land should sr this one, for what it's woth,
it looks good to me.

Comment 28

[Marc Attinasi]

Comment on attachment 56790 [details] [diff] [review]
new patch

fix the indentation on the 'else' and 'break' and sr=attinasi

Comment 29

[av (gone)]

Comment on attachment 56790 [details] [diff] [review]
new patch

Tested on Windows, r=av

Comment 30

[Peter Lubczynski]

patch checked in, marking FIXED.

Comment 31

[Christine Hoffman]

Tested using 11_07_09 trunk build on Win2000 - verified fixed. Will verify cross
platform tomorrow and mark bug.

Comment 32

[Christine Hoffman]

Tested on:
MacOS9: 11_08_04_trunk
MacOS10: 11_08_08_trunk

This bug is not fixed on MacOS9 or MacOS10. At initial painting, you still get
the original gray background. If you resize the page, then the background color
is white. Reload again and the color is back to its initial gray. 

Comment 33

[Peter Lubczynski]

Ok, still doesn't work on the Mac. fix on it's way.....

Comment 34

[Peter Lubczynski]

Attachment: mac patch

Can I get some reviews on this new patch? Thanks.

Comment 35

[Simon Fraser [no longer active]]

Comment on attachment 57122 [details] [diff] [review]
mac patch

Fix the confusing comments:

+	// return (color8 == 0xFF ? 0xFFFF : (color8 << 8));
+	return (color8 << 8) | color8;	/* (color8 * 257) == (color8 * 0x0101)
*/
and sr=sfraser

Comment 36

[Asa Dotzler [:asa]]

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

Comment 37

[av (gone)]

Comment on attachment 57122 [details] [diff] [review]
mac patch

r=av

Comment 38

[Peter Lubczynski]

Patch should be in today's builds. Marking FIXED.

Comment 39

[Chris Petersen]

Verifying this problem has been resolved in OS 9 and OS X trunk builds
(2001-11-09-08).

Comment 40

[Christine Hoffman]

marked verified

Comment 41

[Christopher Blizzard (:blizzard)]

Did this get checked into the 0.9.6 branch?

Comment 42

[Chris Petersen]

Confirming issue is resolved in the 2001-11-14-11 0.9.6 build. Tested under
Windows ME.

Comment 43

[Judson Valeski]

minusing. thanks for raising the issue.