430878 - Print-selection doesn't print lower "overflow:scroll" divs unless their upper mirror-image is selected.

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Daniel Holbert [:dholbert]]

In the testcase I'm about to attach, print-selection shows the following strange properties:

A. Selected text is *not* moved to the top of the page, as it should be. 
  (e.g. if I print-selection with just "3" selected, the "3" isn't moved to the top of the page -- it's instead on the 3rd line, at the same place it'd be on a full printout of the page.)

B. The lower lines (lines 5-7) are never printed in print-selection, *UNLESS* their upper mirror-image lines are also selected.
  e.g. line 5 only prints if I've selected lines 5 *and* 3.
  e.g. line 6  only prints if I've selected lines 6 *and* 2
  e.g. if I select lines 3-7, only lines 3-5 are printed. (lines 6 and 7 are missing unless I also select lines 2 and 1)

Both of these behaviors seem to be due to the "overflow" CSS property being set.

Note: You may need to hold the Ctrl key in order to select multiple lines on this testcase.

(This testcase was minimized from reddit.com -- currently, I can print-selection if I select a chunk of the first article title there, but not if I select a chunk from the second or later ones.  I think that's related to this bug.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

This testcase has fewer lines, and some plain (not overflow:hidden) text at the end.  This ending text makes it easier to select multiple lines in FF2 -- just start by selecting some of that ending text, and then drag the cursor upwards.

(multiline-selection doesn't seem possible on testcase 1 in FF2, AFAICT -- it looks like when you start selecting, it only selects within the current overflow-hidden div.)

Anyway -- testcase 2 shows that this is a regression, per this new testcase and the following experiments:

Experiment B:
 1. Click in between "b" and "c", and then move the cursor upwards, so that you've selected "3" and "ab".
 2. Print selection

RESULTS ON FF3: only "abc" is printed, shifted a few lines down.

Experiment C:
 1. Click in between "b" and "c", and then move the cursor upwards, so that you've selected "2", "3", and "ab".
 2. Print selection

RESULTS ON FF3: 2, 3, and "abc" are all printed, but the "3" and the "abc" overlap.  And there's a blank line at the top.

Results on FF2 are what you'd expect -- the selected portion prints, it's aligned with the top of the page, and nothing overlaps.

Comment 3

[Daniel Holbert [:dholbert]]

Ok, so it looks like the main thing going wrong here is that scrollframes' contents aren't being slid to the top of the document, with the rest of the content.

This causes the "upper-mirror-image needs to be selected" symptom in this bug, for this reason:  When we slide the page content frame upwards by mYSelOffset ( = y position of first selection), then the last unslid scrollframe that will still be in the bounds of the shifted page content frame will be the "mirror image" of the first selected item.

So, to fix this, we need to make sure that scrollframes get slid upwards by mYSelOffset, together with the rest of the page.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch v1: call PositionChildViews after shifting page content frame

We need to reposition the scrollframes' views, after the page content frame is shifted.  PositionChildViews does just that. 

See this comment right above its implementation, at
http://mxr.mozilla.org/seamonkey/source/layout/generic/nsContainerFrame.cpp#791

791  * Position the views of |aFrame|'s descendants. A container frame
792  * should call this method if it moves a frame after |Reflow|.

So, we definitely need to be calling this function.

This fixes testcase 1, testcase 2, and reddit.com.  In the experiments described in Comment 0 and Comment 2, the selected text is now visible *and* is shifted correctly to the top of the page, whereas it wasn't before.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 318251 [details] [diff] [review]
patch v1: call PositionChildViews after shifting page content frame

Requesting review.
 * Benefits
   - Fixes print-selection dataloss on major site (reddit)

 * Risk: Minimal
   - Very simple patch
   - only affects print-selection
   - Clearly the right thing to do, per the quoted code-comment in comment 4

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> Requesting review.

Sorry -- I meant "requesting approval" :)

Comment 7

[Damon Sicore (:damons)]

Comment on attachment 318251 [details] [diff] [review]
patch v1: call PositionChildViews after shifting page content frame

a1.9+=damons

Comment 8

[Daniel Holbert [:dholbert]]

Patch v1 checked in.

Checking in layout/generic/nsSimplePageSequence.cpp;
/cvsroot/mozilla/layout/generic/nsSimplePageSequence.cpp,v  <--  nsSimplePageSequence.cpp
new revision: 3.172; previous revision: 3.171
done