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

Fix form reset for uncontrolled select elements #11057

Merged
merged 3 commits into from
Oct 10, 2017

Conversation

aweary
Copy link
Contributor

@aweary aweary commented Oct 3, 2017

Resolves #11010

defaultSelected will now be set on the select's option's elements based on the value(s) in defaultValue. Added a test fixture, too.

@reactjs-bot
Copy link

reactjs-bot commented Oct 3, 2017

Deploy preview ready!

Built with commit 2ee1ba7

https://deploy-preview-11057--reactjs.netlify.com

@@ -103,6 +107,9 @@ function updateOptions(
for (let i = 0; i < options.length; i++) {
if (options[i].value === selectedValue) {
options[i].selected = true;
if (setDefaultSelected) {
options[i].defaultSelected = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps this should be set once on mount? That is a bit iffy i guess. Ostensibly defaultValue is set once for inputs and changing it after that is a noop for the life of the component. It seems like now tho you can set it, change it and reset the form and it'll use the new value. That may not be worth the complexity to address, but it opens up a new inconsistency perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ostensibly defaultValue is set once for inputs and changing it after that is a noop for the life of the component.

That isn't the case, as far as I've seen. Here's an example that you can update defaultValue and a form reset will reset to the new defaultValue option. That happens in ReactDOMFiberInput.updateWrapper here. In both cases, updating .defaultValue is guarded by a loose null check for value first, so I think it should be consistent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, so it's consistent with the other input reset behavior sorry. carry on then :P

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. It is frustrating that there is not a higher level DOM API for this.

Just to be sure: does form reset trigger a change event on selects? It should not, at least it does not for inputs last i checked, but i don't believe in consistent DOM APIs anymore

@jquense
Copy link
Contributor

jquense commented Oct 3, 2017

Do we know yet if defaultSelected works for all browsers on reset or do we still need to do some testing?

@aweary
Copy link
Contributor Author

aweary commented Oct 3, 2017

@jquense I tested in IE9, and the latest Chrome, Firefox, and Safari on macOS. It would be great if we could run through the fixtures in some browsers.

You can find them here: http://uncontrolled-select-reset.surge.sh/

Just to be sure: does form reset trigger a change event on selects? It should not, at least it does not for inputs last i checked, but i don't believe in consistent DOM APIs anymore

@nhunzaker from what I saw, it doesn't trigger a change event, so it should be consistent.

@jquense
Copy link
Contributor

jquense commented Oct 3, 2017

I ran it through on android 4.4, old IOS, edge and safari 8 and 👍

@nhunzaker
Copy link
Contributor

I can do some extra testing in an hour or so.

We should figure out how to map reduce this via a plea over Twitter :)

@nhunzaker
Copy link
Contributor

Finally got to testing. Tried to fill in the gaps:

  • IE10, IE11, IE Edge 15
  • Opera 18
  • Safari 11
  • iOS Safari 10.3
  • Current Chrome for Android

@gaearon
Copy link
Collaborator

gaearon commented Oct 9, 2017

Anything blocking this?

@nhunzaker
Copy link
Contributor

Nothing that I see. @aweary this is checking out for me everywhere. Any final concerns before merging?

@aweary
Copy link
Contributor Author

aweary commented Oct 10, 2017

@gaearon had mentioned that @sebmarkbage might have some thoughts on this, so if it looks good to him then 🚀

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.

Reset of select ignores defaultValue
6 participants