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 26d91b6
• It is not fixed by fix(input): validate minlength/maxlength for non-string values #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 fix(ngRepeat): ensure expected order of execution for directive lifecycle functions is maintained when they are nested along with ngRepeat #7304, the initial flicker went away, but the flicker on click is still there
• maybe related: Order of execution of pre and post linking functions is not consistent when transcluded content is having ng-repeat #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]

@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