Closed
Bug 430748
Opened 17 years ago
Closed 17 years ago
Print-selection places text too low, with initial whitespace and select-all
Categories
(Core :: Printing: Output, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dholbert, Assigned: dholbert)
Details
(Keywords: testcase)
Attachments
(4 files, 3 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
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 | ||
Updated•17 years ago
|
Assignee: nobody → dholbert
Assignee | ||
Comment 1•17 years ago
|
||
(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.
Assignee | ||
Comment 2•17 years ago
|
||
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.)
Assignee | ||
Comment 3•17 years ago
|
||
+ 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
Assignee | ||
Comment 4•17 years ago
|
||
(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
Assignee | ||
Comment 5•17 years ago
|
||
(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)
Assignee | ||
Updated•17 years ago
|
Attachment #317763 -
Attachment is obsolete: true
Assignee | ||
Comment 6•17 years ago
|
||
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+
Assignee | ||
Comment 8•17 years ago
|
||
(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?
Assignee | ||
Updated•17 years ago
|
Attachment #318278 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Flags: wanted1.9.0.x?
Assignee | ||
Updated•17 years ago
|
Flags: wanted1.9.0.x?
Comment 9•17 years ago
|
||
Comment on attachment 318301 [details] [diff] [review]
patch v3a (addresses review comment)
a1.9=beltzner
Attachment #318301 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 10•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Comment 11•17 years ago
|
||
https://litmus.mozilla.org/show_test.cgi?id=5306 added to Litmus
Flags: in-litmus+
You need to log in
before you can comment on or make changes to this bug.
Description
•