Skip to content
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

Merged
merged 3 commits into from
Mar 14, 2019

Conversation

kulek1
Copy link
Contributor

@kulek1 kulek1 commented Nov 15, 2018

Fixes #14239

size attribute needs to be added to select before option's are inserted.
Currently with size greater than 1 and without multiple attribute added, the first option is selected which is not expected result in that case.

@sizebot
Copy link

sizebot commented Nov 15, 2018

ReactDOM: size: 0.0%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: d7fd679...fa175ca

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 716.11 KB 716.62 KB 165.75 KB 165.91 KB UMD_DEV
react-dom.production.min.js 0.0% 🔺+0.1% 97.66 KB 97.68 KB 31.8 KB 31.81 KB UMD_PROD
react-dom.profiling.min.js 0.0% -0.0% 100.56 KB 100.58 KB 32.5 KB 32.5 KB UMD_PROFILING
react-dom.development.js +0.1% +0.1% 711.42 KB 711.93 KB 164.38 KB 164.54 KB NODE_DEV
react-dom.production.min.js 0.0% 🔺+0.1% 97.65 KB 97.68 KB 31.31 KB 31.33 KB NODE_PROD
react-dom.profiling.min.js 0.0% +0.1% 100.65 KB 100.68 KB 31.93 KB 31.95 KB NODE_PROFILING
ReactDOM-dev.js +0.1% +0.1% 732.57 KB 733.08 KB 165.51 KB 165.69 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.2% 🔺+0.1% 309.63 KB 310.13 KB 57.06 KB 57.1 KB FB_WWW_PROD
ReactDOM-profiling.js +0.2% +0.1% 316.53 KB 317.03 KB 58.46 KB 58.51 KB FB_WWW_PROFILING

Generated by 🚫 dangerJS

if (props.multiple) {
node.multiple = true;
} else if (props.size) {
node.size = props.size;
Copy link
Contributor

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?

Copy link
Contributor

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:

https://codepen.io/nhunzaker/pen/VgRQEy

Copy link
Contributor

@nhunzaker nhunzaker left a 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!

@kulek1
Copy link
Contributor Author

kulek1 commented Feb 20, 2019

@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
@nhunzaker
Copy link
Contributor

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.

@nhunzaker
Copy link
Contributor

Also pinged the DOM team for a review, so I'm not just approving my own changes :)

Copy link
Contributor Author

@kulek1 kulek1 left a 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.

Copy link
Contributor

@philipp-spiess philipp-spiess left a 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) {
Copy link
Contributor

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.

@nhunzaker
Copy link
Contributor

@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.

@nhunzaker
Copy link
Contributor

Okay! This checks out in:

  • Desktop Safari 8, 12
  • iOS Safari 9, 12
  • Chrome 49, 72
  • Firefox 52, 65
  • IE 9, 10, 11
  • Edge 18

I checked for:

  • Initial selection is blank
  • Selections are possible
  • Only one selection is possible

Thanks @kulek1 for sending in this PR!

@nhunzaker nhunzaker merged commit ab5fe17 into facebook:master Mar 14, 2019
gaearon pushed a commit to gaearon/react that referenced this pull request Mar 22, 2019
…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
@gaearon gaearon mentioned this pull request Mar 22, 2019
This was referenced Oct 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants