From 2abb23244fc54535e009f427b5592180289037ae Mon Sep 17 00:00:00 2001 From: Dmytro Zasiadko Date: Wed, 25 Jul 2018 23:29:46 +0200 Subject: [PATCH 1/6] Make sure that `select` has `multiple` attribute set to appropriate state before appending options fixes #13222 --- packages/react-dom/src/client/ReactDOMFiberComponent.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/react-dom/src/client/ReactDOMFiberComponent.js b/packages/react-dom/src/client/ReactDOMFiberComponent.js index bad296429e43..df92cc38c802 100644 --- a/packages/react-dom/src/client/ReactDOMFiberComponent.js +++ b/packages/react-dom/src/client/ReactDOMFiberComponent.js @@ -378,6 +378,12 @@ 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); + // Make sure that `select` has `multiple` attribute set to appropriate state before appending options + // To prevent first option be initialy made selected + // see more details in https://github.com/facebook/react/issues/13222 + if (type === 'select' && !!props.multiple) { + domElement.setAttribute('multiple', 'true'); + } } } else { domElement = ownerDocument.createElementNS(namespaceURI, type); From d7c57e5f233dc6b38e2106c6d102ca9d6bea68c4 Mon Sep 17 00:00:00 2001 From: Dmytro Zasiadko Date: Fri, 27 Jul 2018 12:41:14 +0200 Subject: [PATCH 2/6] Add dom test fixture to test long multiple select scrolling to the first selected option fixes #13222 --- .../src/components/fixtures/selects/index.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/fixtures/dom/src/components/fixtures/selects/index.js b/fixtures/dom/src/components/fixtures/selects/index.js index 5460919cc231..fa13ee8c06f4 100644 --- a/fixtures/dom/src/components/fixtures/selects/index.js +++ b/fixtures/dom/src/components/fixtures/selects/index.js @@ -158,6 +158,25 @@ class SelectFixture extends React.Component { + + + + First selected option should be visible + + +
+
(this._multipleFormDOMNode = n)}> + +
+
+ ); } From 5d8d46aeef5e5ec438226ccbd0482a5a77d3bb74 Mon Sep 17 00:00:00 2001 From: Dmytro Zasiadko Date: Wed, 1 Aug 2018 23:52:10 +0200 Subject: [PATCH 3/6] typo fix --- fixtures/dom/src/components/fixtures/selects/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fixtures/dom/src/components/fixtures/selects/index.js b/fixtures/dom/src/components/fixtures/selects/index.js index fa13ee8c06f4..0e82110acb6e 100644 --- a/fixtures/dom/src/components/fixtures/selects/index.js +++ b/fixtures/dom/src/components/fixtures/selects/index.js @@ -159,7 +159,7 @@ class SelectFixture extends React.Component { - + First selected option should be visible From b17ed8c68e89f2e3b3606470b6406f4eae726e24 Mon Sep 17 00:00:00 2001 From: Dmytro Zasiadko Date: Thu, 2 Aug 2018 07:28:06 +0200 Subject: [PATCH 4/6] update comment remove redundant conversion to bool type --- .../react-dom/src/client/ReactDOMFiberComponent.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMFiberComponent.js b/packages/react-dom/src/client/ReactDOMFiberComponent.js index df92cc38c802..e434dcb8dc45 100644 --- a/packages/react-dom/src/client/ReactDOMFiberComponent.js +++ b/packages/react-dom/src/client/ReactDOMFiberComponent.js @@ -378,10 +378,12 @@ 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); - // Make sure that `select` has `multiple` attribute set to appropriate state before appending options - // To prevent first option be initialy made selected - // see more details in https://github.com/facebook/react/issues/13222 - if (type === 'select' && !!props.multiple) { + // 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. + // See https://github.com/facebook/react/issues/13222 + if (type === 'select' && props.multiple) { domElement.setAttribute('multiple', 'true'); } } From 561efab98150152d7b87661056effff215b0fbcf Mon Sep 17 00:00:00 2001 From: Dmytro Zasiadko Date: Fri, 3 Aug 2018 08:06:23 +0200 Subject: [PATCH 5/6] change a way of assigning property to domElement --- packages/react-dom/src/client/ReactDOMFiberComponent.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/client/ReactDOMFiberComponent.js b/packages/react-dom/src/client/ReactDOMFiberComponent.js index fd3d0b4f0ad9..3928541fda88 100644 --- a/packages/react-dom/src/client/ReactDOMFiberComponent.js +++ b/packages/react-dom/src/client/ReactDOMFiberComponent.js @@ -390,7 +390,8 @@ export function createElement( // `select` elements automatically pick the first item. // See https://github.com/facebook/react/issues/13222 if (type === 'select' && props.multiple) { - domElement.setAttribute('multiple', 'true'); + const node = ((domElement: any): HTMLSelectElement); + node.multiple = true; } } } else { From afedb687fcc0ef57fe4bf3344da58b4186330b45 Mon Sep 17 00:00:00 2001 From: Nate Hunzaker Date: Fri, 3 Aug 2018 09:58:13 -0700 Subject: [PATCH 6/6] Remove unused ref on select fixture form --- fixtures/dom/src/components/fixtures/selects/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fixtures/dom/src/components/fixtures/selects/index.js b/fixtures/dom/src/components/fixtures/selects/index.js index b6bdf83b0fd9..174fc32fb25d 100644 --- a/fixtures/dom/src/components/fixtures/selects/index.js +++ b/fixtures/dom/src/components/fixtures/selects/index.js @@ -165,7 +165,7 @@ class SelectFixture extends React.Component {
-
(this._multipleFormDOMNode = n)}> +