191938 - Rework how "don't print backgrounds" works / print preview scrollbars look strange/corrupted/invisible/missing/black

Status: VERIFIED FIXED

(Core :: Printing: Output, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Currently, the "don't print backgrounds" pref is implemented at paint time. 
This means we have to propagate these booleans around while painting, do various
checks in CanPaintBackground and PaintBackground, etc.

This has a couple of issues.  One is that if done "right" the testcase

<div style="position:absolute; top:0">
aaaaaaaaaaaa
</div>
<div style="position:absolute; top:0; background: red">
bbbbbbb
</div>

Shows both sets of text.

Perhaps instead of not painting the background we should just paint it white
when a background is set?

Perhaps the implemention of this pref should be at style resolution time, not
paint time?

So 

1)  We should decide what this pref should really do
2)  We should decide how best to implement it.

Thoughts?

Comment 1

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

I think we already do something like this if the user selects their own
background color.

I don't like it much, though. It's easy to mangle the display with blocks that
were intended to be transparent.

How about this: when background suppression for printing is selected, we treat
every solid non-transparent background as white? Everything else is unaltered.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, ok.  So we take out the changes to CanPaintBackground() and just reset the
background color to white at paint time?

(Doing it during style resolution is hard, since we can't do it to scrollbars. 
Yay having the print preview scrollbars be part of the _printing_ prescontext.)

Comment 3

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

This would just be a small hack in nsCSSRendering::PaintBackground, would it not?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, given all the code we already have slinging around our Paint() methods to
deal with this....

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm... So it's not that small a hack due to the oodles of background painting
methods we have and I will not be able to deal with this in the foreseeable
future.. Can someone else take this?

Note that even with this change the XUL frame changes from bug 191875 need to go
in anyway.

Comment 6

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

Attachment: suggested patch

I'm not sure what problems you're worried about. This patch seems to work well.
It ignores the "honour printing status" flag. It fixes the scrollbars. It horks
buttons, but we could fix that by reinstating the "honour printing status" flag
just for buttons.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I wasn't worried; just had/have no time to track down all the background
painting functions.

Horking buttons is not good.  ;)

Why does this fix the scrollbars?  Is that because of -moz-appearance? 
Shouldn't their backgrounds go white with this patch?

Comment 8

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

We only have 3 functions for painting backgrounds that I know of. I thought you
meant there were more hiding somewhere :-).

Yeah, I'll fix the button stuff.

Comment 9

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

Attachment: better fix

Here you go. Buttons look good. Do you want to review this?

Comment 10

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

Taking

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113761 [details] [diff] [review]
better fix

>+  nscolor color = aColor.mBackgroundColor;
>+  if (!aCanPaintNonWhite) {
>+    color = NS_RGB(255, 255, 255);
>+  }
>   aRenderingContext.SetColor(aColor.mBackgroundColor);

SetColor(color)?

I'm still not quite sure why this fixes scrollbars, since those should be
following the print setting.... again, maybe it's because of moz-appearance.  I
assume you tested this in both modern and classic?

Comment 12

[fantasai]

Do you want to remove the aDX aDY arguments while you're at it? IIRC, they're
not used anywhere and they're always passed in as zero.

Comment 13

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

Attachment: better patch

-- Fixed scrollbars on Modern. Now all XUL boxes display their backgrounds no
matter what (except for tree bodies, but that's OK).
-- Removed aDX, aDY as fantasai suggested.
-- I also noticed a bug where background-attachment:fixed wasn't working right
in print preview (background attached to the viewport, not the page). So I
fixed that too :-).

Comment 14

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

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

>+  /** ---------------------------------------------------
>+   *  Giving a child frame it searches "up" the tree until it

"Given".  I know it's just copy-paste, but....

And "up" should not be quoted, should it?

The rest of this looks good.... I'm a little leery of landing the "cleanup"
parts of that in 1.3b, though.....

Comment 16

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

Come on. What could possibly go wrong? :-)

Comment 17

[Boris Zbarsky [:bzbarsky]]

That's what I thought in bug 191574 too.  ;)

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[José Jeria]

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

Comment 20

[José Jeria]

This is the bug that shows black scrollbars and elements with black background
in Print Preview, right? (bug 191875 was duped to this one). 
Then maybe the summary should be changed to avoid dupes now that 1.3b has been
released.

Comment 21

[Andreas Kunz]

Changing the summary to avoid dupes. Hope I covered most words... :-)

This bug is no regression, the other one clearly is. So I wonder a bit how they
can be *exact* duplicates (as opposed to depending on each other as they were
previously marked). But I guess it doesn't matter once it is fixed...

Comment 22

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

bz, care to give r= too?

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[José Jeria]

Has this been checked in? Because I dont see any black scrollbars anymore.

Comment 25

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

It has not been checked in AFAIK.

Comment 26

[Andreas Kunz]

...and the scrollbars are still black here (2003021204 Win2k). Not sure what
you're seeing, José...

Comment 27

[José Jeria]

Attachment: Screenshot of no black scrollbars

Feels liks i'm spamming (sorry), but this screenshot shows on win2k that bug is
gone (well, at least the bug i was following about the black scrollbars that
was duped to this one).

Comment 28

[Andreas Kunz]

Have you enabled "Print Background" in the "Page Setup" in Print Preview?
That lets the bug disappear, see bug 191875 comment #18.

Comment 29

[José Jeria]

That was it..

Comment 30

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

Jose, how come your print preview toolbar has all those features? My print
preview toolbar is missing the "Print" and "Page Setup" buttons, and also the
"Scale", "Portrait" and "Landscape" options.

Comment 31

[José Jeria]

? those buttons have been there for ages. At least on windows 2000/xp

Comment 32

[Roland Mainz]

Robert O'Callahan wrote:
> Jose, how come your print preview toolbar has all those features? My print
> preview toolbar is missing the "Print" and "Page Setup" buttons, and also the
> "Scale", "Portrait" and "Landscape" options

On Unix/Linux this functionality is only available if the underlying gfx code
supports multiple independant device context instances - the PostScript module
code does not support that (only the Xprint and QPrinter modules support that).

Comment 33

[Asa Dotzler [:asa]]

This doesn't block the 1.3 release.

Comment 34

[Michael Lefevre]

apologies for more spammage, but has it been decided that this isn't going to go
into 1.3 (shipping a release with a visibly screwed up print preview seems like
a strange decision)? or is it just that nobody has requested approval yet? thanks

Comment 35

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

The former.

Comment 36

[Andreas Kunz]

If you're willing to ship black print preview scrollbars with 1.3 final, would
it make sense to put it in the release notes?

Comment 37

[José Jeria]

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

Comment 38

[R.K.Aa.]

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

Comment 39

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

Fix checked in.

Comment 40

[Bill Mason]

Verified in 2003022304 PC/WinXP

Comment 41

[Olivier Cahagne]

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

Comment 42

[Andreas Kunz]

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

Comment 43

[Boris Zbarsky [:bzbarsky]]

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

Comment 44

[José Jeria]

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

Comment 45

[José Jeria]

Is this not checked into 1.3 because of regressions risk? I find it a bit odd
that its ok to release 1.3 with this nasty bug in it.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Yes, regression risk.  See comment 15 through comment 17.

Comment 47

[Boris Zbarsky [:bzbarsky]]

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

Comment 48

[José Jeria]

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

Comment 49

[R.K.Aa.]

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

Comment 50

[Michael Lefevre]

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

Comment 51

[Rick DeBay]

Can the non-cleanup portion of the fix make it in 1.3?

Comment 52

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

Fix checked into 1.3 branch.