fix(input): make md-maxlength validation happen on initialization #11150

feloy · 2018-03-04T20:44:40Z

PR Checklist

Please check that your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

When an input has an md-maxlength attribute and an initial value longer than this maxlength, no error is triggered until the input value is changed.

Issue Number: #10320

What is the new behavior?

When an input has an md-maxlength attribute and an initial value longer than this maxlength, an error is triggered at initialization.

Note that the error message is not shown at initialization but only when the focus leave the input (without content changes), which seems to be the correct behavior (in a similar way, for 'required' input with empty initial value, no message is displayed at initialization).

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Closes #10320

Splaktar · 2018-03-04T21:12:40Z

Same as #11149 (comment), Please add "Closes #10320" to the footer of your commit message as detailed in the commit message guidelines. Just adding it the PR description is not enough as that won't trigger the issue to be closed on merge or show up in the changelog properly.

Splaktar

This looks great! Thanks for adding the tests!

I just had a couple of minor questions.

Splaktar · 2018-03-04T21:16:21Z

src/components/input/input.js

@@ -634,11 +634,25 @@ function mdMaxlengthDirective($animate, $mdUtil) {
  };

  function postLink(scope, element, attr, ctrls) {
-    var maxlength;
+    var maxlength = Number.parseInt(attr.mdMaxlength);
+    if (isNaN(maxlength)) maxlength = -1;


Should this use !angular.isNumber(maxlength) to be consistent with line 645, or is there a reason that isNaN better here?

No, that's not equivalent here, because angular.isNumber(NaN) returns true but in this case, we do not want to keep NaN.

Ah yes, I thought that angular.isNumber() was smarter than it actually is.

Splaktar · 2018-03-04T21:18:03Z

src/components/input/input.js

    });

    function renderCharCount(value) {
      // If we have not been appended to the body yet; do not render
-      if (!charCountEl.parent) {
+      if (!charCountEl || !charCountEl.parent) {


Is || correct here? Should this be &&?

We now have to check that charCountEl is not undefined or charCountEl.text will fail in the following lines in some cases (because charCountEl is initialized at nextTick and renderCharCount can now be called before nextTick, from validator).

OK, can you please update the comment as well?

Splaktar · 2018-03-05T00:54:35Z

Btw, amending commits is the best way to keep your changes in a single commit when updates are needed. Then force push to your branch is the best way to sync those updates to the branch.

Splaktar · 2018-03-05T07:58:42Z

Looks great! Thank you for your patience. I've sent this over to the caretaker to have presubmit tests run.

Splaktar · 2018-03-10T02:08:50Z

This is currently triggering some presubmit failures that appear to be flakes (i.e. from flaky tests, not actual real failures). This is being looked at internally by Google. No work is needed on your side at this time.

Splaktar · 2018-03-10T02:10:57Z

I re-ran the failing build steps in Travis and everything is green now 😃

jelbourn · 2018-03-12T17:59:03Z

This fails google presubmit because Number.parseInt is not supported in IE11

Splaktar

IE11 support is needed, then we can merge this. Thanks!

Splaktar · 2018-03-12T18:10:44Z

src/components/input/input.js

@@ -634,11 +634,25 @@ function mdMaxlengthDirective($animate, $mdUtil) {
  };

  function postLink(scope, element, attr, ctrls) {
-    var maxlength;
+    var maxlength = Number.parseInt(attr.mdMaxlength);


As pointed out by Jeremy, Number.parseInt is not supported in IE11. So we'll need to use something different here.

It looks like the global parseInt should work?
MDN also points out that Number.parseInt has no IE support.

I've used the global parseInt instead

Closes angular#10320

Splaktar

LGTM

…gular#11150) Closes angular#10320

…1150) Closes #10320

googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Mar 4, 2018

Splaktar self-assigned this Mar 4, 2018

Splaktar self-requested a review March 4, 2018 21:10

Splaktar added this to the 1.1.8 milestone Mar 4, 2018

Splaktar added g3: reported The issue was reported by an internal or external product team. P2: required Issues that must be fixed. labels Mar 4, 2018

Splaktar reviewed Mar 4, 2018

View reviewed changes

Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Mar 4, 2018

Splaktar added the needs: squash commits label Mar 5, 2018

feloy force-pushed the issue-10320 branch 2 times, most recently from 759a501 to c31fffd Compare March 5, 2018 07:08

Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer and removed needs: squash commits in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Mar 5, 2018

Splaktar assigned jelbourn Mar 5, 2018

Splaktar added the pr: presubmit-failures label Mar 10, 2018

Splaktar suggested changes Mar 12, 2018

View reviewed changes

fix(input): make md-maxlength validation happen on initialization

0325341

Closes angular#10320

feloy force-pushed the issue-10320 branch from c31fffd to 0325341 Compare March 12, 2018 19:06

Splaktar approved these changes Mar 12, 2018

View reviewed changes

jelbourn merged commit 9ced357 into angular:master Mar 12, 2018

Splaktar removed the pr: presubmit-failures label Mar 17, 2018

chmelevskij pushed a commit to chmelevskij/material that referenced this pull request Jun 19, 2018

fix(input): make md-maxlength validation happen on initialization (an…

21a9e68

…gular#11150) Closes angular#10320

Splaktar pushed a commit that referenced this pull request Jul 31, 2018

fix(input): make md-maxlength validation happen on initialization (#1…

78233b5

…1150) Closes #10320

fix(input): make md-maxlength validation happen on initialization #11150

fix(input): make md-maxlength validation happen on initialization #11150

Uh oh!

Conversation

feloy commented Mar 4, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

Splaktar commented Mar 4, 2018

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Splaktar commented Mar 5, 2018

Uh oh!

Splaktar commented Mar 5, 2018

Uh oh!

Splaktar commented Mar 10, 2018

Uh oh!

Splaktar commented Mar 10, 2018

Uh oh!

jelbourn commented Mar 12, 2018

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!