1419739 - Make printing a selection not terrible.

Status: RESOLVED FIXED

(Core :: Printing: Output, enhancement, P1)

Description

[Bob Owen (:bobowen)]

Currently, we print a selection by:
* reflowing the whole document into one infinitely long page
* position at the top of the selection to print the first page
* move the position down by a page length each time to print subsequent pages

This has several shortcomings:
* the way we find the top of the first selection sometimes doesn't work:
** in some cases the top of the containing node (possibly the body itself) is flagged as selected and so we always print from the top, which can mean you get a number of blank pages if the selection starts further down
** the calculation of the top of the first frame is sometimes incorrect causing it to lose part of the frame
* if the selection is longer than a page, you often get content printed over the footer (even though it will print again at the top of the next page)
* moving forward by a page length is quite likely not going to match up with a frame top and so you often get content half rendered at the top of pages
* if you have multiple selections that are far apart then you will get blank pages printed between them
* the printing of multiple pages doesn't use the nsPagePrintTimer and so is not properly coordinated with the parent, which also causes an issue for bug 1414834
* the way we paint just selected nodes means the whole of a text node gets printed, instead of just the selection

Apart from that, it's great. :-)
Some, but probably not all of the above could be fixed, but the code is already pretty complicated.

Instead, I'm taking a different approach:
* use the selection in the original document to create an inverted selection in the document cloned for printing, adding an ellipsis when ranges start or end in text nodes
* delete the selected nodes
* print the page as normal

From my testing, this works well and address all of the above problems.

Comment 1

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04380cea430927af2651b474c127f30afdf44922

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 1: Change selection printing to remove unselected nodes and then print as normal

Currently, we print a selection by:
* reflowing the whole document into one infinitely long page
* position at the top of the selection to print the first page
* move the position down by a page length each time to print subsequent pages

This has several shortcomings, detailed in the bug.

This approach usees the original document selection to create an inverted selection in the document cloned for printing, adding an ellipsis when ranges start or end in text nodes.
Then deletes that selection from the document prior to printing.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 2: Clean up resulting (and some existing) unused code

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 8930893 [details] [diff] [review]
Part 1: Change selection printing to remove unselected nodes and then print as normal

Review of attachment 8930893 [details] [diff] [review]:
-----------------------------------------------------------------

While I remember to note this: In the commit comment, s/usees/uses/, and wrap that last paragraph.

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 8930893 [details] [diff] [review]
Part 1: Change selection printing to remove unselected nodes and then print as normal

Review of attachment 8930893 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, and I think this approach to selection printing is better. Nice idea!

For the commit message:

s/Currently, we print a selection/Prior to this change, we would print a selection/

s/approach usees/approach uses/

Join the last sentence "Then deletes that selection from the document prior to printing" with the previous one using a comma, I think. That makes it long, but at least it then doesn't sound grammatically broken, and it's still quite readable.

Also wrap the last paragraph properly, please.

::: layout/printing/nsPrintEngine.cpp
@@ +2483,5 @@
>      origShell->GetCurrentSelection(SelectionType::eNormal);
>    RefPtr<Selection> selection =
>      shell->GetCurrentSelection(SelectionType::eNormal);
>    NS_ENSURE_STATE(origSelection && selection);
> +  

Remove white space on blank line.

@@ +2542,2 @@
>    }
> +  

Trailing white space.

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 8930894 [details] [diff] [review]
Part 2: Clean up resulting (and some existing) unused code

Review of attachment 8930894 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! :)

Can you make the commit message "Clean up resulting (and some existing) unused print code"?

Comment 7

[Bob Owen (:bobowen)]

Whitespace and comment changes made locally.
I'd copied much of that larger comment and obviously bodged the edit, thanks for the corrections.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be9c6cb46e1b
Part 1: Change selection printing to remove unselected nodes and then print as normal. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/49d521bb0307
Part 2: Clean up resulting (and some existing) unused printing code. r=jwatt

Comment 9

[Jonathan Watt [:jwatt]]

One thing that occurred to me - if a <style> element is in the inverted ranges, is it deleted and can that cause problems?

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/be9c6cb46e1b
https://hg.mozilla.org/mozilla-central/rev/49d521bb0307

Comment 11

[Bob Owen (:bobowen)]

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #9)
> One thing that occurred to me - if a <style> element is in the inverted
> ranges, is it deleted and can that cause problems?

After a bit of debugging, it seems that a style element can be in the inverted range and get deleted, but the page still seems to print correctly.
I'm not sure why that is at the moment, I'll have to do a bit more investigation.

Comment 12

[Jonathan Watt [:jwatt]]

(In reply to Bob Owen (:bobowen) from comment #11)

After a bit of debugging, it seems that a style element can be in the
inverted range and get deleted, but the page still seems to print correctly.
I'm not sure why that is at the moment, I'll have to do a bit more
investigation.

Possibly because we don't (in some cases?) flush after the removal? (Haven't checked.)