fix(ngValue): setting binding to undefined should update the DOM value to an empty string #15605

frederikprijck · 2017-01-12T22:32:33Z

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)
Setting the binding to undefined calls jqLite/jQuery's prop() function, passing undefined as the second argument: element.prop('value', undefined).
This is identical to element.prop('value') and will retrieve the dom value instead of updating it.

See: #15603

What is the new behavior (if this is a feature change)?

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:

…e to an empty string Previously, when setting the binding to undefined would call jqLite/jQuery's `prop()` function, passing undefined as the second argument: `element.prop('value', undefined)`. This is identical to `element.prop('value') and will retrieve the dom value instead of updating it. This commit will prevent from undefined being passed as a second argument by passing `null` instead. Fixes: 15603

…OM value to an empty string

gkalpak

IE doesn't approve yor changes 😃

gkalpak · 2017-01-13T11:01:53Z

test/ng/directive/inputSpec.js

+      });
+
+      expect(inputElm[0].value).toBe('');
+      expect(inputElm[0].getAttribute('value')).toBe(null);


It turns out that in IE you can't remove the value attribute (🥂). This is not related to this issue though and at this point I don't think it is worth fixing (since nobody has complained about it and it is actually a browser issue).

You can skip this expectation in IE, by checking the msie property:

// Support: IE 9-11 // In IE it is not possible to remove the `value` attribute from an input element. if (!msie) { expect(inputElm[0].getAttribute('value')).toBeNull(); }

Should I do it this way, or just get rid of the except as this has nothing to do with my change ?

I prefer to keep the expectation on other browsers (even if it is not directly related to this PR, it is still relevant for the added test).

gkalpak · 2017-01-13T11:17:02Z

src/ng/directive/input.js

@@ -2142,7 +2142,7 @@ var ngValueDirective = function() {
   *  makes it possible to use ngValue as a sort of one-way bind.
   */
  function updateElementValue(element, attr, value) {
-    element.prop('value', value);
+    element.prop('value', angular.isUndefined(value) ? null : value);


Another idea (to avoid the arbitrary conversion to null) is to change to element[0].value = value. (I am not sure what is better.) It will not matter in most cases (e.g. when used with native inputs), only if someone is using it with a custom input.

It turns out that in IE9 input.value = null results in input.value === 'null' (same for undefined). So we have a problem there.

It matters in cases where value in always undefined (so updateElementValue will only run once at the beginning). Previously, element.prop('value', undefined) would be ignored (and the initial value will be kept), but now it will be set to 'null' (or 'undefined').

We could check for IE9 and set the value to '' instead (and also add a "known issue" in the docs):

var propValue = angular.isDefined(value) ? value : (msie === 9) ? '' : null; element.prop('value', propValue);

(It is not ideal, but it is a good middleground imo - plus it will only affect IE9 users.)

I will update the PR with the msie check and using isDefined instead of isUndefined.

If I get this straight, using element[0].value = propValue should be possible aswell? The difference is we're not using jQuery/jqLite in this case, which may be a better idea but I have no idea what you guys prefer...

I am 99% sure it won't make a difference in this particular case, but let's stick to .prop() (because...it doesn't make a difference).

BTW, I forgot to mention, you don't need the angular. prefix; just is[Un]Defined(...).

Fixed!

So for the docs part:

Setting the value to undefined will update the DOM value to null.

IE9: updating the DOM value to null results in 'null'. Setting the value to undefined will update the DOM value to '' instead of null.

Leaving out angular. resulted in a broken build. But it looks like it's not related to that ?

gkalpak · 2017-01-13T11:19:17Z

test/ng/directive/inputSpec.js

+    it('should update the dom "value" to "" and attribute to null when the binding is set to undefined', function() {
+      var inputElm = helper.compileInput('<input type="text" ng-value="value">');
+
+      $rootScope.$apply('value = \'something\'');


Nit: Feel free to use " inside string expressions 😁 (E.g.: 'value = "something"')

gkalpak · 2017-01-13T11:19:57Z

test/ng/directive/inputSpec.js

+      expect(inputElm[0].getAttribute('value')).toBe('something');
+
+      $rootScope.$apply(function() {
+        delete $rootScope.value;


Nit: This could be $rootScope.$apply('value = undefined'). _{(just sayin'...}

I'm not going to fix this nit as I prefer delete. If you prefer me to use undefined anyway, I'll update it!

gkalpak · 2017-01-13T11:23:06Z

test/ng/directive/inputSpec.js

@@ -4211,6 +4211,22 @@ describe('input', function() {
      expect(inputElm[0].getAttribute('value')).toBe('something');
    });

+    it('should update the dom "value" to "" and attribute to null when the binding is set to undefined', function() {


I would say something like:

should clear the value attribute and property when the value is undefined

The reason why I took this was because this test has a comparable way of describing itself:
https://github.com/angular/angular.js/blob/master/test/ng/directive/inputSpec.js#L4205

So combining my solution with yours, I'd propose:

Should clear the "dom" value property and attribute when the value is undefined.

Yet I do think this being somehow odd, talking about clearing both feels like having the same result for both. However, dom value is set to '' whereas attr is set to null.

Should clear the "dom" value property and attribute when the value is undefined.

👍

Yet I do think this being somehow odd, talking about clearing both feels like having the same result for both. However, dom value is set to '' whereas attr is set to null.

"Clear" is generic and can mean different things for different...things (e.g. property vs attribute). What we clear is an implementation detail - it doesn't have to go to the test description imo.

…OM value to an empty string

frederikprijck · 2017-01-13T12:10:28Z

I'm sorry for the huge amount of fixup's btw ...

gkalpak · 2017-01-13T12:14:04Z

src/ng/directive/input.js

@@ -2142,7 +2142,8 @@ var ngValueDirective = function() {
   *  makes it possible to use ngValue as a sort of one-way bind.
   */
  function updateElementValue(element, attr, value) {
-    element.prop('value', value);
+    var propValue = isDefined(value) ? value : (msie === 9) ? '' : null;


It is a good idea to add a comment here explaining the IE9 issue. Something like:

// Support: IE9 only // In IE9 values are converted to string (e.g. `input.value = null` results in `input.value === 'null'`).

gkalpak · 2017-01-13T12:14:43Z

I'm sorry for the huge amount of fixup's btw ...

I'm sorry for the large amount of comments 😛

…OM value to an empty string

gkalpak · 2017-01-13T12:59:57Z

@Narretz, I could use an extra pair of eyes here 😉

Narretz · 2017-01-16T18:19:54Z

Looks like you already worked out the kinks here :)

In the first commit, you talked about updating the docs because of IE9:

Setting the value to undefined will update the DOM value to null.
IE9: updating the DOM value to null results in 'null'. Setting the value to undefined will update the DOM value to '' instead of null.

I think it doesn't hurt to add this.
However, if we add should we also add code / tests that ensure that we set the value to null and '' (IE9) when setting the value to null? Atm we only so this for undefined.

frederikprijck · 2017-01-16T18:29:28Z

I had a chat with @gkalpak offline to leave out the docs part. As this wasn't part of the reported issue I'm fixing here.

I'm happy to still add that if required tho.

gkalpak · 2017-01-17T10:28:15Z

There are three things that could be documented and one issue that potentially needs fixing:

docs: The fact that we use node.prop = null, when value is undefined.
docs: The fact that we use node.prop = '', when `value is undefined in IE9.
docs: The fact that we node.prop === 'null', when value is null` in IE9.
fix: The fact that node.prop === 'null', when value is null in IE9.

(4) is unrelated to this PR, so no need to discuss further (except for saying that it could be considered a BC and is not worth fixing imo).

WRT (1) and (2): These should not make any difference in the majority of cases (the result will be node.prop === '' anyway), so I am afraid that mentioning them in the docs might be more confusing and "upsetting" than helpful. It didn't work correctly before anyway, so if anything this is an improvement.

WRT (3): This is unrelated to this PR, so documentng shouldn't necessarily be done as part of it. Generally, the issue itself only affects IE9 (so only a small number of users) and strictly speaking it is a browser issue (not an Angular issue); i.e. the same would happen if you did node.prop = null. So, I would say Angular works as expected here.

That said, I don't feel too strongly about any of this - I am fine documenting/fixing (not necessarily in this PR) if @Narretz we should.

Narretz · 2017-01-18T12:53:03Z

@gkalpak Thanks for the thorough write-up. Please go ahead and merge. If we need these details later in the docs, we can just take them from your comment :)

…ndefined Previously, when the expression evaluated to `undefined` the `value` property was not updated. This happened because jqLite/jQuery's `prop(_, undefined)` is treated as a getter, thus not apdating the property. This commit fixes it by setting the property to `null` instead. (On IE9 we use `''` - see inline comments for more info.) Fixes #15603 Closes #15605

…ndefined Previously, when the expression evaluated to `undefined` the `value` property was not updated. This happened because jqLite/jQuery's `prop(_, undefined)` is treated as a getter, thus not apdating the property. This commit fixes it by setting the property to `null` instead. (On IE9 we use `''` - see inline comments for more info.) Fixes angular#15603 Closes angular#15605

googlebot added the cla: yes label Jan 12, 2017

frederikprijck added 3 commits January 12, 2017 23:40

fixup! fix(ngValue): setting binding to undefined should update the D…

3ae9f0f

…OM value to an empty string

fixup! fix(ngValue): setting binding to undefined should update the D…

db99f4e

…OM value to an empty string

fixup! fix(ngValue): setting binding to undefined should update the D…

a8e2cec

…OM value to an empty string

gkalpak suggested changes Jan 13, 2017

View reviewed changes

gkalpak added component: misc core frequency: low needs: work severity: broken expected use type: bug component: forms and removed component: misc core labels Jan 13, 2017

frederikprijck added 3 commits January 13, 2017 12:52

fixup! fix(ngValue): setting binding to undefined should update the D…

a29ece6

…OM value to an empty string

fixup! fix(ngValue): setting binding to undefined should update the D…

496a2af

…OM value to an empty string

fixup! fix(ngValue): setting binding to undefined should update the D…

ffb122b

…OM value to an empty string

gkalpak suggested changes Jan 13, 2017

View reviewed changes

fixup! fix(ngValue): setting binding to undefined should update the D…

011c3a8

…OM value to an empty string

gkalpak approved these changes Jan 13, 2017

View reviewed changes

Narretz added the needs: merging label Jan 18, 2017

gkalpak removed the needs: work label Jan 18, 2017

gkalpak closed this in d85b3f5 Jan 18, 2017

frederikprijck deleted the fix/15603 branch January 30, 2017 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngValue): setting binding to undefined should update the DOM value to an empty string #15605

fix(ngValue): setting binding to undefined should update the DOM value to an empty string #15605

frederikprijck commented Jan 12, 2017 •

edited

Loading

gkalpak left a comment

gkalpak Jan 13, 2017

frederikprijck Jan 13, 2017

gkalpak Jan 13, 2017

gkalpak Jan 13, 2017

frederikprijck Jan 13, 2017

gkalpak Jan 13, 2017

frederikprijck Jan 13, 2017 •

edited

Loading

frederikprijck Jan 13, 2017

gkalpak Jan 13, 2017

gkalpak Jan 13, 2017

frederikprijck Jan 13, 2017

gkalpak Jan 13, 2017

frederikprijck Jan 13, 2017 •

edited

Loading

gkalpak Jan 13, 2017

frederikprijck Jan 13, 2017

frederikprijck commented Jan 13, 2017 •

edited

Loading

gkalpak Jan 13, 2017

gkalpak commented Jan 13, 2017

gkalpak commented Jan 13, 2017

Narretz commented Jan 16, 2017

frederikprijck commented Jan 16, 2017

gkalpak commented Jan 17, 2017

Narretz commented Jan 18, 2017

fix(ngValue): setting binding to undefined should update the DOM value to an empty string #15605

fix(ngValue): setting binding to undefined should update the DOM value to an empty string #15605

Conversation

frederikprijck commented Jan 12, 2017 • edited Loading

gkalpak 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

frederikprijck Jan 13, 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

frederikprijck Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikprijck commented Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

gkalpak commented Jan 13, 2017

gkalpak commented Jan 13, 2017

Narretz commented Jan 16, 2017

frederikprijck commented Jan 16, 2017

gkalpak commented Jan 17, 2017

Narretz commented Jan 18, 2017

frederikprijck commented Jan 12, 2017 •

edited

Loading

frederikprijck Jan 13, 2017 •

edited

Loading

frederikprijck Jan 13, 2017 •

edited

Loading

frederikprijck commented Jan 13, 2017 •

edited

Loading