From 19a214eefbd4d7fbf1ba584081d013728e085402 Mon Sep 17 00:00:00 2001 From: kulek1 Date: Thu, 15 Nov 2018 19:09:34 +0100 Subject: [PATCH 1/3] Set 'size' attribute to select tag if it occurs before appending options --- .../src/components/fixtures/selects/index.js | 16 ++++++++++++++++ .../react-dom/src/client/ReactDOMComponent.js | 19 +++++++++++++------ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/fixtures/dom/src/components/fixtures/selects/index.js b/fixtures/dom/src/components/fixtures/selects/index.js index e83bc3efd6676..650d3c62c6f68 100644 --- a/fixtures/dom/src/components/fixtures/selects/index.js +++ b/fixtures/dom/src/components/fixtures/selects/index.js @@ -202,6 +202,22 @@ class SelectFixture extends React.Component { + + + + First option should not be set + + +
+ +
+ ); } diff --git a/packages/react-dom/src/client/ReactDOMComponent.js b/packages/react-dom/src/client/ReactDOMComponent.js index 27ac918a87336..c0b6da7486bb1 100644 --- a/packages/react-dom/src/client/ReactDOMComponent.js +++ b/packages/react-dom/src/client/ReactDOMComponent.js @@ -398,14 +398,21 @@ 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) { + node.size = props.size; + } } } } else { From b3acfd97192b1a72d4843143f0bc2a7e3460f0b7 Mon Sep 17 00:00:00 2001 From: Nate Date: Thu, 28 Feb 2019 12:46:25 -0800 Subject: [PATCH 2/3] 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 --- .../src/components/fixtures/selects/index.js | 16 ++++++++++-- .../src/__tests__/ReactDOMSelect-test.js | 26 +++++++++++++++++++ .../react-dom/src/client/ReactDOMComponent.js | 4 +++ 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/fixtures/dom/src/components/fixtures/selects/index.js b/fixtures/dom/src/components/fixtures/selects/index.js index 650d3c62c6f68..c0423b6df853e 100644 --- a/fixtures/dom/src/components/fixtures/selects/index.js +++ b/fixtures/dom/src/components/fixtures/selects/index.js @@ -205,9 +205,10 @@ class SelectFixture extends React.Component { + relatedIssues="14239" + introducedIn="16.0.0"> - First option should not be set + No options should be selected.
@@ -217,6 +218,17 @@ class SelectFixture extends React.Component {
+ +

+ Notes: This happens if size is assigned after + options are selected. The select element picks the first item by + default, then it is expanded to show more options when{' '} + size is assigned, preserving the default selection. +

+

+ This was introduced in React 16.0.0 when options were added before + select attribute assignment. +

); diff --git a/packages/react-dom/src/__tests__/ReactDOMSelect-test.js b/packages/react-dom/src/__tests__/ReactDOMSelect-test.js index bd87c4543e95f..15b80053b2375 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSelect-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMSelect-test.js @@ -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', () => { + let stub = ( + + ); + 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 = (