Print Preview page number range can be exceeded
Categories
(Core :: Print Preview, defect)
Tracking
()
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)
2.24 MB,
video/mp4
|
Details | |
47 bytes,
text/x-phabricator-request
|
lizzard
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
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
- Open Firefox
- Access this PDF
- Open Print Preview
- 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.
Reporter | ||
Updated•5 years ago
|
Comment 1•5 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
Comment 2•5 years ago
|
||
Regressed by:
00d33e7058c1261b31ddd3b8df31e39c98212c3e Tim Nguyen — Bug 1437641 - Remove numberbox binding and convert usages to input[type=number]. r=bgrins,dao
Updated•5 years ago
|
Comment 4•5 years ago
|
||
(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 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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 5•5 years ago
|
||
Assignee | ||
Comment 6•5 years ago
|
||
(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?
Comment 7•5 years ago
•
|
||
(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.
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8886a5a5ddd4 limit page numbers in print preview, r=mconley
Comment 9•5 years ago
|
||
bugherder |
Assignee | ||
Comment 10•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 11•5 years ago
|
||
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 12•5 years ago
|
||
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.
Comment 13•5 years ago
|
||
bugherder uplift |
Comment 14•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 15•5 years ago
|
||
That video in #c0 looks pretty broken - should we take this on ESR68 too?
Assignee | ||
Comment 16•5 years ago
|
||
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
Comment 17•5 years ago
|
||
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.
Comment 18•5 years ago
|
||
bugherder uplift |
Comment 19•5 years ago
|
||
Verified - fixed on latest 68.2.0esr (64-bit) (Build id: 20191014163636) on Windows 10 and Ubuntu 18.04.
Description
•