Closed Bug 1659005 Opened 4 years ago Closed 4 years ago

Reflect page range settings in Print Preview

Categories

(Core :: Print Preview, task, P1)

task

Tracking

()

VERIFIED FIXED
82 Branch
Tracking Status
firefox81 --- verified
firefox82 --- verified

People

(Reporter: emmamalysz, Assigned: dholbert)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [print2020_v81])

Attachments

(4 files)

Changing the page rage settings (startPageRange, endPageRange, and printRange) does not update the print preview.

The front-end patch for updating the page range is being completed in Bug 1653389. With that patch applied, the specified range is printed correctly, which seems to be handled in https://searchfox.org/mozilla-central/rev/358cef5d1a87172f23b15e1a705d6f278db4cdad/layout/generic/nsPageSequenceFrame.cpp#491-497.

Should something similar be added to nsPrintJob?

Flags: needinfo?(svoisen)

Daniel: Would you be able to look into this?

Flags: needinfo?(svoisen) → needinfo?(dholbert)
Summary: Reflect page rage settings in Print Preview → Reflect page range settings in Print Preview

Sure, I'll take a look.

Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: needinfo?(dholbert)

First step here (necessary but not sufficient): we need to remove this chunk, which (for now) explicitly disables print-range support in print preview:
https://searchfox.org/mozilla-central/rev/fa7f47027917a186fb2052dee104cd06c21dd76f/layout/printing/nsPrintJob.cpp#871-874

The just-attached patch is just a demo of an approach for flagging certain pages as hidden from print preview. Right now, the patch is just hardcoded to hide all even pages; it doesn't check the print settings at all. (hence, proof-of-concept)

The strategy here is to just have hidden pages as extra children on a given PrintedSheetFrame (just in the regular child list, so that nsPageFrame sibling relationships / next-in-flow relationships are maintained & sane). And to skip them, we:

  • tag them with a dedicated frame state bit, which lets us know to avoid painting them.
  • allow them to exist as plural children, even though strictly speaking we only support 1 page per sheet (right now). (And once we support N pages per sheet, a given PrintedSheetFrame may have N+1 children if it has a skipped-page child.)

This seems to work in the given WIP patch - print-preview shows us page 1, 3, 5, 7, 9, and 11 at https://www.mozilla.org/en-US/MPL/2.0/. Though it does show those all as "...of 6" which is obviously wrong. :) (i.e. "page 11 of 6") So there's something that we'll need to fix up there -- presumably we're mixing up the sheet count vs. the page count somewhere, and I guess we do need to support a separate "getNumberOfSheets" API, separate from the "getNumberOfPages" API. (where the former would be used to report how much paper will be printed, and the latter would be used to print e.g. "page 11 of 12" on the page footer, regardless of how may pages are skipped & how many sheets are being printed)

(In reply to Daniel Holbert [:dholbert] from comment #5)

This seems to work ... print-preview shows us page 1, 3, 5, 7, 9, and 11 ...
Though it does show those all as "...of 6" which is obviously wrong. :) [...]
I guess we do need to support a separate "getNumberOfSheets"
separate from the "getNumberOfPages" API

Actually, this string comes from an internal representation that we can just fix. Probably/hopefully we won't don't need these separate page-vs-sheet-count APIs after all (and the existing sheet-count API serves our purposes).

Depends on: 1660166

Bug 1660166 addressed the issue quoted/discussed in comment 6, BTW.

One other issue I'm noticing: we do in fact need to expose a "numPages" API that truly returns the number of (virtual) pages, so that we can accurately range-check the pages in the "From/To" fields.

(Up until now, it wasn't clear that we needed to actually report the number of pages -- all of the other usages so far seem to really want the number of sheets. But for this case, we do need the number of pages.)

Blocks: 1631440

This patch shouldn't change behavior at all; it's just moving some member
variables to a new home on a helper-struct (and the struct's lifetime is the
same as the lifetime of the nsPageSequenceFrame where these member variables
lived, prior to this patch).

These members need to move so that PrintedSheetFrame can have access to them.
PrintedSheetFrame is now where pages are generated, and it will handle our
page-range-induced page skipping, as of a later patch in this series.

This patch shouldn't change behavior.

After this change, we'll be able to reason about the page range during reflow
(in a later patch in this series). The old place where we determine the
page-range information -- in nsPageSequenceFrame::StartPrint -- unfortunately
runs after reflow. So that was running too late for the information to be
useful when we're laying out pages on sheets.

Depends on D88468

This patch shouldn't change behavior at all.

Since our print reflow pipeline doesn't try to handle incremental reflow (per
the early return in nsPageSequence::Reflow), we can safely assume that
our page frames don't have a preexisting next-in-flow (until we explicitly
create one for them).

This simplifies the logic & the number of scenarios that we need to consider.

Depends on D88469

Attachment #9170572 - Attachment description: Bug 1659005 WIP patch / proof-of-concept → Bug 1659005 part 4: [WIP] Check for page-range-skipped pages during reflow, and group them as unpainted additional children on the nearest PrintedSheetFrame. r?
Attachment #9170572 - Attachment description: Bug 1659005 part 4: [WIP] Check for page-range-skipped pages during reflow, and group them as unpainted additional children on the nearest PrintedSheetFrame. r? → Bug 1659005 part 4: Check for page-range-skipped pages for tab-modal print preview, and group them as unpainted additional children on the nearest PrintedSheetFrame. r?TYLin

Note: I've triggered lando to get parts 1-3 landed. (They're non-behavior-changing patches that pave the way for the main patch, part 4.)

Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/62b5adea43de part 1: Move nsPageSequenceFrame's page-range-specific members into nsSharedPrintData. r=TYLin https://hg.mozilla.org/integration/autoland/rev/fd0c31c45692 part 2: Make nsPageSequenceFrame determine its page range information earlier on, at the start of reflow. r=TYLin https://hg.mozilla.org/integration/autoland/rev/0f469ac03a00 part 3: Remove checks for a hypothetical pre-existing pageFrame next-in-flow (which can't happen, in practice). r=TYLin

ni=me to remember to request uplift (planning on doing so on Monday, so that this has at least a day or so of nightly sanity-checking)

Flags: needinfo?(dholbert)
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7c6a345fec07 part 4: Check for page-range-skipped pages for tab-modal print preview, and group them as unpainted additional children on the nearest PrintedSheetFrame. r=TYLin
Blocks: 1661868

Comment on attachment 9170572 [details]
Bug 1659005 part 4: Check for page-range-skipped pages for tab-modal print preview, and group them as unpainted additional children on the nearest PrintedSheetFrame. r?TYLin

Beta/Release Uplift Approval Request

  • User impact if declined: The user's selected printed-page-range (e.g. "from: 3 / to: 4") won't be reflected in the print preview rendering that's shown as part of the new tab-modal print dialog
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: 1. Ensure that about:config pref "print.tab_modal.enabled" is enabled
  1. View some multi-page document (e.g. https://www.mozilla.org/en-US/MPL/2.0/ )
  2. Ctrl+P to bring up print dialog
  3. Click the dropdown under "Pages", and choose "Custom" (instead of "All")
  4. Choose some custom start/end pages, and ensure that the Print Preview rendering is updated.
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): I'd say this is somewhere between low-risk and medium-risk; calling it "medium" to be conservative.
  • It's higher than "low" because more than a few lines are changing, and we don't have great automated tests for this sort of stuff.
  • But I've done manual testing and it seems to work with all the scenarios I've come up with. (Note that I found one closely-related issue, bug 1661852 -- but that bug is independent of this change.)
  • Also: the patch stack is designed to only impact the print-preview rendering; there's an existing legacy implementation that handles page ranges for the actual final printed output. I'm intentionally not touching/replacing that implementation at this point, to avoid breaking something that works.
  • And the implementation is relatively straightforward; it simply tags out-of-range pages, and then prevents tagged pages from drawing.
  • String changes made/needed: None
Flags: needinfo?(dholbert)
Attachment #9170572 - Flags: approval-mozilla-beta?
Attachment #9172484 - Flags: approval-mozilla-beta?
Attachment #9172485 - Flags: approval-mozilla-beta?
Attachment #9172486 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-regression-triage]
QA Whiteboard: [qa-regression-triage] → [qa-triaged]

Comment on attachment 9170572 [details]
Bug 1659005 part 4: Check for page-range-skipped pages for tab-modal print preview, and group them as unpainted additional children on the nearest PrintedSheetFrame. r?TYLin

Approved for 81.0b5.

Attachment #9170572 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9172484 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9172485 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9172486 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

This is verified fixed using Firefox 82.0a1 (BuildId:20200904094341) and Firefox 81.0b6 (BuildId:20200903205131) on Windows 10 64bit, macOS 10.14 & Ubuntu 18.04 64bit.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1668400
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: