Make sure that `select` has `multiple` attribute set to appropriate state before appending options #13270

segoddnja · 2018-07-25T21:53:09Z

Applying multiple options to select node before it's multiple attribute is set to true leads to the first option be selected by default, event in the case there is no value or defaultValue passed in the props.

It leads to another undesired outcome. As setting real selected attributes values happens in the post mount wrapper, browser does not scroll list to selected option if it is outside of the select.

fixes #13222

…tate before appending options fixes facebook#13222

pull-bot · 2018-07-25T22:03:27Z

ReactDOM: size: 0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: b565f49...afedb68

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.1%	+0.1%	643 KB	643.51 KB	151 KB	151.17 KB	UMD_DEV
react-dom.production.min.js	0.0%	0.0%	96.27 KB	96.31 KB	31.21 KB	31.22 KB	UMD_PROD
react-dom.development.js	+0.1%	+0.1%	639.14 KB	639.65 KB	149.83 KB	150 KB	NODE_DEV
react-dom.production.min.js	0.0%	0.0%	96.26 KB	96.3 KB	30.74 KB	30.75 KB	NODE_PROD
ReactDOM-dev.js	+0.1%	+0.1%	646.35 KB	646.86 KB	148.13 KB	148.3 KB	FB_WWW_DEV
ReactDOM-prod.js	🔺+0.1%	0.0%	278.1 KB	278.29 KB	52.2 KB	52.22 KB	FB_WWW_PROD
react-dom.profiling.min.js	0.0%	0.0%	97.39 KB	97.44 KB	31.14 KB	31.15 KB	NODE_PROFILING
ReactDOM-profiling.js	+0.1%	0.0%	281.39 KB	281.58 KB	52.92 KB	52.95 KB	FB_WWW_PROFILING

Generated by 🚫 dangerJS

nhunzaker · 2018-07-26T16:07:27Z

Awesome. Thank you for getting to the bottom of this!

I'm looking into whether or not this is the best place to ensure multiple is set on a select before options are added. In the meantime, I'd like to figure out how to get test coverage on this.

Could you check to see if JSDOM sets the scroll position on multi-selects? If so, I'd like to add a test for this here:
https://github.com/facebook/react/blob/master/packages/react-dom/src/__tests__/ReactDOMSelect-test.js

Otherwise, I'd like to get manual dom test fixture for this. These live here:
https://github.com/facebook/react/tree/master/fixtures/dom

And we should add a test case to this module:
https://github.com/facebook/react/blob/master/fixtures/dom/src/components/fixtures/selects/index.js

Please let me know if you hit a snag, I'd be happy to provide additional assistance!

…rst selected option fixes facebook#13222

segoddnja · 2018-07-27T10:58:56Z

@nhunzaker No problem. I think there is really not so much options where to ensure that select has appropriate multiple value. It creates instance by createInstance method call and appends children right after this. So createInstance and createElement only two places where is possible to do this.

Could you check to see if JSDOM sets the scroll position on multi-selects?

I don't think it is possible to do with JSDOM. And even if it had been possible it would have been tested only against JSDOM. Do not see much sense in this.

I've added manual dom fixture.

gaearon · 2018-08-01T17:10:03Z

fixtures/dom/src/components/fixtures/selects/index.js

@@ -158,6 +158,25 @@ class SelectFixture extends React.Component {
            </form>
          </div>
        </TestCase>
+
+        <TestCase title="A multiple selec being scrolled to first selected option">


Typo: selec

gaearon · 2018-08-01T17:11:18Z

Can this be colocated with other ReactDOMFiberSelect logic?

segoddnja · 2018-08-01T21:59:14Z

Can this be colocated with other ReactDOMFiberSelect logic?

I don't think so. At least I don't see a way how to do this.

gaearon · 2018-08-01T22:22:58Z

Can you explain why? I’m curious why we usually adjust properties later but here we have to do it right after creation. What’s special?

nhunzaker · 2018-08-01T22:50:42Z

Just to recap, the issue is that the multiple attribute needs to be set before the option children elements are appended. Otherwise the first item is considered selected and the scroll position of the multiselect isn't updated when it transitions to multi-mode.

It looks like we were actually doing this in the postMountHook:
https://github.com/facebook/react/blob/master/packages/react-dom/src/client/ReactDOMFiberSelect.js#L174-L183

But, by this point, the option items are already appended to the select. This is a change in the order of operations from React 15 to React 16 (children are now appended before attributes are added):

What if we made a createHook, and switched on the tag, similar to postMountHook?

That could look like:

export function createElement(
  type: string,
  props: Object,
  rootContainerElement: Element | Document,
  parentNamespace: string,) {
  // ...
  switch (type) {
    case 'select'
      ReactDOMFiberSelect.createHook(props);
      break;
  }
}

Though for this case, since it's the only element so far that has an issue, maybe we could just use an if

gaearon · 2018-08-01T22:53:58Z

the multiple attribute needs to be set before the option children elements are appended.

The comment should say this.

gaearon

Looks all right to me. Posted some nits.

@nhunzaker Please feel free to merge once they’re addressed.

gaearon · 2018-08-01T23:07:23Z

packages/react-dom/src/client/ReactDOMFiberComponent.js

+      // 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) {


In this context !! is redundant. You can drop it.

gaearon · 2018-08-01T23:07:46Z

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


Please change the comment to the suggestion in the discussion thread

nhunzaker · 2018-08-01T23:04:02Z

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


This isn't perfect, but could you update this comment to read like:

// 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

@gaearon Could this be clearer?

nhunzaker · 2018-08-01T23:09:00Z

packages/react-dom/src/client/ReactDOMFiberComponent.js

+      // 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');


We use property assignment in the postMountWrapper, could you do the same here?

postMountWrapper attempts to update options, but as at this moment select doesn't have any child appended, it makes no sense, it will iterate over empty options collection. However it should not cause any problems so if you think it is better approach I can do this change.

gaearon · 2018-08-01T23:09:55Z

What if props.multiple changes during an update, but we insert a child into it before the change? Is that also going to cause the same issue?

remove redundant conversion to bool type

aweary · 2018-08-02T06:02:19Z

Could we consider changing the order back to match React 15? I brought this up a while back but there was no follow up on it. Was reordering these operations intentional?

segoddnja · 2018-08-02T06:06:12Z

@gaearon Google chrome scrolls to selected option only once.

const s = document.createElement('select');
s.multiple = true;
for(let i = 0; i < 10; i++) {
  const o = document.createElement('option');
  o.text = i;
  o.value = i;
  s.appendChild(o);
}
//Scroll down to selected option
s.options[9].selected = true;
//Do not scroll up to selected option
s.options[9].selected = false;
s.options[0].selected = true;

So any further changes won't change scroll position.

gaearon · 2018-08-02T11:15:42Z

Could we consider changing the order back to match React 15? I brought this up a while back but there was no follow up on it. Was reordering these operations intentional?

Reordering was done in ea34204 as far as I can see. Ironically it was partially done because of <select>:

Finally, we need the ability to update an instance after all the children
have been insertion. Such as <select value={...} />. I called this
finalizeInitialChildren.

That said we can probably split setting initial properties in two phases as well (before and after appending the children). Do we have a proposal for which ones should go where?

segoddnja · 2018-08-02T21:26:02Z

I do not know about other cases when it is necessary to assign properties before appending the children. As it turned out this issue (as well as solution) was known long time ago. So, probably there are some other related issues in the list.

gaearon · 2018-08-02T21:39:13Z

What about the suggested fix in #12242 (comment)? Would that also solve the issue?

segoddnja · 2018-08-02T22:07:13Z

Well, it solves only issue described in the pull request title. But, this change doesn't fix the scrolling issue.

gaearon · 2018-08-02T22:20:56Z

Okay. That sounds good then. Can you please fix conflicts/CI?

Requested changes look good, though I think we still need to answer whether or not we're okay placing this fix where it is at.

nhunzaker · 2018-08-02T22:49:32Z

What if props.multiple changes during an update, but we insert a child into it before the change? Is that also going to cause the same issue?

I'm not sure, but:

That said we can probably split setting initial properties in two phases as well (before and after appending the children). Do we have a proposal for which ones should go where?

What if we just placed a hook here:

react/packages/react-dom/src/client/ReactDOMHostConfig.js

Lines 183 to 192 in 28cd494

    
             const domElement: Instance = createElement( 
        
               type, 
        
               props, 
        
               rootContainerInstance, 
        
               parentNamespace, 
        
             ); 
        
             precacheFiberNode(internalInstanceHandle, domElement); 
        
             updateFiberProps(domElement, props); 
        
             return domElement; 
        
           }

Maybe something like:

// ... createInstance()
  const domElement: Instance = createElement(
    type,
    props,
    rootContainerInstance,
    parentNamespace,
  );
  precacheFiberNode(internalInstanceHandle, domElement);
  updateFiberProps(domElement, props);
  
  // Open to a better name
  setPropsBeforeChildren(type, props, domElement)

  return domElement;
}

Then in ReactDOMFiberComponent:

function setPropsBeforeChildren(type, props, domElement) {
  if (type === 'select' && props.multiselect) {
    select.multiselect = true
  }
}

gaearon · 2018-08-03T01:15:12Z

@nhunzaker Is your suggestion technically different except code organization? I'm fine with either tbh.

gaearon · 2018-08-03T01:15:47Z

I guess a separate function makes it a bit harder to miss why we're doing it. And GCC will inline it anyway.

nhunzaker · 2018-08-03T01:25:21Z

@gaearon I don't think it introduces anything new. Purely code organization.

Honestly I'm okay with this how it is, though I do think it's worth starting catalog why these sorts of fixes need to happen at specific points in the process for a later refactor. There isn't really great place for a fix like this and adding an extra layer of indirection doesn't feel right.

nhunzaker · 2018-08-03T01:28:51Z

packages/react-dom/src/client/ReactDOMFiberComponent.js

+      // `select` elements automatically pick the first item.
+      // See https://github.com/facebook/react/issues/13222
+      if (type === 'select' && props.multiple) {
+        domElement.setAttribute('multiple', 'true');


Just for consistence with ReactDOMFiberSelect, can you assign this as a property, like:

domElement.multiple = true

gaearon · 2018-08-03T01:29:20Z

Might want to start a document with a list of hacks for any input type and reasons for them.

nhunzaker · 2018-08-03T04:06:50Z

Sounds good. I'll start a markdown document and add it to the fixtures. Maybe it could be hosted along side a public version of the fixtures using Netlify (need to circle back to that too).

nhunzaker · 2018-08-03T16:52:29Z

@gaearon This looks good on my end and I have verified it locally.

nhunzaker · 2018-08-03T17:01:07Z

@segoddnja I pushed a small commit to remove an unused ref just now, but I believe this is good to merge.

Make sure that select has multiple attribute set to appropriate s…

2abb232

…tate before appending options fixes facebook#13222

facebook-github-bot added the CLA Signed label Jul 25, 2018

segoddnja force-pushed the master branch from 4709cc0 to 2abb232 Compare July 27, 2018 10:33

Add dom test fixture to test long multiple select scrolling to the fi…

d7c57e5

…rst selected option fixes facebook#13222

gaearon reviewed Aug 1, 2018

View reviewed changes

segoddnja force-pushed the master branch 2 times, most recently from 473b52f to d7c57e5 Compare August 1, 2018 21:50

typo fix

5d8d46a

gaearon reviewed Aug 1, 2018

View reviewed changes

nhunzaker previously requested changes Aug 1, 2018

View reviewed changes

update comment

b17ed8c

remove redundant conversion to bool type

Merge remote-tracking branch 'upstream/master'

b58cc88

nhunzaker reviewed Aug 3, 2018

View reviewed changes

segoddnja force-pushed the master branch from 00a87ec to b58cc88 Compare August 3, 2018 06:05

change a way of assigning property to domElement

561efab

nhunzaker approved these changes Aug 3, 2018

View reviewed changes

Remove unused ref on select fixture form

afedb68

gaearon merged commit 2d0356a into facebook:master Aug 3, 2018

This was referenced Aug 3, 2018

Add a regression test for #12200 #12242

Merged

Regression: React 16 automatically marks first item of a mutliple select as checked #12200

Closed

Fix/12200 regresssion select multiple #12240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure that `select` has `multiple` attribute set to appropriate state before appending options #13270

Make sure that `select` has `multiple` attribute set to appropriate state before appending options #13270

segoddnja commented Jul 25, 2018 •

edited

pull-bot commented Jul 25, 2018 •

edited

react-dom

nhunzaker commented Jul 26, 2018

segoddnja commented Jul 27, 2018

gaearon Aug 1, 2018

gaearon commented Aug 1, 2018

segoddnja commented Aug 1, 2018

gaearon commented Aug 1, 2018

nhunzaker commented Aug 1, 2018 •

edited

gaearon commented Aug 1, 2018

gaearon left a comment •

edited

gaearon Aug 1, 2018

gaearon Aug 1, 2018

nhunzaker Aug 1, 2018

nhunzaker Aug 1, 2018

segoddnja Aug 2, 2018

gaearon commented Aug 1, 2018

aweary commented Aug 2, 2018

segoddnja commented Aug 2, 2018

gaearon commented Aug 2, 2018

segoddnja commented Aug 2, 2018

gaearon commented Aug 2, 2018

segoddnja commented Aug 2, 2018

gaearon commented Aug 2, 2018

nhunzaker commented Aug 2, 2018 •

edited

gaearon commented Aug 3, 2018

gaearon commented Aug 3, 2018

nhunzaker commented Aug 3, 2018

nhunzaker Aug 3, 2018

gaearon commented Aug 3, 2018

nhunzaker commented Aug 3, 2018

nhunzaker commented Aug 3, 2018

nhunzaker commented Aug 3, 2018

Make sure that select has multiple attribute set to appropriate state before appending options #13270

Make sure that select has multiple attribute set to appropriate state before appending options #13270

Conversation

segoddnja commented Jul 25, 2018 • edited

pull-bot commented Jul 25, 2018 • edited

react-dom

nhunzaker commented Jul 26, 2018

segoddnja commented Jul 27, 2018

gaearon Aug 1, 2018

Choose a reason for hiding this comment

gaearon commented Aug 1, 2018

segoddnja commented Aug 1, 2018

gaearon commented Aug 1, 2018

nhunzaker commented Aug 1, 2018 • edited

gaearon commented Aug 1, 2018

gaearon left a comment • edited

Choose a reason for hiding this comment

gaearon Aug 1, 2018

Choose a reason for hiding this comment

gaearon Aug 1, 2018

Choose a reason for hiding this comment

nhunzaker Aug 1, 2018

Choose a reason for hiding this comment

nhunzaker Aug 1, 2018

Choose a reason for hiding this comment

segoddnja Aug 2, 2018

Choose a reason for hiding this comment

gaearon commented Aug 1, 2018

aweary commented Aug 2, 2018

segoddnja commented Aug 2, 2018

gaearon commented Aug 2, 2018

segoddnja commented Aug 2, 2018

gaearon commented Aug 2, 2018

segoddnja commented Aug 2, 2018

gaearon commented Aug 2, 2018

nhunzaker commented Aug 2, 2018 • edited

gaearon commented Aug 3, 2018

gaearon commented Aug 3, 2018

nhunzaker commented Aug 3, 2018

nhunzaker Aug 3, 2018

Choose a reason for hiding this comment

gaearon commented Aug 3, 2018

nhunzaker commented Aug 3, 2018

nhunzaker commented Aug 3, 2018

nhunzaker commented Aug 3, 2018

Make sure that `select` has `multiple` attribute set to appropriate state before appending options #13270

Make sure that `select` has `multiple` attribute set to appropriate state before appending options #13270

segoddnja commented Jul 25, 2018 •

edited

pull-bot commented Jul 25, 2018 •

edited

nhunzaker commented Aug 1, 2018 •

edited

gaearon left a comment •

edited

nhunzaker commented Aug 2, 2018 •

edited