Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6e30312
fix(ngValue): setting binding to undefined should update the DOM valu…
frederikprijck Jan 12, 2017
3ae9f0f
fixup! fix(ngValue): setting binding to undefined should update the D…
frederikprijck Jan 12, 2017
db99f4e
fixup! fix(ngValue): setting binding to undefined should update the D…
frederikprijck Jan 12, 2017
a8e2cec
fixup! fix(ngValue): setting binding to undefined should update the D…
frederikprijck Jan 13, 2017
a29ece6
fixup! fix(ngValue): setting binding to undefined should update the D…
frederikprijck Jan 13, 2017
496a2af
fixup! fix(ngValue): setting binding to undefined should update the D…
frederikprijck Jan 13, 2017
ffb122b
fixup! fix(ngValue): setting binding to undefined should update the D…
frederikprijck Jan 13, 2017
011c3a8
fixup! fix(ngValue): setting binding to undefined should update the D…
frederikprijck Jan 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ng/directive/input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...).

Copy link
Contributor Author

@frederikprijck frederikprijck Jan 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

attr.$set('value', value);
}

Expand Down
16 changes: 16 additions & 0 deletions test/ng/directive/inputSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say something like:

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

Copy link
Contributor Author

@frederikprijck frederikprijck Jan 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

var inputElm = helper.compileInput('<input type="text" ng-value="value">');

$rootScope.$apply('value = \'something\'');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


expect(inputElm[0].value).toBe('something');
expect(inputElm[0].getAttribute('value')).toBe('something');

$rootScope.$apply(function() {
delete $rootScope.value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

expect(inputElm[0].value).toBe('');
expect(inputElm[0].getAttribute('value')).toBe(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

});

they('should update the $prop "value" property and attribute after the bound expression changes', {
input: '<input type="text" ng-value="value">',
textarea: '<textarea ng-value="value"></textarea>'
Expand Down