feat(input[range]): support step #14970
Conversation
Narretz
changed the title
|
@gkalpak I can't replicate the bug you described in #14870 (comment). Here's my repro: http://plnkr.co/edit/cBc2YAySJzD8y0NukAEQ?p=preview There's also another test case (fn angTestCase()) that shows how Chrome stops adjusting the element value based on the step when the value has been modified previously. I think I'll raise both bugs in the Webkit tracker, but I'm totally prepared that this is following the spec in some obscure way ... |
|
I can't reproduce it on Chrome v52 either. (I was on v51 when I reproduced it. It got probably fixed, introducing another bug 😉) |
|
Reported as https://bugs.chromium.org/p/chromium/issues/detail?id=633075 (stops adjusting) and https://bugs.chromium.org/p/chromium/issues/detail?id=633074 (adjusts to wrong value) |
| stepChange(attr.step); | ||
| attr.$observe('step', stepChange); | ||
| } | ||
|
|
There was a problem hiding this comment.
There is a lot of similar (almost duplicate) code here. In the interests of keeping the code size down is it possible to move this into a helper function? Perhaps:
function setupHandlers(type, attrType, noopValidationFn, validatorFn, changeFn) {
if (attrType) {
ctrl.$validators[type] = attributeType === type && supportsRange ? noopValidatorFn : validatorFn;
if (attrType === type) {
element.attr(type, attr[type]);
}
changeFn(attr[type]);
attr.$observe(type, changeFn);
}
}
setupHandlers('max', maxAttrType, ...);
|
One refactoring comment. Other than that LGTM |
Narretz
force-pushed
the
feat-input-step
branch
from
ad3ec3d to
2e5112b
Compare
|
@petebacondarwin Sorry Pete, the current state of the PR is outdated :o I just pushed the updates. The main difference for range to before is that we don't support ngStep, because the browser would still use the default step of 1, and we don't test the step change updates anymore, because not all browsers implement them. |
Narretz
changed the title
Narretz
force-pushed
the
feat-input-step
branch
from
2e5112b to
94528bd
Compare
| @@ -1642,6 +1690,21 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) { | |||
| } | |||
| } | |||
|
|
|||
| function stepChange(val) { | |||
| stepVal = parseNumberAttrVal(val); | |||
There was a problem hiding this comment.
Perhaps do this after the check for NaN below to save unnecessary computation?
There was a problem hiding this comment.
Oh - this is a side-effect? OK
|
I feel sad that we don't support |
|
That's why I didn't document it at all. Because it's not really useful. |
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.
input[number] will now set the step error if the input value (ngModel $viewValue) does not fit the step constraint set in the step / ngStep attribute. Fixes angular#10597
Narretz
force-pushed
the
feat-input-step
branch
from
94528bd to
132698a
Compare
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feat
What is the current behavior? (You can also link to an open issue here)
Input range doesn't handle the step attribute wrt to validation and model updates
Does this PR introduce a breaking change?
No
TODO
Other information:
Wrt the Chrome bug mentioned in #14870 (comment), it looks like setting a number isn't the problem, but that Chrome has a bug when you change the step value multiple times. Not sure yet, though.