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

Fix form reset for uncontrolled select elements #11057

Merged
merged 3 commits into from
Oct 10, 2017
Merged

Fix form reset for uncontrolled select elements #11057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c681bf6
Set defaultSelected on option element from select's defaultValue
Oct 2, 2017
c5969ad
Add form reset fixture for selects
Oct 2, 2017
2ee1ba7
Pass explicit value for setDefaultSelected
Oct 3, 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
55 changes: 55 additions & 0 deletions fixtures/dom/src/components/fixtures/selects/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,23 @@ const ReactDOM = window.ReactDOM;
class SelectFixture extends React.Component {
state = {value: ''};
_nestedDOMNode = null;
_singleFormDOMNode = null;
_multipleFormDOMNode = null;

onChange = event => {
this.setState({value: event.target.value});
};

resetSingleOptionForm = event => {
event.preventDefault();
this._singleFormDOMNode.reset();
};

resetMultipleOptionForm = event => {
event.preventDefault();
this._multipleFormDOMNode.reset();
};

componentDidMount() {
this._renderNestedSelect();
}
Expand Down Expand Up @@ -100,6 +112,49 @@ class SelectFixture extends React.Component {
</select>
</div>
</TestCase>

<TestCase title="A single select being reset">
<TestCase.Steps>
<li>Open the select</li>
<li>Select "baz" or "foo"</li>
<li>Click the "Reset" button</li>
</TestCase.Steps>
<TestCase.ExpectedResult>
The select should be reset to the inital value, "bar"
</TestCase.ExpectedResult>

<div className="test-fixture">
<form ref={n => (this._singleFormDOMNode = n)}>
<select defaultValue="bar">
<option value="foo">foo</option>
<option value="bar">bar</option>
<option value="baz">baz</option>
</select>
<button onClick={this.resetSingleOptionForm}>Reset</button>
</form>
</div>
</TestCase>

<TestCase title="A multiple select being reset">
<TestCase.Steps>
<li>Select any combination of options</li>
<li>Click the "Reset" button</li>
</TestCase.Steps>
<TestCase.ExpectedResult>
The select should be reset to the initial values "foo" and "baz"
</TestCase.ExpectedResult>

<div className="test-fixture">
<form ref={n => (this._multipleFormDOMNode = n)}>
<select multiple defaultValue={['foo', 'baz']}>
<option value="foo">foo</option>
<option value="bar">bar</option>
<option value="baz">baz</option>
</select>
<button onClick={this.resetMultipleOptionForm}>Reset</button>
</form>
</div>
</TestCase>
</FixtureSet>
);
}
Expand Down
19 changes: 13 additions & 6 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 @@ -76,6 +76,7 @@ function updateOptions(
node: HTMLSelectElement,
multiple: boolean,
propValue: any,
setDefaultSelected: boolean,
) {
type IndexableHTMLOptionsCollection = HTMLOptionsCollection & {
[key: number]: HTMLOptionElement,
Expand All @@ -94,6 +95,9 @@ function updateOptions(
if (options[i].selected !== selected) {
options[i].selected = selected;
}
if (selected && setDefaultSelected) {
options[i].defaultSelected = true;
}
}
} else {
// Do not set `select.value` as exact behavior isn't consistent across all
Expand All @@ -103,6 +107,9 @@ function updateOptions(
for (let i = 0; i < options.length; i++) {
if (options[i].value === selectedValue) {
options[i].selected = true;
if (setDefaultSelected) {
options[i].defaultSelected = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps this should be set once on mount? That is a bit iffy i guess. Ostensibly defaultValue is set once for inputs and changing it after that is a noop for the life of the component. It seems like now tho you can set it, change it and reset the form and it'll use the new value. That may not be worth the complexity to address, but it opens up a new inconsistency perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ostensibly defaultValue is set once for inputs and changing it after that is a noop for the life of the component.

That isn't the case, as far as I've seen. Here's an example that you can update defaultValue and a form reset will reset to the new defaultValue option. That happens in ReactDOMFiberInput.updateWrapper here. In both cases, updating .defaultValue is guarded by a loose null check for value first, so I think it should be consistent?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, so it's consistent with the other input reset behavior sorry. carry on then :P

}
return;
}
if (defaultSelected === null && !options[i].disabled) {
Expand Down Expand Up @@ -173,9 +180,9 @@ var ReactDOMSelect = {
node.multiple = !!props.multiple;
var value = props.value;
if (value != null) {
updateOptions(node, !!props.multiple, value);
updateOptions(node, !!props.multiple, value, false);
} else if (props.defaultValue != null) {
updateOptions(node, !!props.multiple, props.defaultValue);
updateOptions(node, !!props.multiple, props.defaultValue, true);
}
},

Expand All @@ -190,14 +197,14 @@ var ReactDOMSelect = {

var value = props.value;
if (value != null) {
updateOptions(node, !!props.multiple, value);
updateOptions(node, !!props.multiple, value, false);
} else if (wasMultiple !== !!props.multiple) {
// For simplicity, reapply `defaultValue` if `multiple` is toggled.
if (props.defaultValue != null) {
updateOptions(node, !!props.multiple, props.defaultValue);
updateOptions(node, !!props.multiple, props.defaultValue, true);
} else {
// Revert the select back to its default unselected state.
updateOptions(node, !!props.multiple, props.multiple ? [] : '');
updateOptions(node, !!props.multiple, props.multiple ? [] : '', false);
}
}
},
Expand All @@ -207,7 +214,7 @@ var ReactDOMSelect = {
var value = props.value;

if (value != null) {
updateOptions(node, !!props.multiple, value);
updateOptions(node, !!props.multiple, value, false);
}
},
};
Expand Down