fix(ngValue): set value property instead of attr #14031

m-amr · 2016-02-13T13:46:38Z

in case of input element set value using property instead of attribute
fixed #13984

gkalpak · 2016-04-11T10:45:54Z

src/ng/directive/input.js

+           *  so we set both the value attribute and property here to avoid this problem.
+           */
+          attr.$set('value', value);
+          elm[0].value = value;


Could you extract these two lines (along with the comment above) into a helper function to avoid duplication.

gkalpak · 2016-04-11T10:49:19Z

The docs also need to be updated to reflect the changes.

I would still add a breaking change note, since we are breaking a (documented-ish) assumption.
I can't think of any usecase, where this change would indeed break an app, but it is a breaking change.

…ribute only input element reads the value from value property and override the value attribute so we need to set both Closes angular#14031 Closes angular#13984 Breaking Change: NgValue is no longer updating the value attribute only when the model change, because input element reads the value from value attribute only in case of value property has no value but when value property changed the input element will stop reading the value from value attribute and read it form the value propery. so the input value in the gui will not equal the value of the model, becuase the model update the value attribute only. Now ngValue updates both of value attribute and value property when the model change This shouldn't affect any application becuase this is a bug and input value in the gui should be updated when the model change

…ribute only input element reads the value from value property and override the value attribute so we need to set both Closes angular#14031 Closes angular#13984 BREAKING CHANGE: NgValue is no longer updating the "value attribute" only when the model change, because input element reads the value from "value attribute" only in case of "value property" has no value but when "value property" changed the input element will stop reading the value from "value attribute" and read it form the "value property". so the input value in the gui will not equal the value of the model, becuase the model update the "value attribute" only. Now ngValue updates both of "value attribute" and "value property" when the model change This shouldn't affect any application becuase this is a bug and input value in the gui should be updated when the model change

m-amr · 2016-04-11T23:37:05Z

@gkalpak

I have extracted a method to update the value to prevent code duplication .
I have updating the commit message with the breaking change.
I can't found anything to be update in the documentation.
because the default behaviour of the ng-value is to update the input when the model change and this is a bug. please tell me if documentation is need to be updated ?

Thanks for your feedback.

…ribute only input element reads the value from value property and override the value attribute so we need to set both Closes angular#14031 Closes angular#13984 BREAKING CHANGE: NgValue is no longer updating the "value attribute" only when the model change, because input element reads the value from "value attribute" only in case of "value property" has no value but when "value property" changed the input element will stop reading the value from "value attribute" and read it form the "value property". so the input value in the gui will not equal the value of the model, becuase the model update the "value attribute" only. Now ngValue updates both of "value attribute" and "value property" when the model change This shouldn't affect any application becuase this is a bug and input value in the gui should be updated when the model change

gkalpak · 2016-04-13T08:43:59Z

WRT to the docs, e.g. somewhere it says that the resulting "value will be bound to the value attribute of the input element", which is not accurate anymore.

The docs need a more thorough make-over anyway, because they mention option and input[radio] elements, but with this change ngValue can be used for more purposes, so I can take care of the docs update while mergin or later.

It LGTM.
@Narretz, since you were working on some ngValue stuff, do you have any objection/concerns about this ?

Narretz · 2016-04-13T11:09:23Z

I would like to see the setter code to be more precise in terms of which elements need attribute set and which need property set. So basically the setter fn should be set up based on the element type. I assume the value property should be set for textarea and input[text], while option and input[radio|checkbox] are good with attribute only.
And the docs must be updated. Atm, the docs are pretty explicit that the directive is only for radio/checkbox and option elements.
So the change is actually a feature and not a fix.

m-amr · 2016-04-13T20:24:38Z

@Narretz I think that the setter is setting the property in case of boolean values only, i tried ti fix it in this pull request #14023

Narretz · 2016-04-13T21:22:56Z

@m-amr I'm not sure I follow. Input elements / controls cannot have boolean values. Their values are always string values. Angular models are a different situation, but they generally follow this rule, with exceptions.

…ribute only input element reads the value from value property and override the value attribute so we need to set both Closes angular#14031 Closes angular#13984 BREAKING CHANGE: NgValue is no longer updating the "value attribute" only when the model change, because input element reads the value from "value attribute" only in case of "value property" has no value but when "value property" changed the input element will stop reading the value from "value attribute" and read it form the "value property". so the input value in the gui will not equal the value of the model, becuase the model update the "value attribute" only. Now ngValue updates both of "value attribute" and "value property" when the model change This shouldn't affect any application becuase this is a bug and input value in the gui should be updated when the model change

m-amr · 2016-04-18T19:34:14Z

@Narretz @gkalpak
Could you please give it another look, i have done the following

use prop function to set the value instead of element.value
remove redundant code from tests
i had updated the docs

Thanks.

…ribute only input element reads the value from value property and override the value attribute so we need to set both Closes angular#14031 Closes angular#13984 BREAKING CHANGE: NgValue is no longer updating the "value attribute" only when the model change, because input element reads the value from "value attribute" only in case of "value property" has no value but when "value property" changed the input element will stop reading the value from "value attribute" and read it form the "value property". so the input value in the gui will not equal the value of the model, becuase the model update the "value attribute" only. Now ngValue updates both of "value attribute" and "value property" when the model change This shouldn't affect any application becuase this is a bug and input value in the gui should be updated when the model change

gkalpak · 2016-04-22T10:16:33Z

src/ng/directive/input.js

@@ -1701,6 +1701,11 @@ var CONSTANT_VALUE_REGEXP = /^(true|false|\d+)$/;
 * so that when the element is selected, the {@link ngModel `ngModel`} of that element is set to
 * the bound value.
 *
+ * It can be used to achieve one-way binding of a given expression to the value of
+ * {@link input[text] `input[text]`} or {@link textarea `textarea`}. so that when the expression change


Why only input[text]. Won't it work with any input type ?

I have to change it to be input type

gkalpak · 2016-04-22T10:24:24Z

I would like to see the setter code to be more precise in terms of which elements need attribute set and which need property set. So basically the setter fn should be set up based on the element type. I assume the value property should be set for textarea and input[text], while option and input[radio|checkbox] are good with attribute only.

I'd rather set both and not worry about browser quirks (unless this "strategy" would lead to problems).
@Narretz, do you know of any usecase, where setting both would be a problem ?

…ribute only input element reads the value from value property and override the value attribute so we need to set both Closes angular#14031 Closes angular#13984 BREAKING CHANGE: NgValue is no longer updating the "value attribute" only when the model change, because input element reads the value from "value attribute" only in case of "value property" has no value but when "value property" changed the input element will stop reading the value from "value attribute" and read it form the "value property". so the input value in the gui will not equal the value of the model, becuase the model update the "value attribute" only. Now ngValue updates both of "value attribute" and "value property" when the model change This shouldn't affect any application becuase this is a bug and input value in the gui should be updated when the model change

gkalpak · 2016-04-25T17:35:55Z

src/ng/directive/input.js

@@ -1701,6 +1701,10 @@ var CONSTANT_VALUE_REGEXP = /^(true|false|\d+)$/;
 * so that when the element is selected, the {@link ngModel `ngModel`} of that element is set to
 * the bound value.
 *
+ * It can be used to achieve one-way binding of a given expression to the value of
+ * {@link input `input`}. so that when the expression change the element is set to the bound value.


input --> an element
. --> ,

…ribute only input element reads the value from value property and override the value attribute so we need to set both Closes angular#14031 Closes angular#13984 BREAKING CHANGE: NgValue is no longer updating the "value attribute" only when the model change, because input element reads the value from "value attribute" only in case of "value property" has no value but when "value property" changed the input element will stop reading the value from "value attribute" and read it form the "value property". so the input value in the gui will not equal the value of the model, becuase the model update the "value attribute" only. Now ngValue updates both of "value attribute" and "value property" when the model change This shouldn't affect any application becuase this is a bug and input value in the gui should be updated when the model change

Narretz · 2016-04-26T15:48:04Z

@gkalpak I don't know of any way this could cause problems. I was just conservative, I guess.
The BC notice needs some love, because it's a bit hard to understand, otherwise LGTM.

…ue attribute Input elements use the value attribute as their default value if the value property is not set. Once the value property has been set (by adding input), it will not react to changes to the value attribute anymore. Setting both attribute and property fixes this behavior, and makes it possible to use ngValue as a one-way bind. Closes angular#14031 Closes angular#13984 POSSIBLE BREAKING CHANGE: `ngValue` now also sets the value *property* of its element. Previously, it would only set the value *attribute*. This allows `ngValue` to be used as a one-way binding mechanism on `input[text]` and `textarea` elements without `ngModel`. Previously, these input types would not update correctly when only the value attribute was changed. This change should not affect any applications, as `ngValue` is mainly used on `input[radio]` and `option` elements, both of which are unaffected by this change.

googlebot added the cla: yes label Feb 13, 2016

m-amr mentioned this pull request Feb 13, 2016

fix(ngValue): set value property instead of attribute #14023

Closed

m-amr force-pushed the fix_ng-value_locally branch from a98660c to c179cce Compare February 13, 2016 14:04

gkalpak added component: $compile type: bug component: forms frequency: low needs: review severity: confusing and removed component: $compile labels Feb 13, 2016

gkalpak added this to the Backlog milestone Feb 13, 2016

gkalpak reviewed Apr 11, 2016
View reviewed changes

gkalpak added needs: work and removed needs: review labels Apr 11, 2016

m-amr force-pushed the fix_ng-value_locally branch 2 times, most recently from 0dc6c46 to 7e84dd1 Compare April 11, 2016 23:21

m-amr force-pushed the fix_ng-value_locally branch from 7e84dd1 to 5080d7c Compare April 11, 2016 23:22

m-amr force-pushed the fix_ng-value_locally branch from 5080d7c to 8917b22 Compare April 11, 2016 23:28

m-amr force-pushed the fix_ng-value_locally branch from 8917b22 to 6c1ae85 Compare April 11, 2016 23:38

gkalpak added the needs: breaking change label Apr 13, 2016

m-amr force-pushed the fix_ng-value_locally branch from fa2a50b to aad98d8 Compare April 18, 2016 19:33

m-amr force-pushed the fix_ng-value_locally branch from aad98d8 to c9d2a9d Compare April 18, 2016 19:36

m-amr force-pushed the fix_ng-value_locally branch from c9d2a9d to 9bdc680 Compare April 18, 2016 19:37

gkalpak reviewed Apr 22, 2016
View reviewed changes

m-amr force-pushed the fix_ng-value_locally branch from 9bdc680 to 092ef36 Compare April 22, 2016 13:19

m-amr force-pushed the fix_ng-value_locally branch from 092ef36 to a164453 Compare April 22, 2016 13:20

gkalpak reviewed Apr 25, 2016
View reviewed changes

m-amr force-pushed the fix_ng-value_locally branch from a164453 to dbcf123 Compare April 25, 2016 22:26

m-amr force-pushed the fix_ng-value_locally branch from dbcf123 to 24b7df6 Compare April 25, 2016 22:28

m-amr force-pushed the fix_ng-value_locally branch from 24b7df6 to 6f8aa1d Compare April 25, 2016 22:34

Narretz self-assigned this Jul 28, 2016

Narretz closed this in e6afca0 Aug 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(ngValue): set value property instead of attr #14031

fix(ngValue): set value property instead of attr #14031

Uh oh!

m-amr commented Feb 13, 2016

Uh oh!

gkalpak Apr 11, 2016

Uh oh!

gkalpak commented Apr 11, 2016

Uh oh!

m-amr commented Apr 11, 2016

Uh oh!

gkalpak commented Apr 13, 2016 •

edited

Loading

Uh oh!

Narretz commented Apr 13, 2016

Uh oh!

m-amr commented Apr 13, 2016

Uh oh!

Narretz commented Apr 13, 2016

Uh oh!

m-amr commented Apr 18, 2016 •

edited

Loading

Uh oh!

gkalpak Apr 22, 2016

Uh oh!

m-amr Apr 22, 2016

Uh oh!

gkalpak commented Apr 22, 2016

Uh oh!

gkalpak Apr 25, 2016 •

edited

Loading

Uh oh!

m-amr Apr 25, 2016

Uh oh!

Narretz commented Apr 26, 2016

Uh oh!

Uh oh!

fix(ngValue): set value property instead of attr #14031

fix(ngValue): set value property instead of attr #14031

Uh oh!

Conversation

m-amr commented Feb 13, 2016

Uh oh!

gkalpak Apr 11, 2016

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Apr 11, 2016

Uh oh!

m-amr commented Apr 11, 2016

Uh oh!

gkalpak commented Apr 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Narretz commented Apr 13, 2016

Uh oh!

m-amr commented Apr 13, 2016

Uh oh!

Narretz commented Apr 13, 2016

Uh oh!

m-amr commented Apr 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak Apr 22, 2016

Choose a reason for hiding this comment

Uh oh!

m-amr Apr 22, 2016

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Apr 22, 2016

Uh oh!

gkalpak Apr 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m-amr Apr 25, 2016

Choose a reason for hiding this comment

Uh oh!

Narretz commented Apr 26, 2016

Uh oh!

Uh oh!

gkalpak commented Apr 13, 2016 •

edited

Loading

m-amr commented Apr 18, 2016 •

edited

Loading

gkalpak Apr 25, 2016 •

edited

Loading