Preserve browser input behavior.

[jdalton]

This PR closes #7328. In IE/Edge if you set the value of a password input it knocks off any of the helpers like the unmasker and save-password-prompt:

I basically just wrapped the bulk of setValueForProperty in a condition,
('' + node[propName]) !== ('' + value), which is similar to the old HAS_SIDE_EFFECTS check.

If it's too aggressive or needs to be scoped down, that's cool too.

[sophiebits]

Is that code really setting value? My expectation is that input values are only set by these lines, which have the check:

react/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js

Lines 183 to 186 in da021ca

	// To avoid side effects (such as losing text selection), only set value if changed
	if (newValue !== node.value) {
	node.value = newValue;
	}

[jdalton]

@spicyj

Is that code really setting value?

Yeah, I believe so. I'll create a screencast to walk through it tomorrow though.

[syranide]

react/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js

Line 66 in da021ca

value: value != null ? value : inst._wrapperState.initialValue,

Perhaps I'm reading it wrong, but it seems that even for controlled inputs we're updating the value in props which is then applied by DOMPropertyOperations. If that is true then that seems like the core problem (and a bug), value should only be updated directly by ReactDOMInput logic.

[sebmarkbage]

@syranide I think the other path sets the attribute instead of the property now. Which was controversial but probably intentional at the time.

I think the idea was that the value attribute was supposed to match up the value property. Since the browser won't update the value attribute due to a change, we end up updating it ourselves. Which then causes this issue.

IMO, we should revert the idea that attributes should track properties. People expect it to all the time, but it's just not how the DOM works.

[jdalton]

@spicyj

Is that code really setting value?

Yeah, I believe so. I'll create a screencast to walk through it tomorrow though.

Based on the repro from #7328 using the latest master branch with and without this PR.

I know there's discussion about reworking bits here. On the up side this also reduces unnecessary DOM mutations for attributes 😄

[nhunzaker]

It's possible I have this wrong, but I think I went through sort of the same issue in #7359 with Chrome/Safari's number input.

Updating the value attribute, or touching the value property at all can cause the input to drop decimal places or clear the value unexpectedly. My PR attempts to avoid touching the value attribute/property as much as possible. I'm curious if there might be overlap between these PRs.

[syranide]

@nhunzaker I wouldn't say so, this is a case where trying to be nice and updating the attribute to reflect the current property value backfires.

[nhunzaker]

this is a case where trying to be nice and updating the attribute to reflect the current property value backfires

Right. This also backfires for number inputs in Safari/Chrome, and to a lesser extend email inputs. Something I'm working around in #7359 to maintain the current behavior in React as best possible (only update the attribute when the input isn't focused, to at least prevent the issue while the user is typing).

I don't want to distract too much from the core problem addressed by this PR, but I think it touches on a more general challenge with syncing the value attribute with the value property.

Re: @sebmarkbage's comments:

IMO, we should revert the idea that attributes should track properties. People expect it to all the time, but it's just not how the DOM works.

I totally agree. Seems like the only thing to catch would be form.reset() causing controlled inputs to get out of sync with React. This, I believe, is the main reason attributes are synced with properties; at least for inputs.

[nhunzaker]

Wrote a test case for this here: #9269

[gaearon]

Superseded by #11534.