Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jul 5, 2016

Conversation

zpao
Copy link
Member

@zpao zpao commented Jul 5, 2016

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
Copy link
Contributor

jimfb commented Jul 5, 2016

👍

@zpao zpao added this to the 15-next milestone Jul 5, 2016
@zpao zpao merged commit 5c737b9 into facebook:master Jul 5, 2016
@syranide
Copy link
Contributor

syranide commented Jul 6, 2016

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
Copy link
Contributor

syranide commented Jul 6, 2016

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
Copy link
Member Author

zpao commented Jul 6, 2016

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
Copy link
Contributor

jimfb commented Jul 6, 2016

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
Copy link
Contributor

jimfb commented Jul 6, 2016

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
Copy link
Contributor

jimfb commented Jul 6, 2016

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
Copy link
Member Author

zpao commented Jul 6, 2016

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
Copy link
Contributor

jimfb commented Jul 6, 2016

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
Copy link
Member Author

zpao commented Jul 6, 2016

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
Copy link
Contributor

jimfb commented Jul 6, 2016

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

@sophiebits
Copy link
Collaborator

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
Copy link
Contributor

jimfb commented Jul 6, 2016

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
Copy link
Contributor

syranide commented Jul 7, 2016

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"?

zpao added a commit that referenced this pull request Jul 8, 2016
@zpao zpao modified the milestones: 15-next, 15.2.1 Jul 8, 2016
usmanajmal pushed a commit to usmanajmal/react that referenced this pull request Jul 11, 2016
@ghost ghost added the CLA Signed label Jul 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants