Don't set the first option as selected in select tag with size attribute
#14242
Conversation
|
ReactDOM: size: 0.0%, gzip: 🔺+0.1% Details of bundled changes.Comparing: d7fd679...fa175ca react-dom
Generated by 🚫 dangerJS |
| if (props.multiple) { | ||
| node.multiple = true; | ||
| } else if (props.size) { | ||
| node.size = props.size; |
There was a problem hiding this comment.
Small thing, but could you add a comment indicating that assigning size early is only necessary when multiple is false?
There was a problem hiding this comment.
It sounds like, this does essentially the same thing as setting multiple=true before options are appended because they visually become similar:
https://codepen.io/nhunzaker/pen/VgRQEy
|
@nhunzaker Sure, go ahead :) |
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
|
I added a clarifying comment and some additional information to the DOM text fixture. I also added a basic unit test, but uncovered a difference in behavior between JSDOM and Chrome. I left some information about that in the test for future work, like if we revisit the decision to assign attributes to selects after child options are attached. |
|
Also pinged the DOM team for a review, so I'm not just approving my own changes :) |
| @@ -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 = ( | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
@kulek1 Thanks for the PR!
@nhunzaker Do you think we should do some cross-browser tests just in case?
| node.multiple = true; | ||
| if (props.multiple) { | ||
| node.multiple = true; | ||
| } else if (props.size) { |
There was a problem hiding this comment.
I tested in Chrome and noticed that when a select element is marked as multiple, this issue is not present so the else if seems OK.
|
@philipp-spiess Ya. I did some browser testing, but I can't recall how comprehensive it was. I can take a sweep with the new fixture. |
|
Okay! This checks out in:
I checked for:
Thanks @kulek1 for sending in this PR! |
…ibute (facebook#14242) * Set 'size' attribute to select tag if it occurs before appending options * 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 * Use let, not const in select tag stub assignment
Fixes #14239
sizeattribute needs to be added toselectbeforeoption's are inserted.Currently with
sizegreater than 1 and withoutmultipleattribute added, the first option is selected which is not expected result in that case.