fix(input): re-validate when partially editing a date-family input #12902

gkalpak · 2015-09-21T15:53:09Z

In date-family input types (date, datetime-local, month, time, week), the user can interact with parts of the value (e.g. year, month, hours etc) independently. Neverhteless, the actual value of the element is empty ('') unless all parts are filled (and valid).

Thus, editing a signle part of the value may result in a change in the validity state of the <input> (see below), without an accompanying change in the actual value of the element. In such cases, no input event is fired by the browser to inform Angular of the change (and the need to re-validate).

The following scenario describes a series of interactions that would run into the problem (on a browser that supports the date input type):

Initially empty field.
- input.value: ''
- input.validity: {valid: true, badInput: false, ...}
The user fills part of the value (e.g. the year) using the keyboard.
- input.value: ''
- input.validity: {valid: false, badInput: true, ...}
- 'input' event: Not fired (since input.value hasn't changed)
The user fills the value completely (using either the keyboard or the
date-picker).
- input.value: ''
- input.validity: {valid: true, badInput: false, ...}
- 'input' event: Fired
The user deletes part of the value (e.g. the year) using the keyboard.
- input.value: '' (since a partial value is invalid)
- input.validity: {valid: false, badInput: true, ...}
- 'input' event: Fired
The user deletes all parts of the value using the keyboard (i.e. clears the field).
- input.value: ''
- input.validity: {valid: true, badInput: false, ...}
- 'input' event: Not fired (since input.value hasn't changed)

The problematic cases are (2) and (5), because there is a change in the validity state, but no 'input' event is fired to inform Angular of that change and the need to re-validate.

This commit fixes the issue by firing an input event on keyup, correctly triggering re-validation.

Fixes #12207

Narretz · 2015-09-24T15:17:48Z

src/ng/directive/input.js

+    // without an accompanying change in the input value (thus no `input` event).
+    // (This is only necessary on browsers that support inputs of that type - other browsers set the
+    //  `type` property to "text".)
+    var isTypeSupported = (type === attr.type);


can this name be a bit more explicit, such as browserSupportsType?

Narretz · 2015-09-24T15:21:07Z

I wonder if the tests might benefit from the they helper that matsko introduced some time ago. Otherwise it's a lot of repeated code.

gkalpak · 2015-09-25T08:56:45Z

I thought that too (having to copy-paste all around and change small things was no fun at all).

The problem is that some of the existing tests are different and some are the same, so only some of them could be they'd. And that would result in having tests for each type in two places (some in a dedicated describe block and some in a they block).
That's why I preferred having all tests for each type in a single block (even if that means duplication).

But I'm OK with they'ing as well.

jbedard · 2015-09-25T09:08:46Z

src/ng/directive/input.js

@@ -19,6 +19,7 @@ var DATETIMELOCAL_REGEXP = /^(\d{4})-(\d\d)-(\d\d)T(\d\d):(\d\d)(?::(\d\d)(\.\d{
 var WEEK_REGEXP = /^(\d{4})-W(\d\d)$/;
 var MONTH_REGEXP = /^(\d{4})-(\d\d)$/;
 var TIME_REGEXP = /^(\d\d):(\d\d)(?::(\d\d)(\.\d{1,3})?)?$/;
+var DATE_INPUT_TYPES = ['date', 'datetime-local', 'month', 'time', 'week'];


Missing datetime?

Angular does not support datetime :)
Is this one new ? (I am pretty sure it wasn't there last time I checked.)

You're right, I always assumed datetime was supported along with all the others. I thought it was just as old as the -local version, but I really don't know...

Should we add support for it ? How come nobody asked for it yet.
Hm...according to MDN, only Opera supports it properly atm.

idk, but that's probably off topic for this pr (sorry!)

In date-family input types (`date`, `datetime-local`, `month`, `time`, `week`), the user can interact with parts of the value (e.g. year, month, hours etc) independently. Neverhteless, the actual value of the element is empty (`''`) unless all parts are filled (and valid). Thus, editing a signle part of the value may result in a change in the validity state of the `<input>` (see below), without an accompanying change in the actual value of the element. In such cases, no `input` event is fired by the browser to inform Angular of the change (and the need to re-validate). --- The following scenario describes a series of interactions that would run into the problem (on a browser that supports the `date` input type): 1. Initially empty field. - `input.value`: '' - `input.validity`: {valid: true, badInput: false, ...} 2. The user fills part of the value (e.g. the year) using the keyboard. - `input.value`: '' - `input.validity`: {valid: false, badInput: true, ...} - 'input' event: Not fired (since `input.value` hasn't changed) 3. The user fills the value completely (using either the keyboard or the date-picker). - `input.value`: '<some-valid-value>' - `input.validity`: {valid: true, badInput: false, ...} - 'input' event: Fired 4. The user deletes part of the value (e.g. the year) using the keyboard. - `input.value`: '' (since a partial value is invalid) - `input.validity`: {valid: false, badInput: true, ...} - 'input' event: Fired 5. The user deletes all parts of the value using the keyboard (i.e. clears the field). - `input.value`: '' - `input.validity`: {valid: true, badInput: false, ...} - 'input' event: Not fired (since `input.value` hasn't changed) The problematic cases are (2) and (5), because there is a change in the validity state, but no 'input' event is fired to inform Angular of that change and the need to re-validate. --- This commit fixes the issue by firing an `input` event on `keyup`, correctly triggering re-validation. Fixes angular#12207

gkalpak · 2015-09-25T11:02:21Z

I pushed a second commit (squashable upon merge) that addresses the comments.
I haven't theyed the tests, though. WDYT (wrt to #12902 (comment)) ?

/cc @Narretz

Narretz · 2015-09-25T11:25:40Z

test/ng/directive/inputSpec.js

@@ -2,7 +2,7 @@

 /* globals getInputCompileHelper: false */

-describe('input', function() {
+ddescribe('input', function() {


Narretz · 2015-09-25T11:32:09Z

Regarding the repro comments in the tests: Can we link to this PR instead of repeating them? They are also in the commit message, right?

It just came to my mind, does a similar problem occur when the user cuts / pastes inside the input?

gkalpak · 2015-09-25T12:19:33Z

@Narretz, I shortened the comment and added a link to the PR (I want there to be a minimal description, because the test-code is very cryptic imo).

Regarding cut/paste: Theoretically, it could (when not using the keyboard), but (in Chrome at least) you can't cut/paste from/to date inputs.

Narretz · 2015-09-28T18:56:18Z

I just noticed that the second path for older IE that also listens to key events does it a bit differently. For one, it listens to keydown instead of keyup. I think we should do the same, or did you use keyup for a specific reason? The other thing is that the listener not called when modifiers are pressed at the same time. I think we want that, too. Final thing: it uses deferListener, but I am not sure why.

jbedard · 2015-09-28T20:23:35Z

I think it uses keydown because we might want to detect changes before keyup (such as when holding a key but not releasing). I think that makes it more like an input event that fires as soon as a change occurs.

The delay is because on keydown the input value is not updated yet.

In the case of keydown we must also check that the value actually changed to avoid marking an input as dirty from keys that don't actually change anything. Some of those are handled in the keydown handler (arrow or modifier keys) but other things (F#, ?) are not. Might be nice if those 2 different ways of ignoring keydown events were combined somehow, I'd think the one in the keydown handler can be dropped.

gkalpak · 2015-09-29T07:38:53Z

As @jbedard explained, on keydown the value is not updated yet. That's why I use keyup (could have gone with keydown and a deferred listener as well).

I wanted to keep this as small as possible, so didn't bother with ignoring not-input keys (I figured the overhead would be minimal). @jbedard, raises an interesting point though, I think: marking the input as dirty when we shouldn't.

@Narretz, @jbedard, do you think I should try to merge the paths for "browsers not supporting input event" and "browsers not supporting the specific type" ?
The problem with the deferListener in the second path is that it only calls listener() is input.value !== origValue, but we will have same value ('') and still want to call the listener.

WDYT ?

Narretz · 2015-09-29T18:14:23Z

I am not sure if the paths need to be merged, but they can probably share some functions.

gkalpak · 2016-01-21T10:40:31Z

Digging a little more into this, the problem does only affect Chrome and Opera atm (as they are the only ones that support these types and let you enter values partially.

AFAICT, there is only a limited number of keys that are able to change the value of these fields (monstly numeric keys):

For date, datetime-local, time, week:

0 - 9
numpad 0 - 9
up arrow / down arrow
Backspace, Delete

For month:

0 - 9
numpad 0 - 9
up arrow / down arrow
Backspace, Delete
Some letters (the initials of each month - depends on locale)

I think it's best to only react on these (and still not going to be a perfect solution).
But, I believe it's better than what we have now.

gkalpak · 2016-01-21T14:34:29Z

I pushed a new commit with the described changes (basically whitelisting specific keys per input type).
PTAL

Narretz · 2016-01-22T13:06:37Z

Looks good. Adds quite a few lines though ...

Narretz · 2016-01-22T13:37:51Z

And I'm not so sure about the 1000 tests that this adds :/

Could we group the date types together that react to the same keys? That should cut 3/5 of the tests.

gkalpak · 2016-01-22T14:54:29Z

What is your concern with the number of tests ? We could group them in several ways (e.g. run all keyCodes in a for-loop in one test, all run all input types together), but that wouldn't reduce the number of expectations.

Narretz · 2016-01-22T15:33:30Z

It just feels weird that this simple change adds 1000 expectations.
What I thought was to only test the default keys once (for everything expect datetime-local), and then once again for datetime-local (because it has additional triggers).

@petebacondarwin what do you think about this (the number of tests)?

gkalpak · 2016-01-22T15:54:26Z

The expectations are no many because I test every keyCode that shouldn't be ignored (~200 x 5 input types).

petebacondarwin · 2016-01-22T20:03:39Z

I understand your feelings @Narretz but I think we are happier with more tests, especially when they are mostly generated.

jbedard · 2016-01-22T20:13:55Z

I'm not 100% sure about this. Seems weird hardcoding specific key ranges that might effect parts of a date. What if different key ranges are used on different devices or browsers, or locales? What if I use the shift key because my months are capitalized?

Can we not just check for value/validity changes on every keypress? Maybe then this logic could be merged with the keydown logic already present? I haven't actually tried this out though, so I might be completely missing something...

jbedard · 2016-01-23T20:27:19Z

src/ng/directive/input.js

+          ? (key === keyOrRange) : (keyOrRange[0] <= key) && (key <= keyOrRange[1]);
+    });
+
+    if (affectsInput) callback('input');


Does passing 'input' here do anything? It seems like this is only used doing event = ev && ev.type which then gets passed as the "trigger" to $setViewValue, so it will be undefined in this case...

Looking at it a bit more I think this should be callback(evt).

Good catch. Since we are trying to "emulate" an input event, I think it should be callback({type: 'input'}).

jbedard · 2016-01-23T22:08:55Z

What about something such as 2ffe061 instead?

I think this has a few key advantages over the approach in this PR:

avoids hardcoding which keys cause changes (might miss some, i18n issues maybe? different device input types?)
avoids issues where those hardcoded keys do not cause changes (false positives)
also supports other events such as clicking the up/down arrows or using the mousewheel
only invokes listener if the validity actually changes as a result of one of the events (no false positives)

But it still has some issues:

hardcodes event types (might miss some on different types of devices?)
will only change the dirty flag if validity changes - if the input is already invalid, then $setPristine is called, then the user changes the value but it remains invalid then the original bug still occurs

I think those issues are worth the gains and simpler code though...

gkalpak · 2016-01-25T13:27:57Z

@jbedard, I think there will be trade-offs in any approach. This will not solve a browser limitation 100%, but improving on the current situation is a big win. I think I like 2ffe061 better (if nothing else it's simpler).

_{(Funny thing, it seems to be on the angular/angular.js repo, but I can't find it in the commit history - what do I miss ?)}

jbedard · 2016-01-26T06:22:36Z

@gkalpak I think that's a github comment bug? If you just put the hash (2ffe061) directly in a comment it seems to always assume it's a commit in this repo, the actual commit is at jbedard@2ffe061 (in this case I pasted the full URL of the commit).

Should I open a PR? Or do you want to change this one since you have some tests setup?

gkalpak · 2016-01-26T09:04:05Z

@jbedard, feel free to open a new PR (or I will later today).

gkalpak · 2016-02-05T09:45:57Z

Closing in favor of #13886.

googlebot added the cla: yes label Sep 21, 2015

Narretz added component: forms type: bug needs: review labels Sep 21, 2015

Narretz added this to the 1.4.x milestone Sep 21, 2015

Narretz added the branch: 1.4.x label Sep 21, 2015

Narretz reviewed Sep 24, 2015
View reviewed changes

jbedard reviewed Sep 25, 2015
View reviewed changes

gkalpak force-pushed the fix-input-detect-partial-edit-of-invalid-date branch 2 times, most recently from c9d4df0 to c0afe4d Compare September 25, 2015 10:59

Narretz reviewed Sep 25, 2015
View reviewed changes

fixup: call the listener directly instead of triggering an input event

1728a2f

gkalpak force-pushed the fix-input-detect-partial-edit-of-invalid-date branch 2 times, most recently from 1401e5e to 1728a2f Compare September 25, 2015 12:06

fixup: shorten comment in tests and point to the PR for more details

eabfcd2

gkalpak mentioned this pull request Nov 2, 2015

Issue with HTML 5's Date input type not firing ng-change when user changes the date using the default date picker... #1989

Closed

Narretz force-pushed the master branch from 70049ae to b77e14b Compare January 16, 2016 21:59

fixup: only react on keys that can affect the inputs display value

795793d

gkalpak mentioned this pull request Jan 22, 2016

input type=date .ng-valid-date stays after reset #13587

Closed

jbedard reviewed Jan 23, 2016
View reviewed changes

jbedard mentioned this pull request Jan 29, 2016

fix(input): re-validate when partially editing date-family inputs #13886

Closed

gkalpak added the branch: 1.5.x label Jan 31, 2016

gkalpak modified the milestones: 1.5.x - upgrade-facilitation, 1.4.x Jan 31, 2016

gkalpak closed this Feb 5, 2016

gkalpak deleted the fix-input-detect-partial-edit-of-invalid-date branch February 11, 2016 18:20

fix(input): re-validate when partially editing a date-family input #12902

fix(input): re-validate when partially editing a date-family input #12902

Uh oh!

Conversation

gkalpak commented Sep 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narretz commented Sep 24, 2015

Uh oh!

gkalpak commented Sep 25, 2015

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!

gkalpak commented Sep 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narretz commented Sep 25, 2015

Uh oh!

gkalpak commented Sep 25, 2015

Uh oh!

Narretz commented Sep 28, 2015

Uh oh!

jbedard commented Sep 28, 2015

Uh oh!

gkalpak commented Sep 29, 2015

Uh oh!

Narretz commented Sep 29, 2015

Uh oh!

gkalpak commented Jan 21, 2016

Uh oh!

gkalpak commented Jan 21, 2016

Uh oh!

Narretz commented Jan 22, 2016

Uh oh!

Narretz commented Jan 22, 2016

Uh oh!

gkalpak commented Jan 22, 2016

Uh oh!

Narretz commented Jan 22, 2016

Uh oh!

gkalpak commented Jan 22, 2016

Uh oh!

petebacondarwin commented Jan 22, 2016

Uh oh!

jbedard commented Jan 22, 2016

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!

jbedard commented Jan 23, 2016

Uh oh!

gkalpak commented Jan 25, 2016

Uh oh!

jbedard commented Jan 26, 2016

Uh oh!

gkalpak commented Jan 26, 2016

Uh oh!

gkalpak commented Feb 5, 2016

Uh oh!

Uh oh!