@samitouri / QOS-React / commits / 80411ea9b4

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

Brandon Dail committed Sep 1, 2017 at 13:34 UTC 80411ea9b47a14ed3de6993fd64fba1d79ec605d
6 files changed +138 -39
fixtures/dom/src/components/IssueList.js new
+26
@@ -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
+7 -1
@@ -1,10 +1,12 @@
1 import cn from 'classnames';
2 import semver from 'semver';
3 -import React from 'react';
3 import PropTypes from 'prop-types';
4 +import IssueList from './IssueList';
5 import {parse} from 'query-string';
6 import {semverString} from './propTypes';
7
8 +const React = window.React;
9 +
10 const propTypes = {
11 children: PropTypes.node.isRequired,
12 title: PropTypes.node.isRequired,
@@ -36,6 +38,7 @@ class TestCase extends React.Component {
38 resolvedIn,
39 resolvedBy,
40 affectedBrowsers,
41 + relatedIssues,
42 children,
43 } = this.props;
44
@@ -93,6 +96,9 @@ class TestCase extends React.Component {
96
97 {affectedBrowsers && <dt>Affected browsers: </dt>}
98 {affectedBrowsers && <dd>{affectedBrowsers}</dd>}
99 +
100 + {relatedIssues && <dt>Related Issues: </dt>}
101 + {relatedIssues && <dd><IssueList issues={relatedIssues} /></dd>}
102 </dl>
103
104 <p className="test-case__desc">
fixtures/dom/src/components/fixtures/selects/index.js
+70 -29
@@ -1,3 +1,6 @@
1 +import FixtureSet from '../../FixtureSet';
2 +import TestCase from '../../TestCase';
3 +
4 const React = window.React;
5 const ReactDOM = window.ReactDOM;
6
@@ -31,35 +34,73 @@ class SelectFixture extends React.Component {
34
35 render() {
36 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>
104 );
105 }
106 }
fixtures/dom/src/style.css
+13 -7
@@ -8,23 +8,20 @@ html {
8 font-size: 10px;
9 }
10 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;
14 font-size: 1.4rem;
15 margin: 0;
16 padding: 0;
17 }
18
17 -select {
18 - width: 12rem;
19 -}
20 -
19 button {
20 margin: 10px;
21 font-size: 18px;
22 padding: 5px;
23 }
24
27 -
25 .header {
26 background: #222;
27 box-shadow: inset 0 -1px 3px #000;
@@ -34,6 +31,10 @@ button {
31 padding: .8rem 1.6rem;
32 }
33
34 +.header select {
35 + width: 12rem;
36 +}
37 +
38 .header__inner {
39 display: table;
40 margin: 0 auto;
@@ -101,7 +102,8 @@ fieldset {
102 overflow: hidden;
103 }
104
104 -ul, ol {
105 +ul,
106 +ol {
107 margin: 0 0 2rem 0;
108 }
109
@@ -212,3 +214,7 @@ li {
214 background-color: #f4f4f4;
215 border-top: 1px solid #d9d9d9;
216 }
217 +
218 +.field-group {
219 + overflow: hidden;
220 +}
src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js
+6 -2
@@ -101,14 +101,18 @@ function updateOptions(
101 // Do not set `select.value` as exact behavior isn't consistent across all
102 // browsers for all cases.
103 let selectedValue = '' + (propValue: string);
104 + let defaultSelected = null;
105 for (let i = 0; i < options.length; i++) {
106 if (options[i].value === selectedValue) {
107 options[i].selected = true;
108 return;
109 }
110 + if (defaultSelected === null && !options[i].disabled) {
111 + defaultSelected = options[i];
112 + }
113 }
110 - if (options.length) {
111 - options[0].selected = true;
114 + if (defaultSelected !== null) {
115 + defaultSelected.selected = true;
116 }
117 }
118 }
src/renderers/dom/shared/wrappers/__tests__/ReactDOMSelect-test.js
+16
@@ -128,6 +128,22 @@ describe('ReactDOMSelect', () => {
128 expect(node.value).toEqual('gorilla');
129 });
130
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 +
147 it('should allow setting `value` to __proto__', () => {
148 var stub = (
149 <select value="__proto__" onChange={noop}>