fix(input[date]): support years with more than 4 digits #13905

LeeAdcock · 2016-01-31T04:36:03Z

Change to regular expressions used for matching ISO date strings. Increases the number of digits allowed in a year from exactly four to instead be four or more. Also matches on the entire string, resolving ambiguity around extra characters at the start of the year, or after the timezone.

Closes #13735

Change to regular expressions used to match date formats that increases the number of digits in a year. Previously exactly four digits were required, change allows four or more. Also matches complete string, resolving ambiguity around extra characters at the beginning or end.

…le.log

gkalpak · 2016-01-31T07:03:55Z

test/ng/directive/inputSpec.js

+      expect(+$rootScope.value).toBe(Date.UTC(10123, 2, 1, 0, 0, 0));
+
+      $rootScope.$apply(function() {
+        $rootScope.value = new Date(Date.UTC(10123, 2, 1, 0, 0, 0));


You should use a different value.

gkalpak · 2016-01-31T07:05:46Z

There's no test for datetime-local 😃

gkalpak · 2016-01-31T07:09:49Z

test/ng/directive/inputSpec.js

+        ['2010-01-01', false],                      // time must be specified
+        ['2010-01-0101:00:00.0000+01:01', false],   // missing date time seperator
+        ['2010-01-01V01:00:00.0000+01:01', false],  // invalid date time seperator
+        ['2010-01-01T01-00-00.0000+01:01', false],  // time must use period seperator


Actually, it must use colon separator.

… format Support for parsing additional variations of ISO dates, specifically the minute offset in timezones is now optional, as is the colon delimiter between the offset hour and minute. Enhancements to the tests were made to increase robustness and validate edge-case scenarios.

LeeAdcock · 2016-02-01T03:25:14Z

Thank you for the feedback, I appreciate it.

gkalpak · 2016-02-01T12:54:22Z

src/ng/directive/input.js

-// Regex code is obtained from SO: https://stackoverflow.com/questions/3143070/javascript-regex-iso-datetime#answer-3143231
-var ISO_DATE_REGEXP = /\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d\.\d+([+-][0-2]\d:[0-5]\d|Z)/;
+// Regex code was initially obtained from SO prior to modification: https://stackoverflow.com/questions/3143070/javascript-regex-iso-datetime#answer-3143231
+var ISO_DATE_REGEXP = /^\d{4,}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d\.\d+([+-][0-2]\d(:?[0-5]\d)?|Z)$/;


Since we are not using the groups anywhere, it would be better for performance to make them non-capturing.
(As an added bonus, you'll have the chance to write a RegExp that includes ?::? - not somethng you do everyday 😁)

BTW, I know I proposed this change, but there is currently an issue with IE/Edge (they can't parse a timezone offset when there is no colon), so additional steps might be needed. That said, I'm not sure when/if ISO_DATE_REGEXP is used after all, so let me look into it some more :)

Likely to be a problem if the input model is populated directly with an ISO formatted text string, instead of from the browser native picker.

I took a look at Edge in Saucelabs and found the same result when using the Date constructor for parsing both '2010-06-15T00:00:00.0000+01:30' as well as '2010-06-15T00:00:00.0000+0130'.

Is there a bug report for the Edge issue you referenced?

I don't know if there is a bug report - it just came up a few days ago and I added a workaround to fix a similar issue in #13887.

Likely to be a problem if the input model is populated directly with an ISO formatted text string

The ISO_DATE_REGEXP is only used in an ngModelController parser, meaning it will only be applied when parsing the $viewValue to transform it to a $modelValue. The only case where the $viewValue can be an ISO-8601 date string, is on browsers that do not natively support input[date] and the users enter it by hand (which is a rather unlikely interaction imo).

Anyway, assuming we still need ISO_DATE_REGEXP, what needs to be done in order to properly support all valid timezone offsets on all supported browsers, is "fixing up" the date string before passing it to new Date(). That would include adding a colon if missing and add 00 minutes if omitted.

Thinking about it again, this is totally unrelated to the initial intent of this PR, so it's not a good idea to mix them. I suggest, leaving only changes to add support for >4-digit years in this PR and if you fancy feel free to submit another one with the required changes to properly support all valid timezone offset formats.

Sorry, I had you make those changes and now telling they don't belong here 😞

…on and minutes

Narretz · 2016-02-08T19:30:06Z

@gkalpak Can you take another look at this?

gkalpak · 2016-02-08T21:33:31Z

@Narretz, will do. Thx for the heads-up ! (I hadn't notice the updates.)

gkalpak · 2016-02-08T22:36:09Z

test/ng/directive/inputSpec.js

+        expect(ISO_DATE_REGEXP.test(date)).toBe(valid);
+      });
+
+      it('should be non-capturing', function() {


This test is not needed. Capturing vs Non-capturing is just an implementation detail.

Happy to make this change, but would point out that if having the regex be non-capturing is important for performance, we should have a test that validates it. The regex was originally capturing, but that mistake wasn't noticed until this unit test was written. It's not unlikely that the next person that modifies it after us will unintentionally make it capturing again, unless there is a test enforces this style of implementation for performance gains.

This is still an implementation detail. E.g. if we decide to add support for all timezone formats, this will change: we'll need some capturing groups. Unless it is identified as a performance issue (which it won't - it's more of a theoretical performance gain, unlikely to be noticable), it remains an implementation detail.

So, while it's nice to avoid capturing groups if you don't need them, having a test for it will only make it more difficult to add functionality later. Unit tests should assert the functionality/behavior, not the performance characteristics, imo.

gkalpak · 2016-02-08T22:37:34Z

LGTM (expect for one minor comment).
Thx for working on this one, @LeeAdcock 👍

LeeAdcock · 2016-02-10T02:49:54Z

Thanks for the feedback. The implementation test has been removed.

Previously, the date-related regular expressions only matched years with no more than 4 digits. This commit adds support for years with more than 4 digits. It also resolves an ambiguity in `ISO_DATE_REGEXP` by matching the whole string (when it previosuly allowed extra characters around the date string). Fixes #13735 Closes #13905

gkalpak · 2016-02-10T10:15:18Z

Backported to v1.5.x as d76951f.

LeeAdcock added 2 commits January 31, 2016 04:08

test(input[date]): improve test descriptions, removed erroneous conso…

2ee3afc

…le.log

googlebot added the cla: yes label Jan 31, 2016

LeeAdcock added 2 commits January 31, 2016 04:41

fix(input[date]): resolving jshint finding

8574cc0

test(input[date]): removing unnecessary space, per jscs

a3408ed

gkalpak reviewed Jan 31, 2016
View reviewed changes

gkalpak added this to the Backlog milestone Jan 31, 2016

gkalpak added type: bug component: forms frequency: low needs: review severity: broken expected use labels Jan 31, 2016

gkalpak reviewed Feb 1, 2016
View reviewed changes

gkalpak self-assigned this Feb 1, 2016

LeeAdcock added 2 commits February 1, 2016 23:58

perf(input[date]): iso date matching regex should be non-capturing

6a0bf4f

fix(input[date]): iso date parsing, Edge timezone offset requires col…

335fc48

…on and minutes

gkalpak reviewed Feb 8, 2016
View reviewed changes

test(input[date]): removing test of non-functional regex implementation

a4acfb5

gkalpak closed this in 9425015 Feb 10, 2016

LeeAdcock deleted the input_date_fix_13735 branch February 10, 2016 14:04

fix(input[date]): support years with more than 4 digits #13905

fix(input[date]): support years with more than 4 digits #13905

Uh oh!

Conversation

LeeAdcock commented Jan 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Jan 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeeAdcock commented Feb 1, 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!

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!

Narretz commented Feb 8, 2016

Uh oh!

gkalpak commented Feb 8, 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!

gkalpak commented Feb 8, 2016

Uh oh!

LeeAdcock commented Feb 10, 2016

Uh oh!

gkalpak commented Feb 10, 2016

Uh oh!

Uh oh!