1653389 - Validate the page range setting

Status: VERIFIED FIXED

(Toolkit :: Printing, enhancement, P1)

Description

[Mark Striemer [:mstriemer]]

We'll want to validate the page range setting. At first we will only support a single page, or a single page range.

The total number of pages is currently shown in the existing print preview UI, this is returned by PrintingChild when the preview is updated [1]. We'll likely need the preview hooked up before we can get this value.

What we'll want to validate is basically that the text matches the format x or x-y (where x and y are both integers), then that 1 <= x <= y <= PAGE_COUNT.

[1] https://searchfox.org/mozilla-central/rev/1b95a0179507a4dc7d4b0c94c2df420dc1a72885/toolkit/components/printing/content/printPreviewToolbar.js#448-453

Comment 1

[Emma Malysz]

Attachment: Bug 1653389, validate page range setting

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1653389 WIP patch / proof-of-concept

Comment 3

[Phabricator Automation]

Comment on attachment 9170571 [details]
Bug 1653389 WIP patch / proof-of-concept

Revision D87394 was moved to bug 1659005. Setting attachment 9170571 [details] to obsolete.

Comment 4

[Daniel Holbert [:dholbert]]

Sorry for spam; disregard comment 2-3. (bug number copypaste error)

Comment 5

[Emma Malysz]

Attachment: Bug 1653389, show error if page range input is invalid

Matches scale naming format.

Comment 6

[Daniel Holbert [:dholbert]]

Right now the patches here are two independent phabricator pages; I suspect you want to link them into a stack, using the "edit related revisions" at the top right of one or them. (Then you can land them together, assuming that's your intent.)

Comment 7

[Daniel Holbert [:dholbert]]

Also, there are a few usability/papercut issues that I'm noticing, when testing with these patches applied (which I'm doing while looking into an implementation strategy for bug 1659005). We can spin these issues off as one or more followups after this lands, too, but I'm not sure if it's better to address them up-front?

STR:

1. Apply both patches on this bug to mozilla-central.
2. Start a print operation, and choose "Pages: Custom"
3. Click the right edge of the "from" or "to" text field
4. Press the backspace key (e.g. pretend you're just getting ready to type in the From/To value that you'd like, which means you have to clear the field first, of course)

ACTUAL RESULTS:

• The red error text and border appears, chastising me about having chosen an invalid page range (simply due to having an empty text field)
• Most of the settings/controls suddenly become disabled. The dialog now forces me to fix this field before it will let me do anything else, it seems.
• Also, the error text occupies space and pushes content down out of the way (which feels jumpy)

EXPECTED RESULTS:

• I would expect the error text to not appear yet when I've simply backspaced the number -- it should give me a "grace period" while I'm editing the text field. That's how form validation works normally, e.g. at data:text/html,<input type="number" value="5" min="5" max="10"><input type="text"> which has a number field that only accepts values from 5-10 -- in this example, I can focus that number field, clear the value with my backspace key, and type in an invalid number, and the field doesn't change to highlight the error until I move focus outside of the field (e.g. by hitting tab or clicking the other field). I would expect the From/To fields to behave the same way.
• When the field is in an "invalid value" state, I would expect the rest of the print settings area to nonetheless remain useful. E.g. maybe I started to edit my page range, and then realized I want to print in Landscape mode or choose a different page size -- it feels overly strict for Firefox to force me to fix my page range before it'll let me change those things (which pretty much invalidate my page range anyway)
• Furthermore, when the field is in an "invalid value" state, I would expect printing to still be possible -- we should just treat the invalid field as if it contained the most-extreme-acceptable value in that field (e.g. treat invalid input as "1" for the "from" field, and treat invalid input as "number-of-pages" for the "to" field). We could either do this implicitly or explicitly (filling in the value for the user - why make them type it?) In fact, this is what already happens if I click "Print using the system dialog" after having typed in a bogus value for part of the range.
• Also: cosmetically, it feels odd for the error text to move things around in the dialog. Ideally, we should warn the user in a way that doesn't push other settings/UI out of the way.

Comment 8

[Daniel Holbert [:dholbert]]

(I guess comment 7 largely applies to the "Scale" field as well, which has similar behavior. So maybe that's all out of scope for this particular bug.)

Comment 9

[Emma Malysz]

Thanks for reviewing this, Daniel! This is all really helpful.

I'll add a timeout (similar to the one for percent scale) to avoid the problem you were seeing in #1. I can add that to the second patch.
For #2/3, that was expected behavior detailed in Bug 1656057. Maybe we should file a follow up? Mark, do you have a preference?
We do want to disable printing when anything in the form is invalid, and the behavior of #3 matches other browsers.
I can file a follow up for #4 to better format the error messages.

Comment 10

[Emma Malysz]

I'll add a timeout (similar to the one for percent scale) to avoid the problem you were seeing in #1. I can add that to the second patch.

I just noticed the timeout only applies to sending the update, not showing the error message. I'm going to create a follow up bug to cover this since it's applicable to scaling as well.

Comment 11

[Mark Striemer [:mstriemer]]

Delaying the error and not showing the error when the field is empty sound good to me.

For disabling the form elements, that's what Chrome and Edge do and we didn't have any error state mock ups so we copied it. We should be disabling the "Print using the system dialog..." as well I think. I agree that the disabling can be a bit annoying if you want to change some other setting that will affect it, we could potentially just disable the Print/Save button and dialog link. I think disabling the other elements helps bring focus to what needs to be changed, though.

Unfortunately I think we'll need to live with the error message pushing other content down for now. I don't think we have time to implement another solution, as Emma said this is also what Chrome and Edge do.

Comment 12

[Daniel Holbert [:dholbert]]

One other issue I noticed with the patch (as of this morning at least, when I last applied it for my local build):

If you provide a range From: [large-but-valid page] to [smaller valid page], e.g. "From 5 to 1" in a 10-page document, then our error message doesn't make sense. We highlight the "From" field as invalid, and we say Range must be a number between 1 and 10; but the value that I've given (5) is a number between 1 and 10.

Comment 13

[Daniel Holbert [:dholbert]]

(We might need a second error message for that condition, to say e.g. The "from" page cannot be larger than the "to" page)

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

Lando could not land the patches, please rebase.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dae38df00129
validate page range setting r=sfoster,mstriemer,fluent-reviewers,flod
https://hg.mozilla.org/integration/autoland/rev/a7d229c67a22
show error if page range input is invalid r=fluent-reviewers,sfoster,flod

Comment 16

[Emma Malysz]

Attachment: Bug 1653389, show a better error message if the start range is greater than the end range

Comment 17

[Emma Malysz]

Attachment: Bug 1653389, show clearer error message if the start range is greater than the end range

Comment 18

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/dae38df00129
https://hg.mozilla.org/mozilla-central/rev/a7d229c67a22

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74b3ba7e7ac0
show clearer error message if the start range is greater than the end range r=fluent-reviewers,mstriemer,flod

Comment 20

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/74b3ba7e7ac0

Comment 21

[Jonathan Watt [:jwatt]]

I think the last patch needs a request for beta approval?

Comment 22

[Emma Malysz]

Comment on attachment 9171087 [details]
Bug 1653389, show clearer error message if the start range is greater than the end range

Beta/Release Uplift Approval Request

• User impact if declined: A user will see a less specific error message, which could be confusing.
• 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. Open nightly

2. Ensure print.tab_modal.enabled is turned on
3. Print a page (command + P) with multiple pages (ex: about:home)
4. Flip page range from "All" to "Custom"
5. Specify start range as 2
6. Specify end range as 1
7. Notice error message displayed is The “from” page number must be smaller than the “to” page number.
8. Type in an invalid range for either display (eg. x)
9. Notice error message displayed is Range must be a number between 1 and...

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9171087 [details]
Bug 1653389, show clearer error message if the start range is greater than the end range

This landed on m-c prior to the merge to Beta AFAICT.

Comment 24

[Emil Ghitta, QA [:emilghitta]]

This is verified fixed using Firefox 81.0b2 (BuildId:20200825191644) on Windows 10 64bit, macOS 10.14 & Ubuntu 20.04