feat(input): add support for input[type=range] #14870

Narretz · 2016-07-05T19:47:45Z

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

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:

the input[type=range] behavior is the same of an input[type=number]
with min=0, max=100 and step=1 as defaults

Closes #5892
Closes #9715

googlebot · 2016-07-05T19:47:46Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-07-05T19:47:46Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

Narretz · 2016-07-08T09:36:27Z

The biggest issue is that browser that implement range set the element value to a valid value if you try to set an invalid value. Two cases: you try to set a non-number: input value is set to 50. You try to set a value that exceeds max or is lower than min: value is set to max / min value.

This is problematic for two reasons:

When we try to set the input value from the scope, as it creates a mismatch between input and scope value.
null / undefined models are set to 50, which means the input can never have the required error set, even though from an application perspective the model is still null / undefined

In the PR, I have changed it so that we update the model after the browser has changed the value.
This obviously runs parsers / validators again.
It also causes some unexpected effects when you bind the same model to range and number for example. When you try to clear the number input, the range will always set the model (and input) back to 50.

As an alternative, I though of creating a validator that adds a misMatch error the the control that is set whenever the browsers updates the value and it is now distinct from the model.

gkalpak · 2016-07-08T11:03:09Z

BTW, it doesn't set it to 50, it sets it to ~~(max - min) / 2~~ (max + min) / 2. So, basically, you can't set it to an undefined value. I am still not sure it is a good idea to add something like this is core - too many quirks 😟

gkalpak · 2016-07-08T11:04:49Z

src/ng/directive/input.js

+
+  ctrl.$render = function() {
+    if (ctrl.$isEmpty(ctrl.$viewValue)) {
+      ctrl.$viewValue = '50';


This shouldn't be hard-coded to 50. It should be (max - min) / 2.

BTW, i case it wasn't clear, 50 is the result of ~~(max - min) / 2~~ (max + min) / 2 for the default min (0) and max (100) values.

Yep, the PR is currently missing the min max handling.

the correct formula is actually min + ((max - min) / 2)

Sorry, I meant (max + min) / 2 (which is equivalent to your formula).
I will correct it to avoid confusion.

gkalpak · 2016-07-08T11:39:06Z

I think we need to better handle the situation where min/max change dynamically. The browser will adjust the element's value to the new min/max values if necessary and we don't account for that.

Narretz · 2016-07-08T13:31:35Z

That's true. I noticed that but haven't included it yet. So what do you in general think about updating the model when the browser changes the values?

gkalpak · 2016-07-08T14:53:33Z

Yeah, I think we should read the viewValue every time the min, max values change (and if necessary run parsers/validators).
(Not sure how step affects the element's value.)

Narretz · 2016-07-11T08:59:22Z

When a step is set, the browser rounds non-matching values to the nearest step ...

gkalpak · 2016-07-11T09:22:19Z

So, how does affect ngModel? When the model is set to an "off-step" value, the browser will round the value to the nearest step and the change will propagate to the modelValue?

Narretz · 2016-07-11T09:42:31Z

That's for us to decide. We already want to set undefined / null models to 50, so we could also propagate the step-adjusted value back to the model. But I'm not sure if this isn't unexpected behavior. For example, you are loading a legacy value from a database that doesn't match your step value. Now you want the user to correct this value. The first problem is that the input will be auto-set to a fitting value (and the model too if we implement it). Now the user does not know that "his" value has been changed, and the application doesn't know if the user actually wanted to set this value. The input is also not marked as invalid, so the user can submit the form etc. and basically send a value to the DB that he didn't even know changed.

Another option for us might be to introduce a "mismatch" error that is set on the model controller when the input value is changed after it is set from the model.

gkalpak · 2016-07-11T10:13:05Z

It is a problematic situation either way. For example if we leave the original (off-step) modelValue, the field will be invalid, but a valid value will be shown to the user. And in order to set the modelValue to the corrected, nearest step value, they will have to move the knob to a different value and back, so we can detect a viewValue change and parse that.

Things like that make me think that supporting range is going to cause more headaches than it will solve problems. Users are probably better of using a non-native slider/range input - e.g. as an independant module or from a UI framework (such as ngMaterial). 😞

Narretz · 2016-07-11T10:16:51Z

But for simple cases, binding to range is almost as simple as binding to number. So I'm not sure we should bail out of this simply because there are some edge cases.

petebacondarwin · 2016-07-11T10:40:37Z

So from reading the HTML5 specs, I think it is fair to say that range is “special"

In the sense that it can _never_ hold an invalid value

That means that we must always update the ngModel $viewValue to a valid value however it is updated (via user interaction or programmatically).

If we agree that this is the case, and document it clearly, then I think we could go ahead with this directive...

I think that we should also, when reacting to model changes, simply compute the appropriate valid value that would work for the control and update the model explicitly ourselves

So in the case of a model that is out of “step”, if we have this range

<input type="range" min="0" max="9" step="2" ng-model="obj.val">

and we try to set obj.val to 2.6, then we force the model to 2 as part of the formatting.

roryokane · 2016-07-23T15:59:07Z

src/ng/directive/input.js

@@ -1530,6 +1530,11 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) {

    // TODO(matsko): implement validateLater to reduce number of validations
    if (minAttrType === 'min') {
+      var elVal = element.val();
+      // IE11 doesn't set the el val correctly if the maxVal is less than the element value


This comment doesn’t match the surrounding code. Bad copy-paste from line 1572?

gkalpak · 2016-07-27T09:38:47Z

src/ng/directive/input.js

+      } :
+      // ngMin doesn't set the min attr, so the browser doesn't adjust the input value as setting min would
+      function minValidator(modelValue, viewValue) {
+        return ctrl.$isEmpty(viewValue) || isUndefined(minVal) || viewValue >= minVal;


Why calling $isEmpty with the viewValue?

Because we are validating the $viewValue. I think dates use the modelValue, but this is something we should change, imo.

I think we almost always call $isEmpty with the modelValue (regardless of what value we are actually validating). Imo it makes sense to (a) be consistent what we call $isEmpty with and (b) always call it with modelValue (which can be $modelValue or $$rawModelValue), in order to account for custom parsers.

For example, an input[range]'s viewValue can never be empty, but I might want to have a custom parser that converts '0' to null and I want this to be considered empty.

_{It is debatable whether we should be validating the $viewValue or the $modelValue. I think there are valid usecases for both.}

Yes, there are definitely both approaches in the code base. But for example the most recent change that touches this, are the 'ng-empty', and 'ng-not-empty' classes, which are set based on the result of $ctrl.isEmpty(viewValue). https://github.com/angular/angular.js/blob/master/src/ng/directive/ngModel.js#L323
I'd rather make all built-in validators use isEmpty(viewValue), because technically all validators validate what the user has entered, even if we parse it a Date or Number.

I haven't thought that through tbh, but I see merit in both cases. Fwiw, the initial intent seems to be to call it against the $modelValue, as indicated by the default implementation (which checks for undefined, null, NaN - values that can hardly appeat as $viewValues).

Imo, the way someone decides to "interpret" the value - i.e. through custom parsers - should be taken into account if possible. But, I understand there are "technical" difficulties with this approach, so checking against the $viewValue might be preferrable.

Most importantly, we should be consistent (if we find ourselves needing to call $isEmpty with both values, it is probably an indication that we need to rethink the API - maybe have two separate methods) and communicate it well through docs.

FWIW, the required validator calls $isEmpty with the viewValue:

angular.js/src/ng/directive/validators.js

Line 70 in cd2f6d9

return !attr.required || !ctrl.$isEmpty(viewValue);

Though I agree that it's a problem in general that validators sometimes validate the viewValue and sometimes the modelValue.

gkalpak · 2016-07-27T11:20:35Z

I just found out that you can make an input[range] invalid. Changing the step at runtime (while having an off-step value) makes it invalid: it sets the validity.stepMismatch flag to true (but keeps the previous value).

I think we need some tests with step.

Narretz · 2016-07-27T19:06:02Z

I left step out, because I wanted to get it to work with min/max first. I'd like to work on step for number/range after this is merged

gkalpak · 2016-07-28T08:16:02Z

I'd like to work on step for number/range after this is merged

I have mixed feelings about this 😃 I would rather not release input[range] without step support.

Btw, here is a browser inconsistency (probably a Chrome bug) I found out:

According to (my understanding of) the spec, browsers should automatically adjust the value whenevr step changes, in order to ensure that there is no step mismatch. When changing the step to a string value (e.g. input.step = '2'), both Chrome and Firefox follow the spec. When using a number (instead of a string), although both browsers convert step to its string representation, only Firefox adjust the value and makes the element valid again. Chrome leaves the previous value and sets the validity.stepMismatch error. E.g.:

<input type="range" step="10" />

console.log(input.step)             // --> '10'
console.log(input.value)            // --> '50'
console.log(input.validity.valid)   // --> true

input.step = '15';
console.log(input.step)             // --> '15'
console.log(input.value)            // --> '45'
console.log(input.validity.valid)   // --> true

input.step = 20;
console.log(input.step)             // --> '20'
console.log(input.value)            // --> Firefox: '40'  |  Chrome: '45'
console.log(input.validity.valid)   // --> Firefox: true  |  Chrome: false (stepMismatch)

Narretz · 2016-07-28T16:28:55Z

Okay, step support should be in the same release as the basic support. But it doesn't have to be in the same commit.

gkalpak · 2016-07-29T10:19:59Z

src/ng/directive/input.js

    } else {
      // TODO(matsko): implement validateLater to reduce number of validations
      ctrl.$validate();
    }
  }

-  var minAttrType = isDefined(attr.min) ? 'min' : attr.ngMin ? 'ngMin' : false;
+  var minAttrType = isDefined(attr.ngMin) ? 'ngMin' : isDefined(attr.min) ? 'min' : false;


The same change is necessary for max below.

gkalpak · 2016-07-29T10:44:18Z

LGTM (the travis failures are not related to this PR and have been fixed on master).
👍

@cironunes

Thanks to @cironunes for the initial implementation in angular#9715 Adds support for binding to input[range] with the following behavior / features: - Like input[number], it requires the model to be a Number, and will set the model to a Number - it supports setting the min/max values via the min/max and ngMin/ngMax attributes - it follows the browser behavior of never allowing an invalid value. That means, when the browser converts an invalid value (empty: null, undefined, false ..., out of bounds: greater than max, less than min) to a valid value, the input will in turn set the model to this new valid value via $setViewValue. -- this means a range input will never be required and never have a non-Number model value, once the ngModel directive is initialized. -- this behavior is supported when the model changes and when the min/max attributes change in a way that prompts the browser to update the input value. -- ngMin / ngMax do not prompt the browser to update the values, as they don't set the attribute values. Instead, they will set the min / max errors when appropriate - browsers that do not support input[range] (IE9) handle the input like a number input (with validation etc.) Closes angular#5892 Closes angular#9715 Close angular#14870

@cironunes

Thanks to @cironunes for the initial implementation in angular#9715 Adds support for binding to input[range] with the following behavior / features: - Like input[number], it requires the model to be a Number, and will set the model to a Number - it supports setting the min/max values via the min/max and ngMin/ngMax attributes - it follows the browser behavior of never allowing an invalid value. That means, when the browser converts an invalid value (empty: null, undefined, false ..., out of bounds: greater than max, less than min) to a valid value, the input will in turn set the model to this new valid value via $setViewValue. -- this means a range input will never be required and never have a non-Number model value, once the ngModel directive is initialized. -- this behavior is supported when the model changes and when the min/max attributes change in a way that prompts the browser to update the input value. -- ngMin / ngMax do not prompt the browser to update the values, as they don't set the attribute values. Instead, they will set the min / max errors when appropriate - browsers that do not support input[range] (IE9) handle the input like a number input (with validation etc.) Closes angular#5892 Closes angular#9715 Close angular#14870

googlebot · 2016-07-29T12:02:21Z

CLAs look good, thanks!

googlebot · 2016-07-29T12:02:21Z

CLAs look good, thanks!

Narretz · 2016-07-29T12:29:46Z

Thanks for the review, @gkalpak ! And thanks again @cironunes for the initial implementation.

@cironunes

Thanks to @cironunes for the initial implementation in #9715 Adds support for binding to input[range] with the following behavior / features: - Like input[number], it requires the model to be a Number, and will set the model to a Number - it supports setting the min/max values via the min/max and ngMin/ngMax attributes - it follows the browser behavior of never allowing an invalid value. That means, when the browser converts an invalid value (empty: null, undefined, false ..., out of bounds: greater than max, less than min) to a valid value, the input will in turn set the model to this new valid value via $setViewValue. -- this means a range input will never be required and never have a non-Number model value, once the ngModel directive is initialized. -- this behavior is supported when the model changes and when the min/max attributes change in a way that prompts the browser to update the input value. -- ngMin / ngMax do not prompt the browser to update the values, as they don't set the attribute values. Instead, they will set the min / max errors when appropriate - browsers that do not support input[range] (IE9) handle the input like a number input (with validation etc.) Closes #5892 Closes #9715 Close #14870

mpromonet · 2016-08-03T17:09:09Z

Maybe I misunderstand, but even if it changes the behaviour, this PR doesnot fix the issue #6726.
Using a simple sample from How to initialize the value of an input[range] using AngularJS when value is over 100 :

    <html ng-app="App">
    <head>
        <script src="https://code.angularjs.org/snapshot/angular.min.js"></script>
        <script>
            angular.module('App', ['App.controllers']);
            angular.module('App.controllers', []).controller('AppController', function($scope) {
                $scope.model = {'min':50, 'max':150, 'value':150};
            });     
        </script>
    </head>
    <body ng-controller="AppController" >
        {{model.min}}<input type="range" min="{{model.min}}" max="{{model.max}}"   ng-model="model.value" value="{{model.value}}"  />{{model.max}}<br/>
        value:{{model.value}}
    </body>
    </html>

Before the commit 9130166 it gives :

value = 150 but the slider is at 100.

After it gives :

value = 100 but the slider is at 100.

The 2 views of value are now consistent, but the value and the slider should be at 150 as it is in the range [50,150].

I tried to change order of min, max, value, but it seems to have no effects.
Is there a way to make this simple case setting min, max and value and showing the slider at the right place ?

Narretz · 2016-08-03T20:31:50Z

@mpromonet Or it's a bug ... (it's a bug). Thanks for testing and reporting.

Narretz added type: feature component: forms frequency: high severity: broken expected use labels Jul 5, 2016

Narretz added this to the 1.5.x milestone Jul 5, 2016

googlebot added the cla: no label Jul 5, 2016

Narretz force-pushed the feat-input-range branch 2 times, most recently from f104808 to 828f286 Compare July 7, 2016 08:11

gkalpak reviewed Jul 8, 2016
View reviewed changes

roryokane reviewed Jul 23, 2016
View reviewed changes

gkalpak reviewed Jul 27, 2016
View reviewed changes

gkalpak reviewed Jul 29, 2016
View reviewed changes

Narretz force-pushed the feat-input-range branch from 4c05f61 to 1001d76 Compare July 29, 2016 11:58

Narretz force-pushed the feat-input-range branch from 1001d76 to 807622e Compare July 29, 2016 12:02

googlebot added cla: yes and removed cla: no labels Jul 29, 2016

Narretz changed the title ~~wip: feat(input): add support for input[type=range]~~ feat(input): add support for input[type=range] Jul 29, 2016

Narretz merged commit 9130166 into angular:master Jul 29, 2016

Narretz deleted the feat-input-range branch July 29, 2016 12:29

Narretz mentioned this pull request Jul 30, 2016

feat(input[range]): support step #14970

Closed

3 tasks

Narretz mentioned this pull request Aug 3, 2016

Input range doesn't respect interpolated min / max on initialization #14982

Closed

feat(input): add support for input[type=range] #14870

feat(input): add support for input[type=range] #14870

Conversation

Narretz commented Jul 5, 2016

googlebot commented Jul 5, 2016

googlebot commented Jul 5, 2016

Narretz commented Jul 8, 2016

gkalpak commented Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

gkalpak Jul 8, 2016 • 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

gkalpak commented Jul 8, 2016

Narretz commented Jul 8, 2016

gkalpak commented Jul 8, 2016

Narretz commented Jul 11, 2016

gkalpak commented Jul 11, 2016

Narretz commented Jul 11, 2016

gkalpak commented Jul 11, 2016

Narretz commented Jul 11, 2016

petebacondarwin commented Jul 11, 2016

roryokane Jul 23, 2016 • 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

gkalpak commented Jul 27, 2016

Narretz commented Jul 27, 2016

gkalpak commented Jul 28, 2016 • edited Loading

Narretz commented Jul 28, 2016

Choose a reason for hiding this comment

gkalpak commented Jul 29, 2016 • edited Loading

googlebot commented Jul 29, 2016

googlebot commented Jul 29, 2016

Narretz commented Jul 29, 2016

mpromonet commented Aug 3, 2016 • edited Loading

Narretz commented Aug 3, 2016

gkalpak commented Jul 8, 2016 •

edited

Loading

gkalpak Jul 8, 2016 •

edited

Loading

roryokane Jul 23, 2016 •

edited

Loading

gkalpak commented Jul 28, 2016 •

edited

Loading

gkalpak commented Jul 29, 2016 •

edited

Loading

mpromonet commented Aug 3, 2016 •

edited

Loading