Skip to content

Commit 80411ea

Browse files
awearygaearon
aweary
authored and
gaearon
committed
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
1 parent f6ceacd commit 80411ea

File tree

6 files changed

+138
-39
lines changed

6 files changed

+138
-39
lines changed

‎fixtures/dom/src/components/IssueList.js‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
const React = window.React;
2+
3+
function csv(string) {
4+
return string.split(/\s*,\s*/);
5+
}
6+
7+
export default function IssueList({issues}) {
8+
if (!issues) {
9+
return null;
10+
}
11+
12+
if (typeof issues === 'string') {
13+
issues = csv(issues);
14+
}
15+
16+
let links = issues.reduce((memo, issue, i) => {
17+
return memo.concat(
18+
i > 0 && i < issues.length ? ', ' : null,
19+
<a href={'https://github.com/facebook/react/issues/' + issue} key={issue}>
20+
{issue}
21+
</a>
22+
);
23+
}, []);
24+
25+
return <span>{links}</span>;
26+
}

‎fixtures/dom/src/components/TestCase.js‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
import cn from 'classnames';
22
import semver from 'semver';
3-
import React from 'react';
43
import PropTypes from 'prop-types';
4+
import IssueList from './IssueList';
55
import {parse} from 'query-string';
66
import {semverString} from './propTypes';
77

8+
const React = window.React;
9+
810
const propTypes = {
911
children: PropTypes.node.isRequired,
1012
title: PropTypes.node.isRequired,
@@ -36,6 +38,7 @@ class TestCase extends React.Component {
3638
resolvedIn,
3739
resolvedBy,
3840
affectedBrowsers,
41+
relatedIssues,
3942
children,
4043
} = this.props;
4144

@@ -93,6 +96,9 @@ class TestCase extends React.Component {
9396

9497
{affectedBrowsers && <dt>Affected browsers: </dt>}
9598
{affectedBrowsers && <dd>{affectedBrowsers}</dd>}
99+
100+
{relatedIssues && <dt>Related Issues: </dt>}
101+
{relatedIssues && <dd><IssueList issues={relatedIssues} /></dd>}
96102
</dl>
97103

98104
<p className="test-case__desc">

‎fixtures/dom/src/components/fixtures/selects/index.js‎

Lines changed: 70 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
import FixtureSet from '../../FixtureSet';
2+
import TestCase from '../../TestCase';
3+
14
const React = window.React;
25
const ReactDOM = window.ReactDOM;
36

@@ -31,35 +34,73 @@ class SelectFixture extends React.Component {
3134

3235
render() {
3336
return (
34-
<form>
35-
<fieldset>
36-
<legend>Controlled</legend>
37-
<select value={this.state.value} onChange={this.onChange}>
38-
<option value="">Select a color</option>
39-
<option value="red">Red</option>
40-
<option value="blue">Blue</option>
41-
<option value="green">Green</option>
42-
</select>
43-
<span className="hint">Value: {this.state.value}</span>
44-
</fieldset>
45-
<fieldset>
46-
<legend>Uncontrolled</legend>
47-
<select defaultValue="">
48-
<option value="">Select a color</option>
49-
<option value="red">Red</option>
50-
<option value="blue">Blue</option>
51-
<option value="green">Green</option>
52-
</select>
53-
<span className="hint" />
54-
</fieldset>
55-
<fieldset>
56-
<legend>Controlled in nested subtree</legend>
57-
<div ref={node => (this._nestedDOMNode = node)} />
58-
<span className="hint">
59-
This should synchronize in both direction with the one above.
60-
</span>
61-
</fieldset>
62-
</form>
37+
<FixtureSet title="Selects" description="">
38+
<form className="field-group">
39+
<fieldset>
40+
<legend>Controlled</legend>
41+
<select value={this.state.value} onChange={this.onChange}>
42+
<option value="">Select a color</option>
43+
<option value="red">Red</option>
44+
<option value="blue">Blue</option>
45+
<option value="green">Green</option>
46+
</select>
47+
<span className="hint">Value: {this.state.value}</span>
48+
</fieldset>
49+
<fieldset>
50+
<legend>Uncontrolled</legend>
51+
<select defaultValue="">
52+
<option value="">Select a color</option>
53+
<option value="red">Red</option>
54+
<option value="blue">Blue</option>
55+
<option value="green">Green</option>
56+
</select>
57+
</fieldset>
58+
<fieldset>
59+
<legend>Controlled in nested subtree</legend>
60+
<div ref={node => (this._nestedDOMNode = node)} />
61+
<span className="hint">
62+
This should synchronize in both direction with the "Controlled".
63+
</span>
64+
</fieldset>
65+
</form>
66+
67+
<TestCase title="A selected disabled option" relatedIssues="2803">
68+
<TestCase.Steps>
69+
<li>Open the select</li>
70+
<li>Select "1"</li>
71+
<li>Attempt to reselect "Please select an item"</li>
72+
</TestCase.Steps>
73+
74+
<TestCase.ExpectedResult>
75+
The initial picked option should be "Please select an
76+
item", however it should not be a selectable option.
77+
</TestCase.ExpectedResult>
78+
79+
<div className="test-fixture">
80+
<select defaultValue="">
81+
<option value="" disabled>Please select an item</option>
82+
<option>0</option>
83+
<option>1</option>
84+
<option>2</option>
85+
</select>
86+
</div>
87+
</TestCase>
88+
89+
<TestCase title="An unselected disabled option" relatedIssues="2803">
90+
<TestCase.ExpectedResult>
91+
The initial picked option value should "0": the first non-disabled option.
92+
</TestCase.ExpectedResult>
93+
94+
<div className="test-fixture">
95+
<select defaultValue="">
96+
<option disabled>Please select an item</option>
97+
<option>0</option>
98+
<option>1</option>
99+
<option>2</option>
100+
</select>
101+
</div>
102+
</TestCase>
103+
</FixtureSet>
63104
);
64105
}
65106
}

‎fixtures/dom/src/style.css‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,23 +8,20 @@ html {
88
font-size: 10px;
99
}
1010
body {
11-
font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", "Roboto", "Oxygen", "Ubuntu", "Cantarell", "Fira Sans", "Droid Sans", "Helvetica Neue", sans-serif;
11+
font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", "Roboto", "Oxygen",
12+
"Ubuntu", "Cantarell", "Fira Sans", "Droid Sans", "Helvetica Neue",
13+
sans-serif;
1214
font-size: 1.4rem;
1315
margin: 0;
1416
padding: 0;
1517
}
1618

17-
select {
18-
width: 12rem;
19-
}
20-
2119
button {
2220
margin: 10px;
2321
font-size: 18px;
2422
padding: 5px;
2523
}
2624

27-
2825
.header {
2926
background: #222;
3027
box-shadow: inset 0 -1px 3px #000;
@@ -34,6 +31,10 @@ button {
3431
padding: .8rem 1.6rem;
3532
}
3633

34+
.header select {
35+
width: 12rem;
36+
}
37+
3738
.header__inner {
3839
display: table;
3940
margin: 0 auto;
@@ -101,7 +102,8 @@ fieldset {
101102
overflow: hidden;
102103
}
103104

104-
ul, ol {
105+
ul,
106+
ol {
105107
margin: 0 0 2rem 0;
106108
}
107109

@@ -212,3 +214,7 @@ li {
212214
background-color: #f4f4f4;
213215
border-top: 1px solid #d9d9d9;
214216
}
217+
218+
.field-group {
219+
overflow: hidden;
220+
}

‎src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,14 +101,18 @@ function updateOptions(
101101
// Do not set `select.value` as exact behavior isn't consistent across all
102102
// browsers for all cases.
103103
let selectedValue = '' + (propValue: string);
104+
let defaultSelected = null;
104105
for (let i = 0; i < options.length; i++) {
105106
if (options[i].value === selectedValue) {
106107
options[i].selected = true;
107108
return;
108109
}
110+
if (defaultSelected === null && !options[i].disabled) {
111+
defaultSelected = options[i];
112+
}
109113
}
110-
if (options.length) {
111-
options[0].selected = true;
114+
if (defaultSelected !== null) {
115+
defaultSelected.selected = true;
112116
}
113117
}
114118
}

‎src/renderers/dom/shared/wrappers/__tests__/ReactDOMSelect-test.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,22 @@ describe('ReactDOMSelect', () => {
128128
expect(node.value).toEqual('gorilla');
129129
});
130130

131+
it('should default to the first non-disabled option', () => {
132+
var stub = (
133+
<select defaultValue="">
134+
<option disabled={true}>Disabled</option>
135+
<option disabled={true}>Still Disabled</option>
136+
<option>0</option>
137+
<option disabled={true}>Also Disabled</option>
138+
</select>
139+
);
140+
var container = document.createElement('div');
141+
stub = ReactDOM.render(stub, container);
142+
var node = ReactDOM.findDOMNode(stub);
143+
expect(node.options[0].selected).toBe(false);
144+
expect(node.options[2].selected).toBe(true);
145+
});
146+
131147
it('should allow setting `value` to __proto__', () => {
132148
var stub = (
133149
<select value="__proto__" onChange={noop}>

0 commit comments

Comments
 (0)