Print Preview: Show error message if text is typed in Copies section

Print Preview: Show error message if text is typed in Copies section

Native number input type returns an empty string for the current value
if it is invalid (non-numeric). Change to show an error message if this
happens. Previously did not show an error message in this case as the
old text input field would return the text in the field rather than an
empty string.

Also:
- fix both copies and scaling handling of scientific notation.
- remove regex parsing and use native input error checking

BUG=693493
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2713033003
Cr-Commit-Position: refs/heads/master@{#453661}
Committed: https://chromium.googlesource.com/chromium/src/+/c1d9ece59f24058d02965208c692c4204dcef799

[rbpotter, 2017-02-24 03:14:27]

Description was changed from

==========
Print Preview: Show error message if text is typed in Copies section

Native number input type returns an empty string for the current value
if it is invalid (non-numeric). Change to show an error message if this
happens. Previously did not show an error message in this case as the
old text input field would return the text in the field rather than an
empty string.

Note Chrome will not allow most non-numeric input for a number input,
but does permit the letter "e".

BUG=693493
==========

to

==========
Print Preview: Show error message if text is typed in Copies section

Native number input type returns an empty string for the current value
if it is invalid (non-numeric). Change to show an error message if this
happens. Previously did not show an error message in this case as the
old text input field would return the text in the field rather than an
empty string.

Note Chrome will not allow most non-numeric input for a number input,
but does permit the letter "e".

BUG=693493
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[rbpotter, 2017-02-24 03:14:32]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 03:14:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-02-24 03:17:39]

Description was changed from

==========
Print Preview: Show error message if text is typed in Copies section

Native number input type returns an empty string for the current value
if it is invalid (non-numeric). Change to show an error message if this
happens. Previously did not show an error message in this case as the
old text input field would return the text in the field rather than an
empty string.

Note Chrome will not allow most non-numeric input for a number input,
but does permit the letter "e".

BUG=693493
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Print Preview: Show error message if text is typed in Copies section

Native number input type returns an empty string for the current value
if it is invalid (non-numeric). Change to show an error message if this
happens. Previously did not show an error message in this case as the
old text input field would return the text in the field rather than an
empty string.

Note Chrome will not allow most non-numeric input for a number input,
but does permit the letter "e".

BUG=693493
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[rbpotter, 2017-02-24 03:17:39]

rbpotter@chromium.org changed reviewers:
+ dpapad@chromium.org

[commit-bot: I haz the power, 2017-02-24 04:48:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 04:48:59]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dpapad, 2017-02-24 20:35:36]

https://codereview.chromium.org/2713033003/diff/1/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/data/ticket_items/copies.js (right):

https://codereview.chromium.org/2713033003/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/ticket_items/copies.js:30: if
(/[^\d]+/.test(value)) {
Nit (optional): Maybe combine the two conditions.

if (value == '' || /[^\d]+/.test(value)) {
  return false;
}

https://codereview.chromium.org/2713033003/diff/1/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/data/ticket_items/copies.js:31: return
false;
Suggestion for improvement, can be done separately, but I'd like to know WDYT?

Now that we use native numerical input, we can take advantage of its native
validation logic and error reporting. See https://jsfiddle.net/5g682z0g/.
 1) We can remove all the custom validatino logic here and simply check
input.validity.valid
 2) When not valid, we can simply call input.reportValidity() and a native error
message will be displayed.

I think this would be a good improvement (and we would be using the platform
more).

[rbpotter, 2017-02-25 03:46:39]

Description was changed from

==========
Print Preview: Show error message if text is typed in Copies section

Native number input type returns an empty string for the current value
if it is invalid (non-numeric). Change to show an error message if this
happens. Previously did not show an error message in this case as the
old text input field would return the text in the field rather than an
empty string.

Note Chrome will not allow most non-numeric input for a number input,
but does permit the letter "e".

BUG=693493
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Print Preview: Show error message if text is typed in Copies section

Native number input type returns an empty string for the current value
if it is invalid (non-numeric). Change to show an error message if this
happens. Previously did not show an error message in this case as the
old text input field would return the text in the field rather than an
empty string.

Also:
- fix both copies and scaling dealing with scientific notation.
- remove regex parsing and use native input error checking

BUG=693493
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[rbpotter, 2017-02-25 03:51:02]

Description was changed from

==========
Print Preview: Show error message if text is typed in Copies section

Native number input type returns an empty string for the current value
if it is invalid (non-numeric). Change to show an error message if this
happens. Previously did not show an error message in this case as the
old text input field would return the text in the field rather than an
empty string.

Also:
- fix both copies and scaling dealing with scientific notation.
- remove regex parsing and use native input error checking

BUG=693493
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Print Preview: Show error message if text is typed in Copies section

Native number input type returns an empty string for the current value
if it is invalid (non-numeric). Change to show an error message if this
happens. Previously did not show an error message in this case as the
old text input field would return the text in the field rather than an
empty string.

Also:
- fix both copies and scaling handling of scientific notation.
- remove regex parsing and use native input error checking

BUG=693493
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[rbpotter, 2017-02-25 04:42:18]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-25 04:42:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-02-25 04:51:51]

Removed regex error handling and used input error checking.
Also made changes to allow scientific notation inputs to
work properly. Applied changes to scaling also.

[commit-bot: I haz the power, 2017-02-25 05:39:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-25 05:39:06]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-02-27 21:56:02]

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/ticket_items/copies.js (right):

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/ticket_items/copies.js:37: if
(this.getValue() == '')
Nit (optional)

var value = this.getValue();
return value == '' ? 0 : parseInt(value, 10);

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/ticket_items/scaling.js
(right):

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/ticket_items/scaling.js:48: return
true;
Is this resulting in the same behavior as before?

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/settings/scaling_settings.js
(right):

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/scaling_settings.js:267: if
(this.inputField_.value == '' && this.inputField_.validity.valid) {
I am slightly confused about this condition. It seems that the two clauses can
never be true at the same time, can they? Same question for Copiessettings.

[rbpotter, 2017-02-28 00:03:06]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 00:03:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-02-28 00:12:59]

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/ticket_items/copies.js (right):

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/ticket_items/copies.js:37: if
(this.getValue() == '')
On 2017/02/27 21:56:02, dpapad wrote:
> Nit (optional)
> 
> var value = this.getValue();
> return value == '' ? 0 : parseInt(value, 10);

Done.

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/data/ticket_items/scaling.js
(right):

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/data/ticket_items/scaling.js:48: return
true;
On 2017/02/27 21:56:02, dpapad wrote:
> Is this resulting in the same behavior as before?

It should be. This function was only called in isValid and in
scaling_settings. isValid was never called. scaling_settings now
checks validity.valid on the input.
This is different from copies because scaling is not allowed to update
to an invalid value since that would change the preview. Copies can
update to an invalid value, so isValid gets checked before the print
ticket gets sent for an actual print/before the print button is
enabled.

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/settings/scaling_settings.js
(right):

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/scaling_settings.js:267: if
(this.inputField_.value == '' && this.inputField_.validity.valid) {
On 2017/02/27 21:56:02, dpapad wrote:
> I am slightly confused about this condition. It seems that the two clauses can
> never be true at the same time, can they? Same question for Copiessettings.

They can actually both be true. This is the case for a truly empty
input, because I did not set required=true in the HTML. This allows
the behavior to stay the same as before (where a blank field does
not result in an error message). If the input is invalid then the
value will be returned as '' but validity.valid will be false. So
the combined condition selects only the case where the user has
an entirely blank input.

[dpapad, 2017-02-28 00:36:42]

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/settings/scaling_settings.js
(right):

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/scaling_settings.js:221: if
(Number(this.inputField_.value).toFixed(0) ===
Is the change from parseInt() to Number(...).toFixed() or Number(...).valueOf()
necessary in this CL?

The docs don't state the Number() constructor is supposed to parse non numerical
values, so (even though it seems to work) I would be hesitant to use it that
way. Can we revert those changes if they are not necessary?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

In addition to that there seems to exist a convenient valueAsNumber field on the
native <input>, so perhaps we could use that directly and avoid using Number()
or parseInt() completely.

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/scaling_settings.js:267: if
(this.inputField_.value == '' && this.inputField_.validity.valid) {
On 2017/02/28 at 00:12:59, rbpotter wrote:
> On 2017/02/27 21:56:02, dpapad wrote:
> > I am slightly confused about this condition. It seems that the two clauses
can
> > never be true at the same time, can they? Same question for Copiessettings.
> 
> They can actually both be true. This is the case for a truly empty
> input, because I did not set required=true in the HTML. This allows
> the behavior to stay the same as before (where a blank field does
> not result in an error message). If the input is invalid then the
> value will be returned as '' but validity.valid will be false. So
> the combined condition selects only the case where the user has
> an entirely blank input.

Thanks for the explanation.

[commit-bot: I haz the power, 2017-02-28 01:15:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 01:15:55]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-02-28 02:34:05]

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/print_preview/settings/scaling_settings.js
(right):

https://codereview.chromium.org/2713033003/diff/40001/chrome/browser/resource...
chrome/browser/resources/print_preview/settings/scaling_settings.js:221: if
(Number(this.inputField_.value).toFixed(0) ===
On 2017/02/28 00:36:42, dpapad wrote:
> Is the change from parseInt() to Number(...).toFixed() or
Number(...).valueOf()
> necessary in this CL?
> 
> The docs don't state the Number() constructor is supposed to parse non
numerical
> values, so (even though it seems to work) I would be hesitant to use it that
> way. Can we revert those changes if they are not necessary?
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
> 
> In addition to that there seems to exist a convenient valueAsNumber field on
the
> native <input>, so perhaps we could use that directly and avoid using Number()
> or parseInt() completely.

Done. Switched this to use the valueAsNumber field. The purpose of
this change was to deal with scientific notation. The number input
type accepts scientific notation (e.g. 1e1 instead of 10) but
parseInt() fails on these values and parses only until it gets to
the "e" (so 1e1 returns 1). valueAsNumber seems to work however.

[rbpotter, 2017-02-28 02:34:23]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 02:35:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 04:18:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 04:18:57]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-02-28 18:27:58]

lgtm

[rbpotter, 2017-02-28 19:18:01]

The CQ bit was checked by rbpotter@chromium.org

[commit-bot: I haz the power, 2017-02-28 19:18:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 19:24:50]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488309481249830,
"parent_rev": "37f315267bdd7c8efbd918e85f790e2794cc2dc5", "commit_rev":
"c1d9ece59f24058d02965208c692c4204dcef799"}

[commit-bot: I haz the power, 2017-02-28 19:25:32]

Description was changed from

==========
Print Preview: Show error message if text is typed in Copies section

Native number input type returns an empty string for the current value
if it is invalid (non-numeric). Change to show an error message if this
happens. Previously did not show an error message in this case as the
old text input field would return the text in the field rather than an
empty string.

Also:
- fix both copies and scaling handling of scientific notation.
- remove regex parsing and use native input error checking

BUG=693493
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Print Preview: Show error message if text is typed in Copies section

Native number input type returns an empty string for the current value
if it is invalid (non-numeric). Change to show an error message if this
happens. Previously did not show an error message in this case as the
old text input field would return the text in the field rather than an
empty string.

Also:
- fix both copies and scaling handling of scientific notation.
- remove regex parsing and use native input error checking

BUG=693493
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2713033003
Cr-Commit-Position: refs/heads/master@{#453661}
Committed:
https://chromium.googlesource.com/chromium/src/+/c1d9ece59f24058d02965208c692...
==========

[commit-bot: I haz the power, 2017-02-28 19:25:33]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c1d9ece59f24058d02965208c692...