fix(input): allow overriding timezone for date input types #16336

Narretz · 2017-11-21T10:18:54Z

Fixes #13382
Closes #16181

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
fix

What is the current behavior? (You can also link to an open issue here)
See #16181

What is the new behavior (if this is a feature change)?
overrideModelOptions can be used to change the timezone.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

petebacondarwin

nice work @Narretz

gkalpak

I wonder if we should document that changing the value timezone will have no effect until the next parse/format.

gkalpak · 2017-11-21T14:39:56Z

src/ng/directive/input.js

    var previousDate;

    ctrl.$$parserName = type;
    ctrl.$parsers.push(function(value) {
-      if (ctrl.$isEmpty(value)) return null;
+      if (ctrl.$isEmpty(value)) {
+        previousDate = null;


I am not sure this change is valid (in fact I think it is not).
AFAICT the idea of previousDate is to store the last valid date set (to use it for feeling missing date field that cannot be derived from the input value).

If it's not valid, then why are there no test failures? :D
The formatter is doing the same thing afaict - it sets previousDate = null if the model is not a valid date (empty is not a valid date, too).
The specific test at https://github.com/angular/angular.js/pull/16336/files/d172e8ecb650ad3220e00e46b08325ad3672e9f1#diff-ce6d0f141f60d25eede7abf044d04b93R1968 shows that when clearing out the input, previousDate is still set and this leads the next input to use an incorrect value for the hour part.

We'd need a test case to show that this is a regression.

If it's not valid, then why are there no test failures? :D

Valid point...nooot 😛

The formatter is doing the same thing afaict - it sets previousDate = null if the model is not a valid date (empty is not a valid date, too).

That is the point. The formatter processes the model value and previousDate keeps track of the last valid (date) model value.

The specific test at https://github.com/angular/angular.js/pull/16336/files/d172e8ecb650ad3220e00e46b08325ad3672e9f1#diff-ce6d0f141f60d25eede7abf044d04b93R1968 shows that when clearing out the input, previousDate is still set and this leads the next input to use an incorrect value for the hour part.

"use an incorrect value for the hour part" is subjective. I'd say currently it uses "the correct value for the hour part".
The idea is to be able to set the hour for a date field. AFAICT, the only way to achieve this today is by setting the model to a date with the desired date. Then, as the user edits the field (including emptying and refilling it) the hour/minute/second part is preserved. I think this is a desired property (I am pretty sure I've used it in my apps 😃).
In any case, I believe this change is breaking not related to this PR. It should go into a separate PR (where it can be debated further 😁).

The idea is to be able to set the hour for a date field.

Ah, that's right. Did you get this from the code or did you search for the commit? Here's the commit: 1a1ef62

As you can see, there are not tests for type=date ... :<

This is at least confusing, because as you can see in the test, the date is re-using the hour part that was set for -0500, even though the timezone is set to UTC again. But maybe this is a contrived example.

I didn't search for the commit (I didn't expect there to be a commit specific for that - I assumed it was part of the initial implementation) 😃

So, I am not sure what the correct behavior is here (wrt to changing the timezone) 😕
Maybe keep track of the previous timezone and re-adjust? Or accept the fact that changing the timezone may change the time part?
Timezones are tricky 😁

gkalpak · 2017-11-21T14:41:11Z

src/ng/directive/input.js

@@ -1456,6 +1459,7 @@ function createDateInputType(type, regexp, parseDate, format) {
      }
      if (isValidDate(value)) {
        previousDate = value;
+        var timezone = ctrl.$options.getOption('timezone');
        if (previousDate && timezone) {


Nit (and unrelated to this PR): previousDate will always be truthy here (else isValidDate() would be false).

gkalpak · 2017-11-21T15:03:07Z

test/ng/directive/inputSpec.js

+      inputElm.controller('ngModel').$overrideModelOptions({timezone: '-0500'});
+
+      $rootScope.$apply(function() {
+        $rootScope.value = new Date(Date.UTC(2014, 6, 1));


Why the year change (here and below)? I would stick to the minimum necessary changes to avoid confusion.

Narretz · 2017-11-21T15:49:37Z

Regarding the docs - any property changes in ngModelOptions only have an effect after the next update. We could add this info in overrideModelOptions generally.

Narretz · 2017-11-23T10:46:50Z

I've removed the date reset and added a different fix that should account for changing timezones. I'm not 100% happy with the implementation, but I also don't want to spend too much time on this.

Narretz · 2017-12-01T11:13:20Z

@gkalpak could you give this another look?

gkalpak · 2017-12-04T11:13:02Z

test/ng/directive/inputSpec.js

+      helper.changeInputValueTo('2013-W03');
+      expect(+$rootScope.value).toBe(Date.UTC(2013, 0, 17));
+
+      inputElm.controller('ngModel').$overrideModelOptions({timezone: '+5000'});


Is this really supposed to be +5000?

Revised the test and added more comments.

gkalpak · 2017-12-04T11:14:39Z

test/ng/directive/inputSpec.js

+      inputElm.controller('ngModel').$overrideModelOptions({timezone: '+5000'});
+
+      $rootScope.$apply(function() {
+        // the 17. with an offset of +5000 moves the date into next week


Can you expand a bit on this comment? It mentions 17. (which is in German format 😛), but the code uses 18. Also, how does it move to the next week?

gkalpak · 2017-12-04T11:16:44Z

test/ng/directive/inputSpec.js

+
+      inputElm.controller('ngModel').$overrideModelOptions({timezone: 'UTC'});
+      helper.changeInputValueTo('23:02:00');
+      // The year is still set from the previous date


gkalpak · 2017-12-04T11:19:58Z

test/ng/directive/inputSpec.js

@@ -1879,6 +1966,30 @@ describe('input', function() {
      dealoc(formElm);
    });

+    it('should not reuse the hour part of a previous date object after changing the timezone', function() {


Not sure how this test checks that the hour part of the previous date object isn't used.

Also Revised this test and added more comments.

gkalpak · 2017-12-04T11:22:38Z

src/ng/directive/input.js

        // Note: We cannot read ctrl.$modelValue, as there might be a different
        // parser/formatter in the processing chain so that the model
        // contains some different data format!
+        if (timezone && previousTimezone && previousTimezone !== timezone) {
+          // If the timezone has changed, adjust the previousDate to the default timzeone


timzeone --> timezone

gkalpak · 2017-12-04T11:24:34Z

src/ng/directive/input.js

        // Note: We cannot read ctrl.$modelValue, as there might be a different
        // parser/formatter in the processing chain so that the model
        // contains some different data format!
+        if (timezone && previousTimezone && previousTimezone !== timezone) {


Why do we need timezone or previousTimezone to be truthy? Don't we always need to adjust as long as previousTimezone !== timezone?

We do need previousTimezone to be defined, since we pass it to addDateMinutes. timezone must not be defined, though.

Narretz

I've revised the tests in hope that they are clearer now, or at least it's easier to understand them. Let me know if I have failed :D

Narretz · 2017-12-04T21:09:43Z

src/ng/directive/input.js

        // Note: We cannot read ctrl.$modelValue, as there might be a different
        // parser/formatter in the processing chain so that the model
        // contains some different data format!
+        if (timezone && previousTimezone && previousTimezone !== timezone) {


We do need previousTimezone to be defined, since we pass it to addDateMinutes. timezone must not be defined, though.

Narretz · 2017-12-04T21:10:55Z

test/ng/directive/inputSpec.js

+      helper.changeInputValueTo('2013-W03');
+      expect(+$rootScope.value).toBe(Date.UTC(2013, 0, 17));
+
+      inputElm.controller('ngModel').$overrideModelOptions({timezone: '+5000'});


Revised the test and added more comments.

Narretz · 2017-12-04T21:11:16Z

test/ng/directive/inputSpec.js

@@ -1879,6 +1966,30 @@ describe('input', function() {
      dealoc(formElm);
    });

+    it('should not reuse the hour part of a previous date object after changing the timezone', function() {


Also Revised this test and added more comments.

gkalpak · 2017-12-05T09:26:32Z

test/ng/directive/inputSpec.js


+      //To check that the timezone overwrite works, apply an offset of +24 hours.
+      //Since January 19 is a Saturday, +24 will turn the formatted Date into January 20 - Sunday -
+      //which is in calendar week 4 instead of 3.


Missing whitspace after // 😇

gkalpak · 2017-12-05T09:29:59Z

test/ng/directive/inputSpec.js

-
-      // Emptying the input should clear the cached date object
-      helper.changeInputValueTo('');
+      // At this point, the cached Date has its hours set to to 20 (00:00 - 05:00 = 19:00)


I am confused 😕
Are you saying that 19 === 20?

No, just a result of trying to understand timezones. If you log a date, you always get it with the local timezone offset which is +1 for me, which means 20. But 19 is correct.

This commit also fixes a bug where part of the Date object was re-used even after the input was emptied. Fixes angular#16181 Closes angular#13382

This commit also fixes a bug where part of the Date object was re-used even after the input was emptied. Fixes #16181 Closes #13382 Closes #16336

Narretz added component: forms frequency: moderate severity: broken expected use type: bug labels Nov 21, 2017

Narretz added this to the 1.6.x milestone Nov 21, 2017

Narretz requested a review from gkalpak November 21, 2017 10:18

googlebot added the cla: yes label Nov 21, 2017

Narretz force-pushed the fix-input-timezone branch from 0da52ef to d172e8e Compare November 21, 2017 10:20

petebacondarwin approved these changes Nov 21, 2017

View reviewed changes

gkalpak reviewed Nov 21, 2017

View reviewed changes

Narretz force-pushed the fix-input-timezone branch from 93a98f5 to 3a985cf Compare November 23, 2017 10:49

Narretz modified the milestones: 1.6.x, 1.6.8 Nov 30, 2017

gkalpak reviewed Dec 4, 2017

View reviewed changes

Narretz commented Dec 4, 2017

View reviewed changes

gkalpak reviewed Dec 5, 2017

View reviewed changes

Narretz force-pushed the fix-input-timezone branch from e843205 to 347a1aa Compare December 5, 2017 13:16

Narretz force-pushed the fix-input-timezone branch from 347a1aa to 7a9447c Compare December 13, 2017 18:44

Narretz modified the milestones: 1.6.8, 1.6.9 Jan 4, 2018

Narretz modified the milestones: 1.6.9, 1.6.10 Feb 2, 2018

Narretz added 3 commits April 6, 2018 13:07

fix(input): allow overriding timezone for date input types

6b21e54

This commit also fixes a bug where part of the Date object was re-used even after the input was emptied. Fixes angular#16181 Closes angular#13382

fixup! remove incorrect date reset bugfix

9ef6897

fixup! simplify and improve test info

6e618ef

Narretz force-pushed the fix-input-timezone branch from 7a9447c to 6e618ef Compare April 6, 2018 11:19

Narretz merged commit 3a2aea7 into angular:master Apr 6, 2018

Narretz added a commit that referenced this pull request Apr 6, 2018

fix(input): allow overriding timezone for date input types

4355dee

This commit also fixes a bug where part of the Date object was re-used even after the input was emptied. Fixes #16181 Closes #13382 Closes #16336

This was referenced Apr 18, 2018

Update angular in package.json from 1.6.9 to 1.6.10 ui-router/sample-app-angular-hybrid#69

Closed

Update 4 dependencies from npm JetBrains/ring-ui#226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(input): allow overriding timezone for date input types #16336

fix(input): allow overriding timezone for date input types #16336

Narretz commented Nov 21, 2017 •

edited

Loading

petebacondarwin left a comment

gkalpak left a comment

gkalpak Nov 21, 2017 •

edited

Loading

Narretz Nov 21, 2017

gkalpak Nov 22, 2017

Narretz Nov 22, 2017

gkalpak Nov 22, 2017

gkalpak Nov 21, 2017

gkalpak Nov 21, 2017

Narretz commented Nov 21, 2017

Narretz commented Nov 23, 2017

Narretz commented Dec 1, 2017

gkalpak Dec 4, 2017

Narretz Dec 4, 2017

gkalpak Dec 4, 2017

gkalpak Dec 4, 2017

gkalpak Dec 4, 2017

Narretz Dec 4, 2017

gkalpak Dec 4, 2017

gkalpak Dec 4, 2017

Narretz Dec 4, 2017

Narretz left a comment

Narretz Dec 4, 2017

Narretz Dec 4, 2017

Narretz Dec 4, 2017

gkalpak Dec 5, 2017

gkalpak Dec 5, 2017

Narretz Dec 5, 2017

fix(input): allow overriding timezone for date input types #16336

fix(input): allow overriding timezone for date input types #16336

Conversation

Narretz commented Nov 21, 2017 • edited Loading

petebacondarwin left a comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Nov 21, 2017

Narretz commented Nov 23, 2017

Narretz commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Nov 21, 2017 •

edited

Loading

gkalpak Nov 21, 2017 •

edited

Loading