Closed Bug 430748 Opened 16 years ago Closed 16 years ago

Print-selection places text too low, with initial whitespace and select-all

Categories

(Core :: Printing: Output, defect)

Product:
Core
Component:
Printing: Output
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

Details

(Keywords: testcase)

Attachments

(4 files, 3 obsolete files)

testcase 1
182 bytes, text/html
Details
reference 1
182 bytes, text/html
Details
testcase 2 (using negative margin)
161 bytes, text/html
Details
patch v3a (addresses review comment)
3.53 KB, patch
beltzner
: approval1.9+
Details | Diff | Splinter Review
Attached file testcase 1 — 
Testing with my Ubuntu PDF Psuedu-printer.

Steps to reproduce:
 1. Load testcase
 2. Select-all with Ctrl-A
 3. Print testcase normally
     - We'll call this printed output NORMAL_PRINT

 4. Highlight some or all of the text, *using the mouse*.
 5. Print selection
     - We'll call this printed output HIGHLIGHT_PRINT

 6. Select all, *using Ctrl-A*
 7. Print selection
     - We'll call this printed output SELECT_ALL_OUTPUT


Now, when I compare these outputs, SELECT_ALL_OUTPUT has the text shifted lower than in the other two.  That's broken.

The broken output only happens when I include the initial whitespace between <body> and <div> in the testcase.
Assignee: nobody → dholbert
Attached file reference 1 — 
(In reply to comment #0)
> The broken output only happens when I include the initial whitespace between
> <body> and <div> in the testcase.

To demonstrate this, here's a reference case without that initial whitespace.
If I perform the steps from Comment 0 on this reference case, all three of the printed outputs match the testcase's NORMAL_PRINT exactly.

So, the odd one out is still testcase 1's SELECT_ALL_OUTPUT.
Attached patch patch v1 (obsolete) — — Splinter Review 
This patch fixes testcase 1, along with fixing the text-overlap issues in testcases 3 and 4 on bug 317627.

Basically, what's happening is this:  The initial whitespace gets a frame with height, but it's got a negative y-position, so it normally has no effect on printouts.

However, when we select it via Select-All, we end up with a selection that starts at a negative y-position.  Later on, when we try to slide our selection to the top on the printed page (here: http://tinyurl.com/6g2rxt), we end up actually shifting the content *downwards*, because of its negative y-position.

(Note: in testcases 3 and 4 on bug 317627, it looks like this undesirable downward shift doesn't affect scrollframes' contents, which is why we ended up with overlapping text there)

This patch just clamps selections so that they can't start at negative y-positions.  This effectively prevents us from shifting content downwards to show "hidden" content that's above the 0-position on the first printed page, which has been selected via Select-All.

I'm pretty sure this patched behavior would be desired & safe.  I'm not aware of any situations in which there could be "hidden" content that's before the first page (i.e. invisible) in normal printouts, but which we'd like users to be able to print specifically via print-selection. (But I'll experiment a bit to see if I can come up with any.)
Attached patch patch v2 (obsolete) — — Splinter Review 
+ endRect.y = PR_MAX(0, startRect.y); // xxxdholbert unneeded?

Oops -- I meant PR_MAX(0, endRect.y) here.  This new patch fixes that.

(Though, as the comment indicates, I'm not sure this line is necessary at at all... I don't think it'd be possible for the user's entire selection to be in negative territory.)
Attachment #317761 - Attachment is obsolete: true
(In reply to comment #3)
> (Though, as the comment indicates, I'm not sure this line is necessary at at
> all... I don't think it'd be possible for the user's entire selection to be in
> negative territory.)

This testcase shows that it definitely *is* possible to have a negative y-position on the endRect.  (If I Select-All here and print-selection, I get startRect.y = endRect.y = -480)  So, it's good that the patch clips startRect *and* endRect.  (Note: To be totally correct, I should adjust the heights as well in the situations where I clip the y-values -- that'll come in my next version of the patch)

Currently, because of this bug, Trunk slides testcase 2 down so that the word "text" is aligned as if there were zero margin.  However, the chopped-off part of the text is still missing.  So our current behavior doesn't buy us any ability to show hidden text via select-all.

Hence, I'm pretty confident that the situation described in this part of comment 2 isn't something we're worried about:
> I'm not aware
> of any situations in which there could be "hidden" content that's before the
> first page (i.e. invisible) in normal printouts, but which we'd like users to
> be able to print specifically via print-selection
Attached patch patch v3 (obsolete) — — Splinter Review 
(In reply to comment #4)
> (Note: To be totally correct, I should adjust the heights as
> well in the situations where I clip the y-values -- that'll come in my next
> version of the patch)

This patch adjusts the heights, whenever it adjusts y-values, to subtract off the clipped-out height.  However, for sanity, it won't let heights go below zero. (which they otherwise would, in a testcase like testcase 2 but with a huge negative margin, which would give us a huge startRect.y and endRect.y)
Attachment #317763 - Attachment is obsolete: true
Comment on attachment 318278 [details] [diff] [review]
patch v3

This patch fixes* both testcase 1 and testcase 2.

*By "fix", I mean it makes select-all+print-selection match normal printed output, rather than being shifted vertically, as in current trunk.
Attachment #318278 - Flags: superreview?(roc)
Attachment #318278 - Flags: review?(roc)
Comment on attachment 318278 [details] [diff] [review]
patch v3

+                  startRect.height = PR_MAX(0, startRect.height + startRect.y);

Use startRect.YMost()/endRect.YMost().
Attachment #318278 - Flags: superreview?(roc)
Attachment #318278 - Flags: superreview+
Attachment #318278 - Flags: review?(roc)
Attachment #318278 - Flags: review+
(In reply to comment #7)
> Use startRect.YMost()/endRect.YMost().

Done, with comments updated to reflect the change.  (also added a newline for clarity)

Requesting approval for checkin.  Justification below

 * Benefit:
   - Makes our print-selection behavior incrementally more sane, better matching what users would expect

 * Risks: Minimal
   - Affects print-selection only
   - Simple patch in well-understood code.
Attachment #318301 - Flags: approval1.9?
Attachment #318278 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x?
Comment on attachment 318301 [details] [diff] [review]
patch v3a (addresses review comment)

a1.9=beltzner
Attachment #318301 - Flags: approval1.9? → approval1.9+
Patch v3a checked in.

Checking in nsPrintEngine.cpp;
/cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v  <--  nsPrintEngine.cpp
new revision: 1.182; previous revision: 1.181
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: