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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
509cabc
Default to first non-disabled option for select
Aug 14, 2017
fb99fbe
Add ReactDOMSelect test for defaulting to non-disabled options
Aug 14, 2017
f2dea9d
Add test fixtures to cover disabled selected options
nhunzaker Jul 11, 2017
7e94da1
Fix bad merge
gaearon Sep 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,18 @@ function updateOptions(
// Do not set `select.value` as exact behavior isn't consistent across all
// browsers for all cases.
Copy link
Contributor

Choose a reason for hiding this comment

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

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 😛.

let selectedValue = '' + (propValue: string);
let defaultSelected = null;
for (let i = 0; i < options.length; i++) {
if (options[i].value === selectedValue) {
options[i].selected = true;
return;
}
if (defaultSelected === null && !options[i].disabled) {
defaultSelected = options[i];
}
}
if (options.length) {
options[0].selected = true;
if (defaultSelected !== null) {
defaultSelected.selected = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
}
}
Expand Down
16 changes: 16 additions & 0 deletions src/renderers/dom/shared/wrappers/__tests__/ReactDOMSelect-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,22 @@ describe('ReactDOMSelect', () => {
expect(node.value).toEqual('gorilla');
});

it('should default to the first non-disabled option', () => {
var stub = (
<select defaultValue="">
<option disabled={true}>Disabled</option>
<option disabled={true}>Still Disabled</option>
<option>0</option>
<option disabled={true}>Also Disabled</option>
</select>
);
var container = document.createElement('div');
stub = ReactDOM.render(stub, container);
var node = ReactDOM.findDOMNode(stub);
expect(node.options[0].selected).toBe(false);
expect(node.options[2].selected).toBe(true);
});

it('should allow setting `value` to __proto__', () => {
var stub = (
<select value="__proto__" onChange={noop}>
Expand Down