Add a regression test for #12200 #12242
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GarethSmall
changed the title
|
@aweary thoughts? |
|
@aweary Would you mind taking a look at this? I just ran into this bug and this seems like a good fix. I tested it locally (after manually copy/pasting to an up-to-date fork) and everything worked as expected :) |
nhunzaker
reviewed
Jun 8, 2018
| @@ -182,6 +182,8 @@ export function initWrapperState(element: Element, props: Object) { | |||
|
|
|||
| export function postMountWrapper(element: Element, props: Object) { | |||
| const node = ((element: any): SelectWithWrapperState); | |||
| //Set selected index to -1 before doing anything else to prevent first option from being selected by default | |||
| node.selectedIndex = -1; | |||
| node.multiple = !!props.multiple; | |||
There was a problem hiding this comment.
Possibly chasing ghosts, but I'm worried about the side-effects of always assigning selectedIndex to -1. Do you think it makes sense to only apply this for the multiple input case?
const multiple = !!props.multiple;
if (multiple) {
node.selectedIndex = -1
node.multiple = true
}There was a problem hiding this comment.
Good suggestion, since this only occurs in the event of multiple this makes sense.
nhunzaker
previously requested changes
Jun 8, 2018
|
Sounds good, ill add them this evening. |
gaearon
force-pushed
the
fix/12200-regresssion-select-multiple-2
branch
from
70b50f0
to
43a7d6d
Compare
gaearon
changed the title
|
We merged a different fix in #13270. I'm taking this for the regression unit test though. Thank you so much! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
When creating a select element and setting multiple the first element would be selected. This was due to the multiple prop being set after the children we're appended.
An non-react example case created by @aweary can been seen here - https://jsfiddle.net/dm6vkq9q/
Tradeoffs / Misc
In my original solution I added a new hook to set props for just the select element before the children we're appended. This worked only because it was setting just the select props first. An alternative idea proposed was to switch the order of appendChildren > setProps to setProps > appendChildren. this will not work in the instance that we have a defualtValue set for our select input. When we change this order a defaultValue is ignored. There we're also many other tests that broke in this process. Although this is bit of a hacky solution this will not require us to make any dangerous changes to the code base.
Solution
In postMountWrapper in ReactDOMFiberSelect.js I set selectedIndex for the select element to -1
Issue
#12200
Related closed pull requests
#12240
@aweary @gaearon