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

Jump to bottom

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 all 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 26 additions & 0 deletions fixtures/dom/src/components/IssueList.js
View file
Open in desktop
@@ -0,0 +1,26 @@
const React = window.React;

function csv(string) {
return string.split(/\s*,\s*/);
}

export default function IssueList({issues}) {
if (!issues) {
return null;
}

if (typeof issues === 'string') {
issues = csv(issues);
}

let links = issues.reduce((memo, issue, i) => {
return memo.concat(
i > 0 && i < issues.length ? ', ' : null,
<a href={'https://github.com/facebook/react/issues/' + issue} key={issue}>
{issue}
</a>
);
}, []);

return <span>{links}</span>;
}
8 changes: 7 additions & 1 deletion fixtures/dom/src/components/TestCase.js
View file
Open in desktop
@@ -1,10 +1,12 @@
import cn from 'classnames';
import semver from 'semver';
import React from 'react';
import PropTypes from 'prop-types';
import IssueList from './IssueList';
import {parse} from 'query-string';
import {semverString} from './propTypes';

const React = window.React;

const propTypes = {
children: PropTypes.node.isRequired,
title: PropTypes.node.isRequired,
Expand Down Expand Up @@ -36,6 +38,7 @@ class TestCase extends React.Component {
resolvedIn,
resolvedBy,
affectedBrowsers,
relatedIssues,
children,
} = this.props;

Expand Down Expand Up @@ -93,6 +96,9 @@ class TestCase extends React.Component {

{affectedBrowsers && <dt>Affected browsers: </dt>}
{affectedBrowsers && <dd>{affectedBrowsers}</dd>}

{relatedIssues && <dt>Related Issues: </dt>}
{relatedIssues && <dd><IssueList issues={relatedIssues} /></dd>}
</dl>

<p className="test-case__desc">
Expand Down
99 changes: 70 additions & 29 deletions fixtures/dom/src/components/fixtures/selects/index.js
View file
Open in desktop
@@ -1,3 +1,6 @@
import FixtureSet from '../../FixtureSet';
import TestCase from '../../TestCase';

const React = window.React;
const ReactDOM = window.ReactDOM;

Expand Down Expand Up @@ -31,35 +34,73 @@ class SelectFixture extends React.Component {

render() {
return (
<form>
<fieldset>
<legend>Controlled</legend>
<select value={this.state.value} onChange={this.onChange}>
<option value="">Select a color</option>
<option value="red">Red</option>
<option value="blue">Blue</option>
<option value="green">Green</option>
</select>
<span className="hint">Value: {this.state.value}</span>
</fieldset>
<fieldset>
<legend>Uncontrolled</legend>
<select defaultValue="">
<option value="">Select a color</option>
<option value="red">Red</option>
<option value="blue">Blue</option>
<option value="green">Green</option>
</select>
<span className="hint" />
</fieldset>
<fieldset>
<legend>Controlled in nested subtree</legend>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this case removed intentionally?

<div ref={node => (this._nestedDOMNode = node)} />
<span className="hint">
This should synchronize in both direction with the one above.
</span>
</fieldset>
</form>
<FixtureSet title="Selects" description="">
<form className="field-group">
<fieldset>
<legend>Controlled</legend>
<select value={this.state.value} onChange={this.onChange}>
<option value="">Select a color</option>
<option value="red">Red</option>
<option value="blue">Blue</option>
<option value="green">Green</option>
</select>
<span className="hint">Value: {this.state.value}</span>
</fieldset>
<fieldset>
<legend>Uncontrolled</legend>
<select defaultValue="">
<option value="">Select a color</option>
<option value="red">Red</option>
<option value="blue">Blue</option>
<option value="green">Green</option>
</select>
</fieldset>
<fieldset>
<legend>Controlled in nested subtree</legend>
<div ref={node => (this._nestedDOMNode = node)} />
<span className="hint">
This should synchronize in both direction with the "Controlled".
</span>
</fieldset>
</form>

<TestCase title="A selected disabled option" relatedIssues="2803">
<TestCase.Steps>
<li>Open the select</li>
<li>Select "1"</li>
<li>Attempt to reselect "Please select an item"</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The initial picked option should be "Please select an
item", however it should not be a selectable option.
</TestCase.ExpectedResult>

<div className="test-fixture">
<select defaultValue="">
<option value="" disabled>Please select an item</option>
<option>0</option>
<option>1</option>
<option>2</option>
</select>
</div>
</TestCase>

<TestCase title="An unselected disabled option" relatedIssues="2803">
<TestCase.ExpectedResult>
The initial picked option value should "0": the first non-disabled option.
</TestCase.ExpectedResult>

<div className="test-fixture">
<select defaultValue="">
<option disabled>Please select an item</option>
<option>0</option>
<option>1</option>
<option>2</option>
</select>
</div>
</TestCase>
</FixtureSet>
);
}
}
Expand Down
20 changes: 13 additions & 7 deletions fixtures/dom/src/style.css
View file
Open in desktop
Expand Up @@ -8,23 +8,20 @@ html {
font-size: 10px;
}
body {
font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", "Roboto", "Oxygen", "Ubuntu", "Cantarell", "Fira Sans", "Droid Sans", "Helvetica Neue", sans-serif;
font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", "Roboto", "Oxygen",
"Ubuntu", "Cantarell", "Fira Sans", "Droid Sans", "Helvetica Neue",
sans-serif;
font-size: 1.4rem;
margin: 0;
padding: 0;
}

select {
width: 12rem;
}

button {
margin: 10px;
font-size: 18px;
padding: 5px;
}


.header {
background: #222;
box-shadow: inset 0 -1px 3px #000;
Expand All @@ -34,6 +31,10 @@ button {
padding: .8rem 1.6rem;
}

.header select {
width: 12rem;
}

.header__inner {
display: table;
margin: 0 auto;
Expand Down Expand Up @@ -101,7 +102,8 @@ fieldset {
overflow: hidden;
}

ul, ol {
ul,
ol {
margin: 0 0 2rem 0;
}

Expand Down Expand Up @@ -212,3 +214,7 @@ li {
background-color: #f4f4f4;
border-top: 1px solid #d9d9d9;
}

.field-group {
overflow: hidden;
}
8 changes: 6 additions & 2 deletions src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js
View file
Open in desktop
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
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