Chris Petersen Reporter
	Description • 23 years ago

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.

	Peter Lubczynski Assignee
	Comment 2 • 23 years ago

-->peterl

	Peter Lubczynski Assignee
	Comment 3 • 23 years ago

nominating mozilla0.9.2

	Peter Lubczynski Assignee
	Comment 4 • 23 years ago

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

	karnaze (gone)
	Comment 5 • 23 years ago

Moving to m0.9.3

	Peter Lubczynski Assignee
	Comment 6 • 23 years ago

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

	Blake Ross
	Comment 7 • 23 years ago

Missed 0.9.3.

	karnaze (gone)
	Comment 8 • 23 years ago

reassigning to m0.9.6

	Peter Lubczynski Assignee
	Comment 9 • 23 years ago

my bug

	av (gone)
	Comment 12 • 23 years ago

Peter, do you this problem on windows?

	av (gone)
	Comment 14 • 23 years ago

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.

	Peter Lubczynski Assignee
	Comment 15 • 23 years ago

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.

	Johnny Stenback (:jst)
	Comment 16 • 23 years ago

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

	Peter Lubczynski Assignee
	Comment 18 • 23 years ago

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.

	av (gone)
	Comment 19 • 23 years ago

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.

	Johnny Stenback (:jst)
	Comment 20 • 23 years ago

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.

	Peter Lubczynski Assignee
	Comment 22 • 23 years ago

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.

	av (gone)
	Comment 23 • 23 years ago

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

This patch works neatly. r=av

	Johnny Stenback (:jst)
	Comment 24 • 23 years ago

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?

	Peter Lubczynski Assignee
	Comment 25 • 23 years ago

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

	Johnny Stenback (:jst)
	Comment 27 • 23 years ago

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

	Marc Attinasi
	Comment 28 • 23 years ago

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

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

	av (gone)
	Comment 29 • 23 years ago

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

Tested on Windows, r=av

	Peter Lubczynski Assignee
	Comment 30 • 23 years ago

patch checked in, marking FIXED.

	Christine Hoffman
	Comment 31 • 23 years ago

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

	Christine Hoffman
	Comment 32 • 23 years ago

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. 

	Peter Lubczynski Assignee
	Comment 33 • 23 years ago

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

	Peter Lubczynski Assignee
	Comment 34 • 23 years ago

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

	Simon Fraser [no longer active]
	Comment 35 • 23 years ago

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

	Asa Dotzler [:asa]
	Comment 36 • 23 years ago

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

	av (gone)
	Comment 37 • 23 years ago

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

r=av

	Peter Lubczynski Assignee
	Comment 38 • 23 years ago

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

	Chris Petersen Reporter
	Comment 39 • 23 years ago

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

	Christine Hoffman
	Comment 40 • 23 years ago

marked verified

	Christopher Blizzard (:blizzard)
	Comment 41 • 23 years ago

Did this get checked into the 0.9.6 branch?

	Chris Petersen Reporter
	Comment 42 • 23 years ago

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

	Judson Valeski
	Comment 43 • 23 years ago

minusing. thanks for raising the issue.