-
Notifications
You must be signed in to change notification settings - Fork 46.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't set the first option as selected in select tag with size
attribute
#14242
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like, this does essentially the same thing as setting multiple=true
before options are appended because they visually become similar:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to add a comment clarifying that assigning size
early is only necessary when multiple
isn't set, but that's not a huge deal.
I can go ahead and commit that if you'd like. Otherwise, this is great! Thank you!
@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 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion regarding variable.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
size
attribute needs to be added toselect
beforeoption
's are inserted.Currently with
size
greater than 1 and withoutmultiple
attribute added, the first option is selected which is not expected result in that case.