Skip to content

Commit

Permalink
Don't set the first option as selected in select tag with size attr…
Browse files Browse the repository at this point in the history
…ibute (#14242)

* Set 'size' attribute to select tag if it occurs before appending options

* Add comment about why size is assigned on select create. Tests

I added some more clarification for why size must be set on select
element creation:

- In the source code
- In the DOM test fixture
- In a unit test

* Use let, not const in select tag stub assignment
  • Loading branch information
kulek1 authored and nhunzaker committed Mar 14, 2019
1 parent 935f600 commit ab5fe17
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 6 deletions.
28 changes: 28 additions & 0 deletions fixtures/dom/src/components/fixtures/selects/index.js
Expand Up @@ -202,6 +202,34 @@ class SelectFixture extends React.Component {
</select>
</div>
</TestCase>

<TestCase
title="A select with the size attribute should not set first option as selected"
relatedIssues="14239"
introducedIn="16.0.0">
<TestCase.ExpectedResult>
No options should be selected.
</TestCase.ExpectedResult>

<div className="test-fixture">
<select size="3">
<option>0</option>
<option>1</option>
<option>2</option>
</select>
</div>

<p className="footnote">
<b>Notes:</b> This happens if <code>size</code> is assigned after
options are selected. The select element picks the first item by
default, then it is expanded to show more options when{' '}
<code>size</code> is assigned, preserving the default selection.
</p>
<p className="footnote">
This was introduced in React 16.0.0 when options were added before
select attribute assignment.
</p>
</TestCase>
</FixtureSet>
);
}
Expand Down
26 changes: 26 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMSelect-test.js
Expand Up @@ -362,6 +362,32 @@ describe('ReactDOMSelect', () => {
expect(node.options[2].selected).toBe(true); // gorilla
});

it('does not select an item when size is initially set to greater than 1', () => {
const stub = (
<select size="2">
<option value="monkey">A monkey!</option>
<option value="giraffe">A giraffe!</option>
<option value="gorilla">A gorilla!</option>
</select>
);
const container = document.createElement('div');
const select = ReactDOM.render(stub, container);

expect(select.options[0].selected).toBe(false);
expect(select.options[1].selected).toBe(false);
expect(select.options[2].selected).toBe(false);

// Note: There is an inconsistency between JSDOM and Chrome where
// Chrome reports an empty string when no value is selected for a
// single-select with a size greater than 0. JSDOM reports the first
// value
//
// This assertion exists only for clarity of JSDOM behavior:
expect(select.value).toBe('monkey'); // "" in Chrome
// Despite this, the selection index is correct:
expect(select.selectedIndex).toBe(-1);
});

it('should remember value when switching to uncontrolled', () => {
let stub = (
<select value={'giraffe'} onChange={noop}>
Expand Down
23 changes: 17 additions & 6 deletions packages/react-dom/src/client/ReactDOMComponent.js
Expand Up @@ -419,14 +419,25 @@ export function createElement(
// See discussion in https://github.com/facebook/react/pull/6896
// and discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1276240
domElement = ownerDocument.createElement(type);
// Normally attributes are assigned in `setInitialDOMProperties`, however the `multiple`
// attribute on `select`s needs to be added before `option`s are inserted. This prevents
// a bug where the `select` does not scroll to the correct option because singular
// `select` elements automatically pick the first item.
// Normally attributes are assigned in `setInitialDOMProperties`, however the `multiple` and `size`
// attributes on `select`s needs to be added before `option`s are inserted.
// This prevents:
// - a bug where the `select` does not scroll to the correct option because singular
// `select` elements automatically pick the first item #13222
// - a bug where the `select` set the first item as selected despite the `size` attribute #14239
// See https://github.com/facebook/react/issues/13222
if (type === 'select' && props.multiple) {
// and https://github.com/facebook/react/issues/14239
if (type === 'select') {
const node = ((domElement: any): HTMLSelectElement);
node.multiple = true;
if (props.multiple) {
node.multiple = true;
} else if (props.size) {
// Setting a size greater than 1 causes a select to behave like `multiple=true`, where
// it is possible that no option is selected.
//
// This is only necessary when a select in "single selection mode".
node.size = props.size;
}
}
}
} else {
Expand Down

0 comments on commit ab5fe17

Please sign in to comment.