Closed Bug 1585276 Opened 3 months ago Closed 2 months ago

Print Preview page number range can be exceeded

Categories

(Core :: Print Preview, defect)

66 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- verified
firefox69 --- wontfix
firefox70 --- verified
firefox71 --- verified

People

(Reporter: csasca, Assigned: Gijs)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Attached video Range exceed.mp4

Affected versions

  • Firefox 68.1.0esr
  • Firefox 70.0b11
  • Firefox 71.0a1

Affected platforms

  • Windows 10/7 (x64)
  • Ubuntu 16.04 (x64)

Steps to reproduce

  1. Open Firefox
  2. Access this PDF
  3. Open Print Preview
  4. Click on < or > from toolbar to navigate through the pages.

Expected result
The page number is locked within the range.

Actual result
The user is able to navigate past the number of the pages the sample has.

Regression range
This seems to be a regession since the issue is not reproducible on 60.9.0esr. I will look for one asap.

Additional notes

  • The actual issue can be seen in the attachment.
Has Regression Range: --- → no
Has STR: --- → yes

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

Regressed by:
00d33e7058c1261b31ddd3b8df31e39c98212c3e Tim Nguyen — Bug 1437641 - Remove numberbox binding and convert usages to input[type=number]. r=bgrins,dao

Regressed by: 1437641
Version: Trunk → 66 Branch
Has Regression Range: no → yes

:ntim, can you take a look?

Flags: needinfo?(ntim.bugs)

(In reply to :Gijs (he/him) from comment #3)

:ntim, can you take a look?

The value setter of the numberbox binding used to validate the value, and validation is also done on every change event. You could re-implement the behaviour here: https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/toolkit/components/printing/content/printPreviewToolbar.js#129-131

or directly in https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/toolkit/components/printing/content/printPreviewToolbar.js#244

or alternatively, we could do this as a customized input element if we wanted to fix this elsewhere too.

The tricky part here is testing, I unfortunately only have a macOS which makes it tricky to test this.

Flags: needinfo?(ntim.bugs)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

(In reply to Tim Nguyen :ntim from comment #4)

The tricky part here is testing, I unfortunately only have a macOS which makes it tricky to test this.

So was the original patch also written without testing it? That seems unfortunate...

Anyway, I've put up a patch to deal with this particular issue, but can you please re-check your patch in bug 1437641 to see if there are other cases where there used to be validation that is now absent?

Flags: needinfo?(ntim.bugs)

(In reply to :Gijs (he/him) from comment #6)

(In reply to Tim Nguyen :ntim from comment #4)

The tricky part here is testing, I unfortunately only have a macOS which makes it tricky to test this.

So was the original patch also written without testing it? That seems unfortunate...

I did have a Linux VM at the time I wrote the patch and did test it there. It unfortunately ran out of disk space and got corrupted, and I haven't had time to setup a new one since.

Anyway, I've put up a patch to deal with this particular issue.

Thanks!

can you please re-check your patch in bug 1437641 to see if there are other cases where there used to be validation that is now absent?

There's only the port fields in the connection settings, where you can now type in invalid values as a result of bug 1437641, but the input will highlight red if you do so. We were aware of this behaviour change in bug 1437641 comment 15, though I guess it wouldn't hurt to add extra validation on submit or something.

Flags: needinfo?(ntim.bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8886a5a5ddd4
limit page numbers in print preview, r=mconley
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Comment on attachment 9097829 [details]
Bug 1585276 - limit page numbers in print preview, r?mconley

Beta/Release Uplift Approval Request

  • User impact if declined: Confusing results from using buttons / inputbox to change page number in print preview
  • 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: See comment #0
  • List of other uplifts needed: n/a
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Straightforward JS change to add some validation; main alternative is accepting the regression for 70.
  • String changes made/needed: Nope
Attachment #9097829 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Reproduced the initial issue using an old Nightly build: 20191002033852
Verified - Fixed on latest Nightly 71.0a1 (2019-10-07) (64-bit) Build ID 20191007092954, on Windows 10 and Ubuntu 18.04.
Updating flag and waiting for fix on Beta.

Comment on attachment 9097829 [details]
Bug 1585276 - limit page numbers in print preview, r?mconley

Fix for fairly recent regression, verified in nightly, let's uplift for beta 13.

Attachment #9097829 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Reproduced the initial issue using an old Beta build: 20191002212950
Verified - fixed on latest Beta 70.0b13 (64-bit) (Build id: 20191007220302) on Windows 10 and Ubuntu 18.04.

QA Whiteboard: [qa-triaged]
QA Whiteboard: [qa-triaged]

That video in #c0 looks pretty broken - should we take this on ESR68 too?

Flags: needinfo?(gijskruitbosch+bugs)

Comment on attachment 9097829 [details]
Bug 1585276 - limit page numbers in print preview, r?mconley

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: current behaviour is visibly really broken
  • User impact if declined: see above
  • Fix Landed on Version: 71 uplifted to 70
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Straightforward JS-only patch to fix the validation here. Alternative is living with the brokenness
  • String or UUID changes made by this patch: nope
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #9097829 - Flags: approval-mozilla-esr68?

Comment on attachment 9097829 [details]
Bug 1585276 - limit page numbers in print preview, r?mconley

Fixes some broken behavior in print preview. Verified on Beta. Approved for 68.2esr.

Attachment #9097829 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Verified - fixed on latest 68.2.0esr (64-bit) (Build id: 20191014163636) on Windows 10 and Ubuntu 18.04.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.