Reflect page range settings in Print Preview
Categories
(Core :: Print Preview, task, P1)
Tracking
()
People
(Reporter: emmamalysz, Assigned: dholbert)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [print2020_v81])
Attachments
(4 files)
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
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?
Comment 1•4 years ago
|
||
Daniel: Would you be able to look into this?
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
Sure, I'll take a look.
Assignee | ||
Comment 3•4 years ago
|
||
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
Assignee | ||
Comment 4•4 years ago
|
||
Assignee | ||
Comment 5•4 years ago
|
||
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)
Assignee | ||
Comment 6•4 years ago
|
||
(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).
Assignee | ||
Comment 7•4 years ago
|
||
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.)
Assignee | ||
Comment 8•4 years ago
|
||
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.
Assignee | ||
Comment 9•4 years ago
|
||
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
Assignee | ||
Comment 10•4 years ago
|
||
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
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 12•4 years ago
|
||
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.)
Comment 13•4 years ago
|
||
Assignee | ||
Comment 14•4 years ago
|
||
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)
Comment 15•4 years ago
|
||
Comment 16•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/62b5adea43de
https://hg.mozilla.org/mozilla-central/rev/fd0c31c45692
https://hg.mozilla.org/mozilla-central/rev/0f469ac03a00
https://hg.mozilla.org/mozilla-central/rev/7c6a345fec07
Assignee | ||
Comment 17•4 years ago
|
||
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
- View some multi-page document (e.g. https://www.mozilla.org/en-US/MPL/2.0/ )
- Ctrl+P to bring up print dialog
- Click the dropdown under "Pages", and choose "Custom" (instead of "All")
- 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
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 18•4 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/8da63abd0d6a
https://hg.mozilla.org/releases/mozilla-beta/rev/da99718062a1
https://hg.mozilla.org/releases/mozilla-beta/rev/1067272c0124
https://hg.mozilla.org/releases/mozilla-beta/rev/fafd83027b7c
Comment 19•4 years ago
|
||
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.
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 20•4 years ago
|
||
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.
Description
•