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

[megawac]

Do you want to request a feature or report a bug?

Bug - Regression

What is the current behavior?

In react 16 when creating a <select multiple> the first child <option> is automatically getting marked as selected. In React 16 there does not seem to be a way to specify no <option> gets selected by default

https://codesandbox.io/s/moxm2on3z9

What is the expected behavior?

In React 15 unless you marked an option to be selected <option selected> no options were selected by default.

https://codesandbox.io/s/ll11z5wqzl

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Versions effected include react^16.2, this worked in react^0.13 and react^15. This bug is reproducible in chrome 64 and Firefox 58.

---

The hacky workaround I found to get around the first option getting selected is to inject a <option style={{display: 'none'}} /> as the first child of the multiselect.

[gaearon]

cc @aweary

[jquense]

The change was made specifically for the single select since that's what the specced behavior is supposed to be. It seems like this behavior is only specified when the size is 1, which is a confusing complication because one can do <select multiple size="1">. The DOM is so fun folks.

ref: https://html.spec.whatwg.org/multipage/form-elements.html#concept-select-size

[jquense]

jsfiddle showing non-react behavior: https://jsfiddle.net/wvr6f6x3/

• multiple: false -> selects first option

• multiple: false, size: 4 -> doesn't select first option

• multiple: true -> doesn't select first option

• multiple: true, size: 1 -> doesn't select first option

Same in Chrome, Safari, and FF (random bonus there is like 7 year old bug in webkit where size has no affect)

looks like the logic is, "only select the first option when it's not displayed like a listbox"

[aweary]

The last time we touched this select logic was in #11057, which doesn't actually affect the behavior we're seeing here (updateOptions isn't called at all)

The reason we see this regression is because the order of operations between when attributes are set and children are appended has changed. In React 15, attributes were set on an element before the children were appended. This means that multiple was set on select before any option elements were appended.

In React 16, children are appended before attributes are set on the parent. So all the option elements are appended before multiple is set.

react/packages/react-reconciler/src/ReactFiberCompleteWork.js

Lines 507 to 519 in 467b103

	appendAllChildren(instance, workInProgress);
	// Certain renderers require commit-time effects for initial mount.
	// (eg DOM renderer supports auto-focus for certain elements).
	// Make sure such renderers get scheduled for later work.
	if (
	finalizeInitialChildren(
	instance,
	type,
	newProps,
	rootContainerInstance,
	currentHostContext,
	)

finalizeInitialChildren is what calls setInitialProperties and is called after appendAllChildren

This exposes some nuanced behavior around how option elements work: if you append a child to a select element that is not multiple it updates select.selectedIndex from -1 to 0. It does not do that if you append a child to a multi-select.

So what happens in React 16 is that we append all the options to a select element that it assumes is a single select, and we see the spec defined behavior @jquense mentioned.

Here's an example reproducing this issue without React. You'll see that if you set select.multiple to true before appending the options, it works as expected.

[aweary]

The solution will likely be either:

1. Ensure that attributes are set before children are appended. This would make React 16 consistent with 15. It would potentially resolve any other unknown issues that will arise due to this, but I think it will also require changes to the reconciler (maybe?) which may affect other renderers with different requirements

2. Have a special set of attributes that are set before children are appended

3. Go back after all the attributes and children are set and manually fix this in postMountWrapper from ReactDOMSelectFiberSelect

The first option is probably the best one if it doesn't cause any other problems. The third option is probably the easiest, but more of a hacky solution.

[GarethSmall]

@aweary Do you mind if I take this on as my first issue?

[jquense]

@GarethSmall go for it 👍 let us know if you have any questions, it's not the most straightforward are of the code base :P

[GarethSmall]

@jquense Thank you! I've got many, i'm looking through the tests now. If I have any afterwards, I'll be sure to ask.

[aweary]

Continuing the discussion from #12240

I think it makes the most sense to ensure that all attributes are set before children are appended. I'll try and comb through the spec for more append behavior that forks based on parent attributes, but my gut tells me multiple isn't unique. If it is unique, or there are other attributes that depend on children being appended before the attribute is set, then maybe just manually fixing this in ReactDOMSelectFiberSelect is the best idea.

The problem with just re-ordering the reconciler methods is that this might not make sense for other renderers like React Native. Maybe @acdlite or @bvaughn have some insights there?

Maybe we could put the setInitialDOMProperties call somewhere else in the lifecycle. I'm not sure.

[GarethSmall]

From what it looks like we're doing two things in setInitialProperties, we're setting the initial properties and we're "handling" events. Is it fair to say these two processes should be separated?

Also, can we confirm this is an issue in other areas? I'm going to do some tests in fiddle and see if I can cause any issues from this order.

[GarethSmall]

Another thing to note is if we set the properties before we append the children we run into other issues. For example: We have a defaultValue property we set properties > append all children our defaultValue won't be set.

I think as you suggested manually fixing this in ReactDOMSelectFiberSelect may be the best option

[gaearon]

This was fixed in #13270, with an extra regression test in #12242.