Fix input range init #14996

Narretz · 2016-08-06T12:27:16Z

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:

This PR fixes 1 bigger and 2 smaller issues:

correctly initialize with interpolated min / max values

The interpolation directive only sets the actual element attribute value after
a digest passed. That means previously the min/max values on input range were
not set when the first $render happened, so the browser would not adjust
the input value according to min/max. This meant the range input and model would
not be initialzed as expected.

With this change, input range will set the actual element attribute value during its own
linking phase, as it is already available on the attrs argument passed to the link fn.

Fixes #14982

correctly sets the range value when min > max. Chrome and IE don't set this correctly in all cases.
removes interpolation from an ng-maxlength attribute in a test (interpolation in an attribute that expects an expression is discouraged)

gkalpak · 2016-08-07T18:13:22Z

src/ng/directive/input.js

@@ -1647,7 +1647,8 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) {
      // IE11 doesn't set the el val correctly if the maxVal is greater than the element value


I just noticed this comment should say "less than" (instead of "greater than"), right?

Narretz · 2016-08-09T08:31:08Z

@gkalpak Travis finally green, you may review this again :>

gkalpak · 2016-08-09T16:25:28Z

src/ng/directive/input.js

+
+    // This initalizes the min/max value, so that non-support browsers validate with the correct
+    // values during the initial $render
+    changeFn(initialAttrValue);


initialAttrValue will be undefined for ngMin/ngMax. Is this intended?

It is not clear why this function would do different things for ngXyz vs xyz attributes. If this is intended, you could add a short comment explaining what is going on.

At this point we really only need to set the element attribute if we have min / max and the browser supports range. I've removed the call to changeFn as its use isn't tested currently.
The actual issue is that all validators that are based on observation if interpolation or ngX attributes only set their values after a digest. That means when the input renders these validators' valuea aren't defined. When the observers fire we will validate again with the correct values but this causes the model to be unset if the value is invalid. Imo this should still count as initialization so that shouldn't happen. The other thing is that we validate too often (once for every observer).
I'll open an issue for part 1 of this.

gkalpak · 2016-08-12T10:28:44Z

LGTM

…alues The interpolation directive only sets the actual element attribute value after a digest passed. That means previously, the min/max values on input range were not set when the first $render happened, so the browser would not adjust the input value according to min/max. This meant the range input and model would not be initialzed as expected. With this change, input range will set the actual element attribute value during its own linking phase, as it is already available on the attrs argument passed to the link fn. Fixes angular#14982

Since ng prefixed attributes do not set their corresponding element attribute, the range input would always have min = 0, and max = 100 (in supported browsers), regardless of the value in ngMin/ngMax. This is confusing and not very useful, so it's better to not support them at all.

This commit fixes the handling of min/max, and removes support for ngMin/ngMax: min/max: Previously, interpolated min/max values on input range were not set when the first $render happens, because the interpolation directive only sets the actual element attribute value after a digest passes. That means that the browser would not adjust the input value according to min/max and the range input and model would not be initialzed as expected. With this change, input range will set the actual element attribute value during its own linking phase, as it is already available on the attrs argument passed to the link fn. ngMin/ngMax Since ng prefixed attributes do not set their corresponding element attribute, the range input would always have min = 0, and max = 100 (in supported browsers), regardless of the value in ngMin/ngMax. This is confusing and not very useful, so it's better to not support these attributes at all. The commit also fixes a test which used an interpolation inside an attribute that expects an expression. Fixes #14982 PR (#14996)

This commit fixes the handling of min/max, and removes support for ngMin/ngMax: min/max: Previously, interpolated min/max values on input range were not set when the first $render happens, because the interpolation directive only sets the actual element attribute value after a digest passes. That means that the browser would not adjust the input value according to min/max and the range input and model would not be initialzed as expected. With this change, input range will set the actual element attribute value during its own linking phase, as it is already available on the attrs argument passed to the link fn. ngMin/ngMax Since ng prefixed attributes do not set their corresponding element attribute, the range input would always have min = 0, and max = 100 (in supported browsers), regardless of the value in ngMin/ngMax. This is confusing and not very useful, so it's better to not support these attributes at all. The commit also fixes a test which used an interpolation inside an attribute that expects an expression. Fixes angular#14982 PR (angular#14996) (reverted from commit b78539b)

This commit fixes the handling of min/max, and removes support for ngMin/ngMax: min/max: Previously, interpolated min/max values on input range were not set when the first $render happens, because the interpolation directive only sets the actual element attribute value after a digest passes. That means that the browser would not adjust the input value according to min/max and the range input and model would not be initialzed as expected. With this change, input range will set the actual element attribute value during its own linking phase, as it is already available on the attrs argument passed to the link fn. ngMin/ngMax Since ng prefixed attributes do not set their corresponding element attribute, the range input would always have min = 0, and max = 100 (in supported browsers), regardless of the value in ngMin/ngMax. This is confusing and not very useful, so it's better to not support these attributes at all. The commit also fixes a test which used an interpolation inside an attribute that expects an expression. Fixes angular#14982 PR (angular#14996)

This commit fixes the handling of min/max, and removes support for ngMin/ngMax: min/max: Previously, interpolated min/max values on input range were not set when the first $render happens, because the interpolation directive only sets the actual element attribute value after a digest passes. That means that the browser would not adjust the input value according to min/max and the range input and model would not be initialzed as expected. With this change, input range will set the actual element attribute value during its own linking phase, as it is already available on the attrs argument passed to the link fn. ngMin/ngMax Since ng prefixed attributes do not set their corresponding element attribute, the range input would always have min = 0, and max = 100 (in supported browsers), regardless of the value in ngMin/ngMax. This is confusing and not very useful, so it's better to not support these attributes at all. The commit also fixes a test which used an interpolation inside an attribute that expects an expression. Fixes #14982 PR (#14996)

icorne · 2016-11-28T13:13:31Z

Will this fix be back-ported to older angular versions?

petebacondarwin · 2016-11-28T13:35:26Z

@icorne - which versions did you have in mind? We have no plans to backport it further back than 1.5.x

icorne · 2016-11-28T13:43:50Z

I'm having trouble figuring out where this was merged into. I'm working on 1.5.x, so that's what I'm interested in. I've tried upgrading from 1.5.5 to 1.5.9 but I still get the issue.

petebacondarwin · 2016-11-28T14:13:35Z

Ahah! Right. OK, it didn't make it into 1.5.9, since that was primarily a security release. It will be in 1.5.10.

gkalpak · 2016-11-28T14:30:44Z

This is already included in 1.5.9 (2e7121b), but support for input[range] is opt-in on 1.5.x in order to avoid a breaking change (see 07b8761 and the docs).

petebacondarwin · 2016-11-28T16:19:23Z

Oops! I am so confused with all the rebasing we did last week that I forgot that we actually did put this in 1.5.9 :-) Thanks @gkalpak

test(validators): remove bad practice from test

21aa003

Narretz added type: bug component: forms frequency: moderate needs: review severity: broken expected use labels Aug 6, 2016

Narretz added this to the 1.5.9 milestone Aug 6, 2016

googlebot added the cla: yes label Aug 6, 2016

Narretz force-pushed the fix-input-range-init branch 2 times, most recently from ba98e9e to f93b09a Compare August 7, 2016 10:50

gkalpak reviewed Aug 7, 2016
View reviewed changes

gkalpak reviewed Aug 9, 2016
View reviewed changes

Narretz added 3 commits August 12, 2016 19:18

fix(input[range]): set correct value when max < min

45f474b

Narretz force-pushed the fix-input-range-init branch from 1dfc7cb to 250801c Compare August 12, 2016 17:21

Narretz merged commit a272a3c into angular:master Aug 12, 2016

Narretz deleted the fix-input-range-init branch August 12, 2016 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix input range init #14996

Fix input range init #14996

Narretz commented Aug 6, 2016 •

edited

Loading

gkalpak Aug 7, 2016

Narretz commented Aug 9, 2016

gkalpak Aug 9, 2016

Narretz Aug 9, 2016

gkalpak commented Aug 12, 2016

icorne commented Nov 28, 2016

petebacondarwin commented Nov 28, 2016

icorne commented Nov 28, 2016

petebacondarwin commented Nov 28, 2016

gkalpak commented Nov 28, 2016

petebacondarwin commented Nov 28, 2016

		@@ -1647,7 +1647,8 @@ function rangeInputType(scope, element, attr, ctrl, $sniffer, $browser) {
		// IE11 doesn't set the el val correctly if the maxVal is greater than the element value

Fix input range init #14996

Fix input range init #14996

Conversation

Narretz commented Aug 6, 2016 • edited Loading

gkalpak Aug 7, 2016

Choose a reason for hiding this comment

Narretz commented Aug 9, 2016

gkalpak Aug 9, 2016

Choose a reason for hiding this comment

Narretz Aug 9, 2016

Choose a reason for hiding this comment

gkalpak commented Aug 12, 2016

icorne commented Nov 28, 2016

petebacondarwin commented Nov 28, 2016

icorne commented Nov 28, 2016

petebacondarwin commented Nov 28, 2016

gkalpak commented Nov 28, 2016

petebacondarwin commented Nov 28, 2016

Narretz commented Aug 6, 2016 •

edited

Loading