fix(input): fix `step` validation for `input[number]`/`input[range]` #15264

gkalpak · 2016-10-13T17:58:58Z

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

What is the current behavior? (You can also link to an open issue here)
step validation (for input[number]/input[range]) fails on some occasions due to limitations of Floating Point Arithmetic (as described in #15257) and does not fully account for a stepBase as descussed in 9a8b8aa#commitcomment-19108436.

What is the new behavior (if this is a feature change)?
It works as expected!
BTW, according to the spec, when there is no min attribute (to determine the step base), but there is a value attribute, then the step base is set to the value of that. In our case this doesn't apply very well, because the value attribute is not compatible with ngModel and is essentially ignored.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
~~Docs have been added / updated (for bug fixes / features)~~

Other information:

Needs to be backported to 1.5.x for [ng-input-range].

Fixes 9a8b8aa#commitcomment-19108436
Fixes #15257

Related to 9a8b8aa and angular#15257. Fixes the issue discussed in angular@9a8b8aa#commitcomment-19108436. Fixes angular#15257

Step support works like min / max, but with the following caveat. Currently, only Firefox fully implements the spec. Other browsers (Chrome, Safari, Edge) have issues when the step value changes after the input has been changed. They do not adjust the input value to a valid value, but instead set the stepMismatch validity state. Angular will take this validity state, and forward it as the ngModel "step" error. Adjusting the error ourselves would add too much code, as the logic is quite involved.

…ange]`

gkalpak · 2016-10-14T22:29:27Z

test/ng/directive/inputSpec.js

@@ -2748,7 +2749,8 @@ describe('input', function() {
          expect($rootScope.value).toBe(30);

          // 0.3 - 0.2 === 0.09999999999999998
-          $rootScope.$apply('min = 0.2; step = 0.09999999999999998; value = 0.3');
+          $rootScope.$apply('min = 0.2; step = (0.3 - 0.2)');
+          helper.changeInputValueTo('0.3');


Not changing the scope value (e.g. $rootScope.$apply('value = '0.3')), because ngModel does not overwrite programmatically changed scope values unless the validity of the input changes. Depending on the order in which the validators are run and the previous state, this may lead to the scope value remaining 0.3 instead of being set to undefined.

Narretz · 2016-10-17T10:36:09Z

test/ng/directive/inputSpec.js

-        expect(inputElm).toBeValid();
-        expect($rootScope.value).toBe(20);
-        expect($rootScope.form.alias.$error.step).toBeFalsy();
+    forEach({


why not they?

I want to repeat the describe block. they is for repeating it blocks.

Oh, I see. Problem is you won't be able to see if ngStep or step failed, because the its have the same name. Maybe put the attribute name in the its?

It will be in describe's...description, so it is visible.

Oh, that's right!

Narretz · 2016-10-17T11:07:36Z

test/ng/directive/inputSpec.js

-        expect($rootScope.value).toBe(10);
-        expect(inputElm.val()).toBe('10');
-        expect($rootScope.form.alias.$error.step).toBeFalsy();
+        it('should correctly validate even in cases where `(x*y % x !== 0)`', function() {


Isn't this case an implementation detail? Can we give a better description?

It short of is, but value % step === 0 is the obvious way to implement it and it is the exact implementation that leads into errors. I mean we wouldn't have this issues if we had implemented it differently, but we want to explicitly test that the obvious implementation falls short.

That said, I am always open to suggestions 😉

Maybe "in cases where the JS floating point arithmetic fails"?

Works for me.

Narretz · 2016-10-17T11:09:05Z

src/ng/directive/input.js

+  // and `viewValue` is expected to be a valid stringified number.
+  var value = Number(viewValue);
+
+  // Due to limitations is Floating Point Arithmetic (e.g. `0.3 - 0.2 !== 0.1` or


is -> in/of

Narretz · 2016-10-17T11:18:26Z

src/ng/directive/input.js

+  var value = Number(viewValue);
+
+  // Due to limitations is Floating Point Arithmetic (e.g. `0.3 - 0.2 !== 0.1` or
+  // `0.5 % 0.1 !== 0`), we need to make sure all numbers are integers before proceeding.


need to convert all numbers to integers?

…ange]`

gkalpak · 2016-10-17T12:06:19Z

@Narretz, PTAL.

…ange]` Closes angular#15264

…ange]`

Narretz · 2016-10-19T10:16:57Z

LGTM

Related to 9a8b8aa and #15257. Fixes the issue discussed in 9a8b8aa#commitcomment-19108436. Fixes #15257 Closes #15264

Related to 9a8b8aa and angular#15257. Fixes the issue discussed in angular@9a8b8aa#commitcomment-19108436. Fixes angular#15257 Closes angular#15264

This reverts commit 90c08b8 feat(input): re-add support for binding to `input[range]` This commit re-applies the related (previously reverted) commits. A follow-up commit will make the support opt-in in order to avoid a breaking change. Included commits: - 296da4b - `feat(input): add support for binding to input[type=range]` (previously reverted with 6a167e8) - b78539b - `fix(input[range]): correctly handle min/max; remove ngMin/ngMax support` (previously reverted with aa60491) - 90c08b8 - `feat(input[range]): support step` (previously reverted with 5b633d8) fix(input): fix `step` validation for `input[number][ng-range-input]` Related to 9a8b8aa and angular#15257. Fixes the issue discussed in angular@9a8b8aa#commitcomment-19108436. Fixes angular#15257 Closes angular#15264

Related to 9a8b8aa and angular#15257. Fixes the issue discussed in angular@9a8b8aa#commitcomment-19108436. Fixes angular#15257 Closes angular#15264

Related to 9a8b8aa and #15257. Fixes the issue discussed in 9a8b8aa#commitcomment-19108436. Fixes #15257 Closes #15264

Related to 9a8b8aa and angular#15257. Fixes the issue discussed in angular@9a8b8aa#commitcomment-19108436. Fixes angular#15257 Closes angular#15264

gkalpak added 2 commits October 13, 2016 19:57

refactor(input): avoid duplicating step/ngStep tests

3b76233

fix(input): fix step validation for input[number]/input[range]

7039395

Related to 9a8b8aa and angular#15257. Fixes the issue discussed in angular@9a8b8aa#commitcomment-19108436. Fixes angular#15257

googlebot added the cla: yes label Oct 13, 2016

gkalpak added type: bug component: forms frequency: low needs: review severity: broken expected use branch: 1.5.x labels Oct 13, 2016

gkalpak modified the milestones: 1.5.x, 1.5.9 Oct 13, 2016

gkalpak force-pushed the fix-step-validation branch from 0083fc1 to bed92aa Compare October 14, 2016 21:56

fixup! fix(input): fix step validation for input[number]/`input[r…

10870c8

…ange]`

gkalpak force-pushed the fix-step-validation branch from bed92aa to 10870c8 Compare October 14, 2016 22:00

gkalpak commented Oct 14, 2016

View reviewed changes

Narretz reviewed Oct 17, 2016

View reviewed changes

fixup! fix(input): fix step validation for input[number]/`input[r…

0ecf8bd

…ange]`

Narretz pushed a commit to Narretz/angular.js that referenced this pull request Oct 17, 2016

fixup! fix(input): fix step validation for input[number]/`input[r…

22a728a

…ange]` Closes angular#15264

fixup! fix(input): fix step validation for input[number]/`input[r…

611b88c

…ange]`

gkalpak closed this in 081d06f Oct 19, 2016

gkalpak deleted the fix-step-validation branch October 19, 2016 12:05

gkalpak added a commit that referenced this pull request Oct 19, 2016

fix(input): fix step validation for input[number][ng-range-input]

e73a853

Related to 9a8b8aa and #15257. Fixes the issue discussed in 9a8b8aa#commitcomment-19108436. Fixes #15257 Closes #15264

petebacondarwin pushed a commit that referenced this pull request Nov 23, 2016

fix(input): fix step validation for input[number][ng-range-input]

c554e5d

Related to 9a8b8aa and #15257. Fixes the issue discussed in 9a8b8aa#commitcomment-19108436. Fixes #15257 Closes #15264

petebacondarwin pushed a commit that referenced this pull request Nov 24, 2016

fix(input): fix step validation for input[number][ng-range-input]

64f6a61

Related to 9a8b8aa and #15257. Fixes the issue discussed in 9a8b8aa#commitcomment-19108436. Fixes #15257 Closes #15264

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(input): fix `step` validation for `input[number]`/`input[range]` #15264

fix(input): fix `step` validation for `input[number]`/`input[range]` #15264

gkalpak commented Oct 13, 2016

gkalpak Oct 14, 2016

Narretz Oct 17, 2016

gkalpak Oct 17, 2016

Narretz Oct 17, 2016

gkalpak Oct 17, 2016

Narretz Oct 17, 2016

Narretz Oct 17, 2016

gkalpak Oct 17, 2016

Narretz Oct 17, 2016

gkalpak Oct 17, 2016

gkalpak Oct 17, 2016

Narretz Oct 17, 2016

Narretz Oct 17, 2016

gkalpak commented Oct 17, 2016

Narretz commented Oct 19, 2016

fix(input): fix step validation for input[number]/input[range] #15264

fix(input): fix step validation for input[number]/input[range] #15264

Conversation

gkalpak commented Oct 13, 2016

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

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

gkalpak commented Oct 17, 2016

Narretz commented Oct 19, 2016

fix(input): fix `step` validation for `input[number]`/`input[range]` #15264

fix(input): fix `step` validation for `input[number]`/`input[range]` #15264