maxlength issue in 1.3.0 beta 15 (invalid -> valid)

[snekbaev]

Hi,

I was using beta 11 and everything was fine, upgraded to 15 and got the issue. I output a complex object with several ng-repeats etc, which eventually boils down to something like:

<input class="form-control" name="questionText" type="text" ng-model="question.text" placeholder="{{ 'ContentEntity_EvaluateTask_ClaimPlaceholder' | i18n }}" maxlength="150" required />

I have CSS classes setup that highlight invalid fields with red. So after upgrade to beta 15 when entire object is rendered I see that input fields become red (my css style for invalid state) for a fraction of a second and then back to "valid" (non red). When I remove the maxlength attribute - no blinking occurs. As I mentioned beta 11 works just fine.

Thank you!

P.S.: using IE11

[Narretz]

Can you please provide a reproduction in a plnkr.co or similar?

[snekbaev]

Hi, sure, here you go: http://plnkr.co/mY219dSGAl1EEngRRavr open in IE11 and you will see a quick red blink. If you comment out beta.15 script reference and put back beta.5 - no blinking occurs. While using beta.15 script try removing the "maxlength" attribute and you will see that blinking doesn't happen. Thank you!

[caitp]

just tried this on saucelabs and I'm not seeing the issue? (I did modify the plunk to use ng-maxlength rather than maxlength though, because browsers will tend to not allow you to enter more characters than the actual maxlength attribute value).

[Narretz]

I checked on IE11, I see the issue, and it's truly confusing. In the plunker, for the working input, I replaced maxlength with ng-maxlength and then the error appears there, too. However, when I replace ng-maxlength on the non-working example, nothing changes.

Also, the flash appears first in beta.12, which is when the validator refactoring was introduced. I remember there was a bug with ng-maxlength and initial validation, maybe that is related.

It's pretty strange though that only the input in ngRepeat is affected

[snekbaev]

Hi @caitp , thank you for your reply. I opened that plnkr and saw the blinking. I changed my local version to use ng-maxlength, I still see the blinking, moveover, now I can enter more characters than the limit I've set (though field gets marked as invalid). That's why I was using the maxlength because it was limiting the input.

[snekbaev]

@Narretz I jumped from 11 to 15, quickly checked that it was reworked from the sources, but didn't check since which version :) but yeah, apparently you found it too :) I'm staying with beta11 meanwhile, otherwise I have massive red blinking on every tree item selection :) Thank you!

[snekbaev]

@Narretz forgot to mention that at least 'textarea's are also affected in addition to input, maybe other elements too. I think this needs to be thoroughly investigated.

[caitp]

what exactly do you mean by "blinking"? can you take a screencapture or something? I didn't see anything that looked like blinking reproducing on SL.

[snekbaev]

Hi @caitp, here you go: http://www.particlefusion.org/Flashing.avi (file will be removed when the issue is resolved): I have opened the plnkr there and pressed F5 - you will see that inputs inside ng-repeat will become red (invalid, because of the css style) for a fraction of a second and back to normal (valid). I switched IE11 to IE10 mode via F12 - same flashing, but when switched to IE9 mode - no flashing. I don't have real IE9 to test, but I think at least a label "browser: IE10" can be relevant to this issue too. Thank you!

[caitp]

well I still can't reproduce that at all on saucelabs (except, on one initial load of the page, before any refreshing, each input was invalid momentarily) --- but after that initial run haven't been able to get it to flicker at all.

Anyways, it's really hard to identify the issue when I can't reproduce it consistently, maybe @Narretz would have a go at figuring out the cause.

[snekbaev]

I've modified the plnkr, now you have a button, if clicked - it blinks :)

[snekbaev]

@caitp you said that "on one initial load of the page, before any refreshing, each input was invalid momentarily" - that's what I'm getting too. But it doesn't have to be the initial load. I have a SPA, one area is divided into 2 parts: left has the content tree and the right area contains the editor specific to the selected tree item. When user clicks on the tree item I load the specific editor for it (SPA uses ui-router). With beta.11 experience was good, but with beta.15 every time user clicks on the tree item the newly loaded editor which contains many inputs/dropdowns/textareas - all those fields blink with red at first just like in the plnkr. As you may have guessed this makes a very strange/bad impression :)

[caitp]

I believe that you're experiencing it, however I can't reproduce it consistently, it would be better for someone who can reproduce it consistently to investigate the issue.

[snekbaev]

of course, thank you!

[Narretz]

I'll take a closer look at this :)

[Narretz]

I had a look at this and it seems fairly complex, although I might be missing something.

• it only appears with input+ngMaxlength inside ngRepeat
• It was introduced by https://github.com/angular/angular.js/commit/26d91b653ac224d9d4166fea855346f5e4c4a7b4
• It is not fixed by https://github.com/angular/angular.js/pull/7968
• The first flicker is because during the first validation maxlength is 0, i.e. the attribute hasn't been interpolated yet
• when inside ngRepeat, the ngModelWatch fires before maxlength is interpolated
• after I applied this patch https://github.com/angular/angular.js/pull/7304, the initial flicker went away, but the flicker on click is still there
• maybe related: https://github.com/angular/angular.js/issues/8156
• the reduction shows another problem related to the model being a number in conjunction with textInput Type (length validator cannot validate numbers), but that is not related to the maxlength validation problem at hand
• I assume it only happens in IE, because it is slower than other browsers to update the classes, i.e. the two validations (from ngModelWatch & the observed maxlength) happen so fast after each other in other browsers that they don't bother to apply the class

[Narretz]

@caitp That actually means you could have a look at this, since it affects all browsers ;)

[caitp]

If it affects all browsers, I would hope to be able to reproduce it in another browser---however I cannot get the affected behaviour at all outside of SL, and even there it occurred very rarely.

I'm not sure this is even worth spending much effort on. It may be that the animation is unable to be cancelled correctly in IE for whatever reason, when it would be cancelled in Chrome or Firefox

[caitp]

/cc @matsko

[Narretz]

I don't think it has anything to do with animations. I tested only in Firefox, but you should see ngModelWatch fires before the maxlength attribute is interpolated when inside ngRepeat, causing the initial validation to fail - you can only see a visual result in IE, though. This looks like an ngRepeat / compile bug, that's why I thought you might want to have a look. Have you had a look at my comment above the one where I @ed you?

[caitp]

far as I can tell we're using CSS transitions in the plunker, otherwise you'd have no idea that any issue ever occurred. Now, we might be adding and removing the classes quickly enough that it doesn't cause any visual effect in other browsers. I have not seen this issue in any observable fashion in either Chrome or Firefox, and as I said, very rarely in IE.

Futzing with the priorities of directives is not really in the cards, because they are so finicky and prone to breakage, so there isn't much we can do about the order of ngModelWatch vs ngMaxLength (however, given the default priority, I would expect this to also happen with ngMinLength and other validators alphabetically preceeding ngModelDirective).

Fixing the visual feedback problem is probably more acheivable.

[Narretz]

You are right, the transitions are making this visible. I still think at the core is a problem with ngRepeat, causing the directive link functions to work differently when repeated.

[snekbaev]

@Narretz apparently not only ng-repeat, I modified the plnkr and added the ng-if (ng-show is not affected) based logic. When button is clicked - same flickering and as usual, removing 'maxlength' attribute solves the problem :( Also if bootstrap.min.css include is removed - no flickering happens, though UI is totally broken, thus not an option :) This is getting more and more interesting :)

P.S.: thank you all for helping me out!

[alexeykudinkin]

This is b/c the link function of the maxlength directive is a crap:

var maxlengthDirective = function() {
        return {
            restrict: 'A',
            require: '?ngModel',
            link: function(scope, elm, attr, ctrl) {
                if (!ctrl) return;

                var maxlength = 0;
                attr.$observe('maxlength', function(value) {
                    maxlength = int(value) || 0;
                    ctrl.$validate();
                });
                ctrl.$validators.maxlength = function(value) {
                    return ctrl.$isEmpty(value) || value.length <= maxlength;
                };
            }
        };
    };

Should go like this instead:

var maxlengthDirective = function() {
        return {
            restrict: 'A',
            require: '?ngModel',
            link: function(scope, elm, attr, ctrl) {
                if (!ctrl) return;

                var maxlength = 0;

                attr.$observe('maxlength', function(value) {
                    maxlength = int(value) || 0;

                    if (!angular.isDefined(ctrl.$validators.maxlength)) {
                        ctrl.$validators.maxlength = function(value) {
                            return ctrl.$isEmpty(value) || value.length <= maxlength;
                        };
                    }

                    ctrl.$validate();
                });

            }
        };
    };

[pandamouse]

@alexeykudinkin This works if value is string but fails if value is a number (type='number'). value.length

[matsko]

https://github.com/angular/angular.js/pull/7968

[matsko]

This is fixed in RC0: http://plnkr.co/edit/R7CMpOkVXJQL2GzFEUfj?p=preview

[snekbaev]

@matsko that plnkr still blinks...

[Narretz]

Yey, I can reproduce this in IE11.

[caitp]

I will see if I can write a failing unit test for this tonight and try to figure it out, but the reproduction is just boggling because it basically never reveals any issues for me (very rarely on SL/IE, never with any other browser for me)

[caitp]

I really don't see how this can only appear with ngRepeat / other structural directives. If this is related to ngMaxlength being its own directive and causing a second validation run, then that's one thing --- but I really don't see what difference a structural directive should make here, since ngModel would be in the same scope and it wouldn't affect the latency between dealing with the model watch and the maxlength.

[caitp]

I guess the issue is like, the element is dynamically shown during a digest --- during the same digest, the validation happens, it is invalid because before attrs.$set() is called, the maxlength is 0 --- then attrs.$set() happens (???) and the element is valid again?

I'm not sure if that's really fixable ._> (I mean the general case where classes are changing back and forth and causing jank on janky browsers)

at any rate, it's a really hard thing to test :c

I guess one way to do it would be to avoid immediately setting classes, we could try and set the classes in post digest instead

[caitp]

@matsko it might take some coordination to implement a fix like I'm talking about above, since it involves ngAnimate

[caitp]

The following test case sort of shows what the jank is:

    iit('should minimize setting of classes', inject(function($animate, $compile, $rootScope) {
      var addClass = $animate.addClass;
      var removeClass = $animate.removeClass;
      var addClassCallCount = 0;
      var removeClassCallCount = 0;
      var input;
      $animate.addClass = function(element, className) {
        addClass.call($animate, element, className);
        if (input && element[0] === input[0]) {
          console.log("adding " + className);
          ++addClassCallCount;
        }
      }

      $animate.removeClass = function(element, className) {
        removeClass.call($animate, element, className);
        if (input && element[0] === input[0]) {
          console.log("removing " + className);
          ++removeClassCallCount;
        }
      }

      dealoc(element);

      $rootScope.value = "123456789";
      element = $compile(
        '<form name="form">' +
            '<input type="text" ng-model="value" name="alias" ng-maxlength="10">' +
        '</form>'
      )($rootScope);

      var form = $rootScope.form;
      input = element.children().eq(0);

      $rootScope.$digest();

      expect(input).toBeValid();
      expect(addClassCallCount).toBe(3);
      expect(removeClassCallCount).toBe(0);

      dealoc(element);
    }));
  });
});

It logs:

LOG: 'removing ng-valid'
LOG: 'adding ng-invalid'
LOG: 'adding ng-invalid-maxlength'
LOG: 'adding ng-valid-parse'
LOG: 'adding ng-valid'
LOG: 'removing ng-invalid'
LOG: 'adding ng-valid-maxlength'
LOG: 'removing ng-invalid-maxlength'

And this is all within one digest --- so what we want to end up with is just adding ng-valid, ng-valid-maxlength, ng-valid-parse --- with no removals. We can do this if we defer adding of classes until the end of digest, or at least until the end of validation

[caitp]

@snekbaev could you do me a favour and try out the patch I just wrote? I'm hoping this will be enough to reduce the janky behaviour you're seeing in IE, but I'm really not sure it will. It's very hard to test this effectively

[caitp]

As soon as the jenkins build is ready I'll give you a link to the scripts built with that patch

[caitp]

@snekbaev you should be able to try it with the files from https://ci.angularjs.org/job/angular.js-caitlin/561/artifact/build/ --- although that build in particular does have some gunk which has been subsequently removed, it should be good enough I think

[caitp]

(from the PR)

http://plnkr.co/edit/d2qK45maTpcPTkU64ewI?p=preview here's a fork of the plunker from the original issue, which when tested on SL with IE11, seems to indicate that this fix does what we want.

I was able to reproduce with the beta 15 plunker pretty quickly (using both the ng-if button and the ng-repeat button), (although the ng-if button took a few more tries to see the blinking), but after a few minutes of trying, the forked plunker would not display any of the blinking.

[JimtotheB]

In IE11 I was able to reproduce the red flashing in http://plnkr.co/mY219dSGAl1EEngRRavr

it seems to be fixed by #9263 in IE11 as seen here http://plnkr.co/edit/d2qK45maTpcPTkU64ewI?p=preview

[snekbaev]

@caitp thank you for taking care of this! I'm on vacation atm, however quick RDP to my work PC has revealed same results as @PaperElectron has previously encountered: first one blinks as expected, but second plnkr doesn't. Apparently you fixed it :) I will test it thoroughly when I'm back in a couple of weeks. Thanks again!

[tbosch]

Assigned to @caitp as she is working on a PR for this