1660908 - Update page range validation code to use rawNumPages attribute

Status: VERIFIED FIXED

(Toolkit :: Printing, defect, P1)

Description

[Daniel Holbert [:dholbert]]

Once nsIWebBrowserPrint gets a rawNumPages attribute (being added in bug 1660502), we need to update the page-range validation code to use that.

Otherwise we end up in a catch-22 scenario. The existing printPreviewNumPages attribute is really poorly named given the way that this interface is evolving -- nowadays, it is really "how many sheets will be printed", and that's not the right value to use for page range validation.

To see this, here's one scenario where this matters: Suppose e.g. you've got a 12-page document and the user chooses a range "from:6 to:10" (5 pages). Then after we've rendered their chosen selection, we'll suddenly start reporting a printPreviewNumPages of 5, which then will cause the user's selected range to be invalid, since 6 and 10 are both out-of-bounds.

Anyway - there's no better option right now, but once bug 1660502 has landed, we should migrate the range validation code to use the new rawNumPages API.

Comment 1

[Daniel Holbert [:dholbert]]

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

To see this, here's one scenario where this matters: Suppose e.g. you've got a 12-page document and the user chooses a range "from:6 to:10" (5 pages). Then after we've rendered their chosen selection, we'll suddenly start reporting a printPreviewNumPages of 5, which then will cause the user's selected range to be invalid, since 6 and 10 are both out-of-bounds.

To clarify: this^ scenario doesn't actually play out with this sort of bad outcome yet, simply because we don't honor the page range in print preview right now. But it does play out this way in a build with my local patch for bug 1659005 (which reduces the number of pages/sheets shown due to honoring the requested page range).

Comment 2

[Daniel Holbert [:dholbert]]

(which means: I think we need to address this before bug 1659005 can land.)

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1660908 - Use rawNumPages rather than printPreviewNumPages for the total number of pages. r?emalysz,dholbert

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78598fcb79e3
Use rawNumPages rather than printPreviewNumPages for the total number of pages. r=emalysz,dholbert

Comment 5

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/78598fcb79e3

Comment 6

[Jonathan Watt [:jwatt]]

Reminder to request beta approval.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9172046 [details]
Bug 1660908 - Use rawNumPages rather than printPreviewNumPages for the total number of pages. r?emalysz,dholbert

Beta/Release Uplift Approval Request

• User impact if declined: Page range validation when printing will be incorrect when the start of the range is greater than the total number of pages
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Open print dialog for a page with enough content to span multiple pages

2. Select "custom" in the page-range dropdown
3. Enter a start and end page number towards the top of the possible range. E.g. 9-10 on a 10 page document.

ER: With this patch, the range should not be marked invalid, printing can proceed and the "sheet count" accurately sums the number of sheets that will be printed (2 in this example)

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small front-end change to which page count total we use when setting min/max in the print UI.
• String changes made/needed: NOne.

Comment 8

[Emil Ghitta, QA [:emilghitta]]

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

Leaving a ni? on myself and the qe-verify+ until this gets uplifted & verified in beta as well.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9172046 [details]
Bug 1660908 - Use rawNumPages rather than printPreviewNumPages for the total number of pages. r?emalysz,dholbert

Approved for 81.0b4.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9172046 [details]
Bug 1660908 - Use rawNumPages rather than printPreviewNumPages for the total number of pages. r?emalysz,dholbert

Actually, this needs rebasing for Beta due to conflicts with bug 1636728.

Comment 11

[Sam Foster [:sfoster] (he/him)]

Attachment: bug-1660908-beta.patch (attachment 9172046 / revision D88223 rebased on beta)

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Page range validation when printing will be incorrect when the start of the range is greater than the total number of pages
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:

1. Open print dialog for a page with enough content to span multiple pages
2. Select "custom" in the page-range dropdown
3. Enter a start and end page number towards the top of the possible range. E.g. 9-10 on a 10 page document.
   ER: With this patch, the range should not be marked invalid, printing can proceed and the "sheet count" accurately sums the number of sheets that will be printed (2 in this example)
   [List of other uplifts needed for the feature/fix]: None
   [Is the change risky?]: Low
   [Why is the change risky/not risky?]:
   Small front-end change to which page count total we use when setting min/max in the print UI.
   [String changes made/needed]: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9172784 [details] [diff] [review]
bug-1660908-beta.patch (attachment 9172046 [details] / revision D88223 rebased on beta)

Thanks for doing the rebase. Approved for 81.0b4.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5681907d4789

Comment 14

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 81.0b4 (BuildId:20200829200810) on Windows 10 64bit, macOS 10.14 & Ubuntu 20.04.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9172046 [details]
Bug 1660908 - Use rawNumPages rather than printPreviewNumPages for the total number of pages. r?emalysz,dholbert

Approved for 81.0b6 (no functional change, but we had to re-do this on account of bug 1660739 being uplifted to Beta).

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/58d924b91e59
https://hg.mozilla.org/releases/mozilla-beta/rev/ebe61ef2de63