Closed Bug 191938 Opened 22 years ago Closed 22 years ago

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

Categories

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

Product:
Core
Component:
Printing: Output
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.4beta

People

(Reporter: bzbarsky, Assigned: roc)

References

Details

(Whiteboard: [fix])

Attachments

(2 files, 2 obsolete files)

better patch
35.48 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Screenshot of no black scrollbars
11.47 KB, image/gif
Details 
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?
Priority: -- → P2
Target Milestone: --- → mozilla1.4beta
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.
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.)
This would just be a small hack in nsCSSRendering::PaintBackground, would it not?
Yeah, given all the code we already have slinging around our Paint() methods to
deal with this....
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.
Blocks: 191875
Attached patch suggested patch (obsolete) — — Splinter Review 
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.
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?
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.
Attached patch better fix (obsolete) — — Splinter Review 
Here you go. Buttons look good. Do you want to review this?
Attachment #113724 - Attachment is obsolete: true
Taking
Assignee: bzbarsky → roc+moz
Whiteboard: [fix]
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?
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.
Attached patch better patch — — Splinter Review 
-- 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 :-).
Attachment #113761 - Attachment is obsolete: true
*** Bug 191875 has been marked as a duplicate of this bug. ***
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.....
Attachment #113860 - Flags: superreview+
Come on. What could possibly go wrong? :-)
That's what I thought in bug 191574 too.  ;)
Flags: blocking1.3?
OS: Linux → All
*** Bug 192664 has been marked as a duplicate of this bug. ***
*** Bug 192738 has been marked as a duplicate of this bug. ***
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.
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...
Summary: Rework how "don't print backgrounds" works → Rework how "don't print backgrounds" works / print preview scrollbars look strange/corrupted/invisible/missing
bz, care to give r= too?
Attachment #113860 - Flags: review+
*** Bug 192866 has been marked as a duplicate of this bug. ***
Has this been checked in? Because I dont see any black scrollbars anymore.
It has not been checked in AFAIK.
...and the scrollbars are still black here (2003021204 Win2k). Not sure what
you're seeing, José...
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).
Have you enabled "Print Background" in the "Page Setup" in Print Preview?
That lets the bug disappear, see bug 191875 comment #18.
That was it..
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.
? those buttons have been there for ages. At least on windows 2000/xp
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).
Blocks: 192938
This doesn't block the 1.3 release.
Flags: blocking1.3? → blocking1.3-
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
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?
*** Bug 193691 has been marked as a duplicate of this bug. ***
*** Bug 193860 has been marked as a duplicate of this bug. ***
Fix checked in.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Verified in 2003022304 PC/WinXP
Status: RESOLVED → VERIFIED
*** Bug 195527 has been marked as a duplicate of this bug. ***
*** Bug 195489 has been marked as a duplicate of this bug. ***
*** Bug 192938 has been marked as a duplicate of this bug. ***
*** Bug 196168 has been marked as a duplicate of this bug. ***
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.
Yes, regression risk.  See comment 15 through comment 17.
*** Bug 196196 has been marked as a duplicate of this bug. ***
*** Bug 196321 has been marked as a duplicate of this bug. ***
Summary: Rework how "don't print backgrounds" works / print preview scrollbars look strange/corrupted/invisible/missing → Rework how "don't print backgrounds" works / print preview scrollbars look strange/corrupted/invisible/missing/black
*** Bug 196613 has been marked as a duplicate of this bug. ***
*** Bug 196908 has been marked as a duplicate of this bug. ***
Can the non-cleanup portion of the fix make it in 1.3?
Fix checked into 1.3 branch.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: