Set value by setAttribute(value) for html input box for #4618

[hayesgm]

This is an alternate approach for #5666, using setAttribute(value) instead of defaultValue as a DOM property. Both this and #5666 are fixes for #4618 and are mutually exclusive approaches.

Fixes issue #4618, which allows updates to defaultValue on uncontrolled s by changing element.value via element.setAttribute(value). We move hasSideEffects code into the input and textarea wrappers (each wrapper updates still via element.value = ...). Browsers will reflect changes to setValue(value) on <input> elements unless the user has adjusted the value on an uncontrolled element. This is the approach suggested by @spicyj.

Note: even after this patch, changes to defaultValue on textarea and select elements will not be reflected after the component is mounted.

Here's a simple test of the patch on JSBin: https://jsbin.com/tobuye/10/edit?html,js,output

[sophiebits]

Previously, the string cast happened before this comparison, so value of 3 and node.value of '3' would not trigger a set. We should keep that.

[sophiebits]

It shouldn't be hard to make this work for textarea too, right? I guess select may be a bit harder.

Btw – lint is failing.

[hayesgm]

Fixed linting issues.

[facebook-github-bot]

@hayesgm updated the pull request.

[hayesgm]

@spicyj It's just not as obvious since we still support children and there are some workarounds for IE8 that I don't want to perturb. I'd be happy to give it a go if we think it's important.

Do you think this approach is better than #5666?

[hayesgm]

Also, @spicyj, this removes all need for HAS_SIDE_EFFECTS prop since we bypass DOMPropertyOperations in the one case it was being used. It might be nice to wipe out the code to clean up setValueForProperty.

[sophiebits]

I would like to have consistency between input and textarea here. I guess for textarea we need to change the children/textContent? :\

I do think this is better than #5666 even though it seems like more work.

Yeah, want to do a follow-up PR to remove HAS_SIDE_EFFECTS?

[facebook-github-bot]

@hayesgm updated the pull request.

[hayesgm]

Got this working with <textarea> with similar behavior to <input>. We are forced to set defaultValue (or textContent) as a property. Nothing with setAttribute seemed to take on Chrome. There was a note about IE9 losing selection when mutating textContent. It is probably worth testing this behavior in a VM.

This playground is up-to-date with this change: https://jsbin.com/tobuye/edit?html,js,output

@spicyj Happy to create that PR once this one is done.

[hayesgm]

Any update on this PR? Happy to make any changes required to get this pulled.

[syranide]

@spicyj I think I've said it before, but using attributes instead of properties for value seems really quirky if you ask me. If you type normally into a field then only the value property is updated, this is according to the standard, it's also standard practice to update the property (and not the attribute) for this reason. So as far as I can tell, this breaks with convention, introduces subtle edge-cases for those relying on it (because of timing and not all changes are cought) and makes React controlled inputs behave differently from all other inputs.

[sophiebits]

HTML does not have a concept of switching from <input value="a"> to <input value="b">. If you render from scratch then the property and attribute are both changed. When doing an update, we have to define the semantics and decide whether we want to update the property or the attribute. There are arguments both ways, but "breaks with convention" is not a valid one (except when comparing with the convention that React itself has set so far).

[sophiebits]

@jimfb Do you want to take a look at this one?

[jimfb]

ReactDOM.render returns the DOM node; you don't need to call findDOMNode

[hayesgm]

Went ahead and replaced all of these after a direct call to ReactDOM.render() in ReactDOMInput-test.js and ReactDOMTextarea-test.js

[syranide]

There are arguments both ways, but "breaks with convention" is not a valid one (except when comparing with the convention that React itself has set so far).

@spicyj I don't agree, put an input on a page, type into it and inspect. That's the convention set by the standard/browsers, we will be explicitly going against that and controlled inputs will thus differ from uncontrolled inputs in behavior too. Any change to a controlled input that doesn't emit an event (like autofill) will also cause unexpected behavior as the attribute will then not mirror the property until updated. What is the argument for using attributes?

[jimfb]

I feel like controlled inputs should not take in a defaultValue - that should be illegal. In a controlled world, a button type=reset should have no effect. To reset a controlled component, the click handler of the reset button should update application state. It makes no sense to have both value and defaultValue specified.

Filed #5819

[hayesgm]

Removed this test case to defer to changes in #5819

[jimfb]

I'm inclined to agree with @spicyj on this one:

If you render from scratch then the property and attribute are both changed.

I'm fine with using attributes here, to match the behavior of a fresh render. I left a few comments/nitpicks. Conceptually, this diff looks fine to me ( 👍 ) .

[sophiebits]

I don't agree, put an input on a page, type into it and inspect. That's the convention set by the standard/browsers, we will be explicitly going against that

@syranide Nowhere have we said that updating the value via React should have the same behavior as typing into an input, and I'm not sure why that should be the case either. It is cleaner and simpler conceptually to have it match what you would get on a clean rerender as long as that doesn't cause problems practically.

The motivation for this change is #4618 – React currently fails to update the defaultValue property (which always matches the value attribute) which means it is not possible to change an input's "default value" after initial render, which is used by an <input type="reset"> button.

[syranide]

The motivation for this change is #4618 – React currently fails to update the defaultValue property (which always matches the value attribute) which means it is not possible to change an input's "default value" after initial render, which is used by an button.

OK, that argument I kind of get, but if you're using controlled inputs, I would think the solution is to reset the state, that's the point of controlled inputs. Anyway, that's HTML for you, I get it.

But if that's what we want then why not just set the defaultValue property? Why set the value attribute? I would also think the useful behavior here would be for it to reset to whatever value the input had when it was created unless defaultValue is explicitly set, not reset to the current value (because reset for controlled inputs really is a cheap shortcut).

PS. I also get the desire to have controlled inputs behave as if they're always fresh... but it's odd behavior for HTML. But that's my opinion.

[sophiebits]

But if that's what we want then why not just set the defaultValue property? Why set the value attribute?

@syranide .defaultValue and the value attribute are directly linked:

[syranide]

@syranide .defaultValue and the value attribute are directly linked:

@spicyj If you set the value attribute yes, not if you're setting the value + defaultValue property...?

[sophiebits]

@syranide My screenshot shows exactly that.

[facebook-github-bot]

@hayesgm updated the pull request.

[hayesgm]

This is now rebased to master. It also properly addresses the (mostly minor) issues raised by @jimfb's review.

[sophiebits]

Hmm this is a behavior change i hadn't anticipated. I was thinking that this PR would be unobservable if you don't have a reset button (or CSS selectors based on the value attribute, etc) but not if this is the behavior. We'll have to think on this probably. Even if this is desirable overall it could break people who expect the other behavior so we need to be cautious.

cc @zpao – what do you think? In order to make <input type="reset" /> work reasonably, this PR changes the value attribute whenever defaultValue is set (equivalent to the .defaultValue property), but if there hasn't been any user interaction then doing so also changes the actual value.

[gaearon]

Don’t we want to remove support for <textarea>{children}</textarea> anyway? Might as well do that together with this. (Obv in 16 earliest.)

[sophiebits]

We do (and we've had that warning for a while now so removing it should be easy), but this has the same problem with <textarea defaultValue="gorilla" /> I think.

[gaearon]

Oh, I see. Even if people migrated to defaultValue they’d still rely on the old behavior?

[sophiebits]

I think the only other thing I'm worried about is if this can cause extra cursor jumps when defaultValue is set. Code comments indicate it might be a problem in IE9.

[hayesgm]

@spicyj I'd be happy to test this in an IE9 VM. I'll report back.

As per changing defaultValue changes value (unless it's been affected by the user), I thought that was actually part of the spec that we wanted to change. To me, it was always odd behavior that "defaultValue" was a one-and-done. It makes logical sense that updating defaultValue updates the input, so long as it was never user manipulated. I agree, however, this goes against the current documentation and should be considered a breaking change (albeit minor).

[hayesgm]

For my first look at IE9, it doesn't look like there's any issue with stealing focus / cursor.

Internet Explorer (11 included) looks to not set the value of a <textarea> based on defaultValue. As per the MSDN docs:

The value of the property can be changed programmatically, but doing so has no effect on the appearance of the object or the submitted value. It does, however, change the initial value of the object when the form is reset.

As evidenced in this example, Chrome/FF/Safari all display the "defaultValue" in lieu of an unset value in both and <textarea> elements. Internet Explorer (9/10/11) will display the defaultValue on elements, but leaves <textarea> blank. In any case, all browsers reset the elements to the correct value after a reset.

[jimfb]

@spicyj Does @hayesgm's response address your concerns? Are we ready to go on this one?

[sophiebits]

Not really. We shouldn't do this switch if it introduces behavior differences between browsers. I'm not even sure this is desirable and would feel more comfortable if the effective value did not change when changing defaultValue. I think that might be possible if we save the old value before each update and restore it after.

[hayesgm]

I agree here that there's an issue on cross-browser behavior with Internet Explorer for this PR. At this point, can we clarify what behavior we want and which approach (if any) we believe can achieve that. This conversation might be better placed in the original issue thread than this PR.

[gaearon]

I’m going to close this out because it doesn’t merge cleanly anymore and, more importantly, there is no consensus on how to proceed. Both this PR and #5666 can exist as a reference for both approaches but let’s continue the discussion in #4618 to figure out what behavior we actually want, now that we know drawbacks of either implementation.

Thank you for your work on this!

[jimfb]

We might want to keep this one open, at least until we come to a conclusion on v15. I think this PR is a perfectly reasonable fix for #6119 and #6219, both of which are blocking v15. As an added bonus, it happens to also fix #4618 :P.

[zpao]

Note for if we come back to this: no need for type="text" on textareas.

[gaearon]

I think this PR is a perfectly reasonable fix for #6119 and #6219, both of which are blocking v15. As an added bonus, it happens to also fix #4618 :P.

Nevertheless, unless I’m mistaken, it introduces a breaking change in how textarea treats changes to its children which can affect many apps. We have not warned about the new behavior anywhere so this might complicate adoption of 15.

More info on behavior change: #5680 (comment).

[syranide]

If HAS_SIDE_EFFECTS is now unused then the corresponding logic should be removed from DOMPropertyOperations as well? (PS. Also, this is outdated and needs to be rebased)

[jimfb]

Already mentioned here: #5680 (comment), resolution was that HAS_SIDE_EFFECTS can be handled as a followup PR.

[jimfb]

superseded by #6406.

@hayesgm Thanks for kicking off this whole change with your PR. This was super helpful!