294836 - When printing, bgcolor of one tag incorrectly applied to a whole page

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Marius Strumyla]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050513 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050513 Firefox/1.0+

Consider this snippet of html:
Normal text <B style="background-color: #ffff66">yellow bground</B>

When Printing/Print Previewing with "Print Background (Colors & Images)", the
yellow background is applied to a whole page. The yellow background should only
be applied to text "yellow bground".

Reproducible: Always

Steps to Reproduce:
1. Load URL
2. Check "Print Background (Colors & Images)"
3. Do Print Preview

Actual Results:  
==> The whole page has a yellow background

Expected Results:  
==> The page has a white background. Text "SUNWapchr" has a yellow one.

SPAN, DIV tags give the same outcome. The URL comes from a bug 272309.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase: XHTML body background

Comment 2

[Mats Palmgren (inactive)]

This code is to eager to find a background color:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSRendering.cpp&rev=3.264&root=/cvsroot&mark=2586-2598#2570
(it finds the <B> in the URL)

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Find the first frame with content, then do what we do in the non-print case.

This patch changes the behaviour for the attached XHTML testcase so it does not

propagate the pink color from BODY over the whole page, I hope I got that
right?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Why is just boring straight down the right thing?  Under what conditions would
we want to take more than one more step down from pageContentFrame anyway?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

under the condition that we reorganize the frame hierarchy? :-)

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 7

[Boris Zbarsky [:bzbarsky]]

Perhaps we should also assert that the child of the page content frame has a
content node?

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 193780 [details] [diff] [review]
Patch rev. 3

Looks reasonable!

Comment 10

[Mats Palmgren (inactive)]

Checked in to trunk 2005-08-28 21:39 PDT

-> FIXED

Comment 11

[Mats Palmgren (inactive)]

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

Comment 12

[Mats Palmgren (inactive)]

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

Comment 13

[Mats Palmgren (inactive)]

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

Comment 14

[Mats Palmgren (inactive)]

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

Comment 15

[Mats Palmgren (inactive)]

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

Comment 16

[L. H.]

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

Comment 17

[Kenneth Herron]

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

Comment 18

[L. H.]

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

Comment 19

[buchholz]

When will we see the fix?  I still have the problem in Firefox 1.0.7 (Linux, Red Hat Enterprise Linux distribution, build date=2005/09/21).

Comment 20

[buchholz]

Attachment: HTML page triggering the bug

All pages render correctly (or 'as expected') in viewer, but pages 2 and 3 have problems in both "Print Preview" and when printing.

Pages 1 and 4 preview/print correctly; Pages 2 and 3 get flooded w/ red background color.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> When will we see the fix?  I still have the problem in Firefox 1.0.7 

That's a rendering engine from April 2004, with security fixes only.

Not that using even Firefox 1.5 helps much in this case; this was fixed on the trunk after Gecko 1.8 branched, so this fix will most likely not appear in Firefox until Firefox 3 or so.

If you feel that this is important enough to fix on the 1.8 branch, please request approval for that branch on the patch, etc.

Comment 22

[[e-mail]]

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Mike Pinkerton (not reading bugmail)]

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

Comment 25

[Håkan Waara]

Comment on attachment 193780 [details] [diff] [review]
Patch rev. 3

Nominating for 1.8-branch.

I think this bug is ugly enough to be worth considering.

Comment 26

[Dan Bornstein]

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

Comment 27

[Håkan Waara]

This is nominated for approval to 1.8.1-branch, but no response yet (did I nominate it incorrectly?).   

Anyway:  are you guys interested in taking this on the branch? I think it's a quite ugly bug, and quite major for all printing functionality (including Printing to PDF on mac).

Comment 28

[Boris Zbarsky [:bzbarsky]]

> did I nominate it incorrectly?

Yes (and I believe I told you so at the time).

Comment 29

[Marius Strumyla]

There's one more real world page: http://sunsolve.sun.com/search/document.do?assetkey=1-26-102262-1

Click [Printer-Friendly Page] and then select Print Preview.

Comment 30

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 193780 [details] [diff] [review]
Patch rev. 3

Boris: this sounds like something we do want on 1.8.1, can you + it if you agree?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 193780 [details] [diff] [review]
Patch rev. 3

I would need to see an analysis that indicates that this is the right thing for branch (specifically, that the frame structure printing creates is the same there as on trunk in the ways that concern us) before I could OK this for branch.

Comment 32

[Håkan Waara]

CC some folks I've seen in printing land recently.

Would any of you be able to confirm what Boris asks for?

Thanks.

Comment 33

[Eli Friedman]

The way the frame tree is built hasn't changed for a while time.  There's a page frame for every page, which paints the background for the pages.  There's a page content frame for every page frame.  The old code just kept on drilling down into the frame tree from there, getting whatever background it could find.  The corrected code only looks at the child of the page content frame, which is constructed from the root content.  Therefore, the corrected code is gets the right frame.

Potential risk is very low; I would add an early return if the first child of the page content frame is null just to be on the safe side (in addition to the assertion), but other than that, the worst that could happen is that the background is wrong.

One odd thing about the way the frames get constructed is that nsCSSFrameConstructor::ConstructDocElementFrame appears to have a codepath where it creates a scroll frame, but that definitely doesn't execute in paginated contexts; in fact, I don't think it can ever execute!  The time when it did execute might have something to do with the way this code was written.  In any case, I'm filing a bug on that.

Comment 34

[John P Baker]

I see this didn't get approval for 1.8.1 and so is still
outstanding in firefox 2 beta - is this after considering
comment #33?

Comment 35

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

Same as rev.3 with an added early return if there is no child frame.

I dumped the frame tree in trunk and 1.8 and they are the same.

Boris, feel free to approve rev.3 or 4 whichever you prefer.
If rev.4 then I want r+sr on the early return for trunk as well.

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229099 [details] [diff] [review]
Patch rev. 4

Please only nominate only after the patch has been properly reviewed and baked on trunk for at least one build cycle, preferably a couple of days.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 229099 [details] [diff] [review]
Patch rev. 4

OK, looks reasonable.  If the frame structure is the same on branch, I'm happy with this on branch.

Comment 38

[Mats Palmgren (inactive)]

Checked in the additional null check from Patch rev.4 on trunk at
2006-07-20 02:41 PDT.

Comment 39

[Mats Palmgren (inactive)]

Comment on attachment 229099 [details] [diff] [review]
Patch rev. 4

The essential part of this patch (rev.3) have baked on trunk for
almost a year.  Rev.4 only adds a null pointer check, which have
now baked on trunk for one build cycle.

Comment 40

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

Comment on attachment 229099 [details] [diff] [review]
Patch rev. 4

a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and mark fixed1.8.1 once you have done so.

Comment 41

[Mats Palmgren (inactive)]

Checked in to MOZILLA_1_8_BRANCH at 2006-07-21 16:53 PDT.

Comment 42

[Mats Palmgren (inactive)]

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

Comment 43

[Mats Palmgren (inactive)]

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