Don't detach value from defaultValue for submit/reset inputs

[zpao]

Fixes #7179

This intentionally ignores both submit and reset inputs from the value detaching code. I'm mostly certain that the only result of the code there for these input types was to set the value to an empty string (if no value was provided). This is because that's what the value was, however browsers do show a default text for value-less inputs of those types (since they are actions). These default values aren't actually accessible the DOM afaict so we don't have a good indication apart from looking at the value in props. But since it doesn't appear to matter, I just skip this step entirely.

Test Plan: jsfiddle in #7179 no longer shows empty submit input.

cc @jimfb, @spicyj

[jimfb]

👍

[syranide]

I'm curious, could it be enough to just do it when value !== '' (or when the attribute exists)? Or does this logic need to happen even for empty values as well?

Also, what about type="button" (EDIT: also type="image" and any future additions), and I think there may be some others that are problematic, like type="file". Doesn't this also add value to type="checkbox"? I'm actually not sure if that affects anything but perhaps it makes it report an empty string when checked?

[syranide]

Side question, if the idea is to "Detach value from defaultValue", couldn't you achieve the same by setting defaultValue instead (which seems less fragile)? Or does it only work for value?

[zpao]

could it be enough to just do it when value !== ''

That was my initial thought too but I wasn't sure so I went with the minimal change. You're right that there are others. I think type="image" is probably ok because that uses the src attribute. type="button" doesn't have a default display value so that's probably fine too. We don't know what the future will bring though I'm willing to bet we don't move in the direction of more <input>s that aren't actually inputs.

I've honestly lost track of the complicated logic going on around inputs. Maybe we can use defaultValue… Not sure what the implications are around controlled vs uncontrolled inputs. Since @jimfb just did a bunch of this input including these lines of code, hopefully he has better insight.

[jimfb]

If we use something like defaultValue, I would be a little worried about how controlled components behave with regards to reset buttons and placeholder text (especially on old browsers like IE8/9), so we would need to test those at the very least. The change @zpao added feels safest to me.

[jimfb]

With regards to checking for empty string... again, feels like we're going to hit edge cases. In particular, I'd worry about that attribute existing on all inputs (like reset buttons) and worried about failing to properly detach when the value was the empty string.

[jimfb]

Side question, if the idea is to "Detach value from defaultValue", couldn't you achieve the same by setting defaultValue instead (which seems less fragile)? Or does it only work for value?

No, it only works if you set value.

[zpao]

IE 8 & 9 didn't support the placeholder attribute so I'm not too concerned about them (unless we're doing something to make that work but I don't think we are). But if it only works for value then that concern is moot.

FWIW I think reading value from inputs is always safe, and even if it weren't it would just be undefined and we wouldn't want to do anything for that case.

Is there anything more specific we'd want to look out for (eg a flow that you expect might break). I'm having some trouble figuring out what the intent behind detaching from defaultValue is. I thought I figured it out for a second but then lost it. Surely something related to controlled components couldn't construct a case.

[jimfb]

FWIW I think reading value from inputs is always safe, and even if it weren't it would just be undefined and we wouldn't want to do anything for that case.

Sounds optimistic to me...

var foo = document.createElement('input');
foo.setAttribute('type', 'reset');
console.log(foo.value === undefined);

[zpao]

You can read it and it is empty, that's why this PR exists in the first place.

However if we were checking for foo.value !== '' before trying to set foo.value = foo.value, that would also sidestep the problem here BUT not sure what other implications it would have.

In other words, replace the same code I replaced here but with this:

var value = node.value;
if (value !== '') {
  node.value = value;
}

[jimfb]

But then you aren't detaching if the initial value is the empty string.

[sophiebits]

Chromium source makes it sound like

button, reset, image, submit, hidden, checkbox, radio

is the complete list of types where value and defaultValue are stored separately. (Mostly got that from https://cs.chromium.org/search/?q=storesValueSeparateFromAttribute&sq=package:chromium&type=cs, then grep BaseButtonInputType and BaseCheckableInputType.)

In addition, file throws when you set its value to anything other than an empty string – though that shouldn't cause this in particular to be a problem because this happens on initial render.

But it sounds like reset and submit are the only ones where they have another default value set:

https://cs.chromium.org/search/?q=class:InputType+function:%5CbdefaultValue%5Cb&sq=package:chromium&type=cs

So this change is probably fine.

[jimfb]

I agree, this change seems perfectly fine to me. The proposed potential alternatives seem to introduce additional risk without introducing additional value, as far as I can tell.

[syranide]

In addition, file throws when you set its value to anything other than an empty string – though that shouldn't cause this in particular to be a problem because this happens on initial render.

I'm curious, could this be an issue if you move away from a page and then back? In some cases the browser will restore what was previously written in the fields AFAIK (might require SSR), that might be an issue in this case with type="file"?