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

Default to first non-disabled option for select elements #10456

Merged
merged 4 commits into from Sep 1, 2017

Conversation

Projects
None yet
6 participants
@aweary
Member

aweary commented Aug 14, 2017

This fixes a regression that @nhunzaker found in #10142 where select elements were defaulting to the first option, regardless of whether it was disabled or not. This was not the behavior in 15.6. Copying my comment on this from Nate's PR:

This issue stems from #8349 which introduced an updateOptions call when a select element is mounted. IfupdateOptions doesn't find a matching option it defaults to setting the first option as selected.

In previous versions updateOptions was not called on the initial mount, so it defaulted to the standard behavior of the browser.

This brings us back in sync with the HTML spec which states:

If the select element's display size is 1, and no option elements in the select element's list of options have their selectedness set to true, set the selectedness of the first option element in the list of options in tree order that is not disabled, if any, to true.

https://html.spec.whatwg.org/multipage/form-elements.html#the-select-element

@nhunzaker I can just cherry-pick the DOM fixtures from #10142 as part of this PR if you'd like!

aweary added some commits Aug 14, 2017

Default to first non-disabled option for select
Instead of defaulting to the first option for a select element, which could be a disabled option, we find the first non-disabled option available while we looping through the options. If no option matches, set the first non-disabled option as selected.
if (options.length) {
options[0].selected = true;
if (defaultSelected !== null) {
defaultSelected.selected = true;

This comment has been minimized.

@jquense

jquense Aug 14, 2017

Collaborator

I know you didn't write all this, but the logic is hard to follow here...can we switch early to see if it's controlled?

@jquense

jquense Aug 14, 2017

Collaborator

I know you didn't write all this, but the logic is hard to follow here...can we switch early to see if it's controlled?

This comment has been minimized.

@aweary

aweary Aug 14, 2017

Member

I agree it's a little dense, can you clarify what you mean by switching early to see if it's controlled? As far as I can tell, updateOptions only branches on multiple and doesn't really care if it's controlled or not.

@aweary

aweary Aug 14, 2017

Member

I agree it's a little dense, can you clarify what you mean by switching early to see if it's controlled? As far as I can tell, updateOptions only branches on multiple and doesn't really care if it's controlled or not.

This comment has been minimized.

@jquense

jquense Aug 15, 2017

Collaborator

I was thinking check if propValue was set, but that wouldn't actually work...I guess i don't have any specific suggestions :P but I felt like I had to read this through 3 times to understand it, and it feels like it should be easier to express

@jquense

jquense Aug 15, 2017

Collaborator

I was thinking check if propValue was set, but that wouldn't actually work...I guess i don't have any specific suggestions :P but I felt like I had to read this through 3 times to understand it, and it feels like it should be easier to express

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Aug 14, 2017

Member

What happens to browser features like attempts to automatically restoring state and auto-fill? Does it care?

Member

sebmarkbage commented Aug 14, 2017

What happens to browser features like attempts to automatically restoring state and auto-fill? Does it care?

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 15, 2017

Collaborator

@aweary 🍒 away.

Collaborator

nhunzaker commented Aug 15, 2017

@aweary 🍒 away.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 15, 2017

Collaborator

@sebmarkbage We don't have fixture coverage for restored state and auto-fill. This would be a good addition to the suite. I'll write up an issue.

Collaborator

nhunzaker commented Aug 15, 2017

@sebmarkbage We don't have fixture coverage for restored state and auto-fill. This would be a good addition to the suite. I'll write up an issue.

@@ -101,14 +101,18 @@ function updateOptions(
// Do not set `select.value` as exact behavior isn't consistent across all
// browsers for all cases.

This comment has been minimized.

@nhunzaker

nhunzaker Aug 15, 2017

Collaborator

I wish I knew what wasn't consistent -- and where? Like was it IE8?. I'll write up an issue to do some archaeology here.

Totally unrelated to this PR 😛.

@nhunzaker

nhunzaker Aug 15, 2017

Collaborator

I wish I knew what wasn't consistent -- and where? Like was it IE8?. I'll write up an issue to do some archaeology here.

Totally unrelated to this PR 😛.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 1, 2017

Member

What is the status on this?

Member

gaearon commented Sep 1, 2017

What is the status on this?

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 1, 2017

Member

@gaearon I cherry-picked the test fixtures, so it should be good. Feel free to merge if it looks good to you.

Member

aweary commented Sep 1, 2017

@gaearon I cherry-picked the test fixtures, so it should be good. Feel free to merge if it looks good to you.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 1, 2017

Member

The fixture is broken for me.
There's a few consts before imports (probably because react-scripts version has changed since).

But I also get a runtime error:

screen shot 2017-09-01 at 1 14 51 pm

Inside selects/index.js, _nestedDOMNode is always null so _renderNestedSelect fails.

Member

gaearon commented Sep 1, 2017

The fixture is broken for me.
There's a few consts before imports (probably because react-scripts version has changed since).

But I also get a runtime error:

screen shot 2017-09-01 at 1 14 51 pm

Inside selects/index.js, _nestedDOMNode is always null so _renderNestedSelect fails.

@gaearon gaearon added this to the 16.0 milestone Sep 1, 2017

<span className="hint" />
</fieldset>
<fieldset>
<legend>Controlled in nested subtree</legend>

This comment has been minimized.

@gaearon

gaearon Sep 1, 2017

Member

Was this case removed intentionally?

@gaearon

gaearon Sep 1, 2017

Member

Was this case removed intentionally?

@gaearon gaearon merged commit 80411ea into facebook:master Sep 1, 2017

1 check was pending

ci/circleci CircleCI is running your tests
Details
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 1, 2017

Member

Thanks!

Member

gaearon commented Sep 1, 2017

Thanks!

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 1, 2017

Member

Thanks for the fix @gaearon!

Member

aweary commented Sep 1, 2017

Thanks for the fix @gaearon!

@bvaughn bvaughn referenced this pull request Sep 7, 2017

Closed

React 16 RC #10294

flarnie added a commit to flarnie/react that referenced this pull request Sep 8, 2017

Default to first non-disabled option for select elements (#10456)
* Default to first non-disabled option for select

Instead of defaulting to the first option for a select element, which could be a disabled option, we find the first non-disabled option available while we looping through the options. If no option matches, set the first non-disabled option as selected.

* Add ReactDOMSelect test for defaulting to non-disabled options

* Add test fixtures to cover disabled selected options

* Fix bad merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment