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

<select> behaviour should be consistent with DOM when <option>s values are identical #7203

Closed
chicoxyzzy opened this issue Jul 6, 2016 · 17 comments

Comments

@chicoxyzzy
Copy link
Contributor

chicoxyzzy commented Jul 6, 2016

This is created as opposite to #6959.

There is a bug in React and it should be fixed to match DOM behaviour instead of solution proposed in #6959.

See #6959 (comment)

@chicoxyzzy
Copy link
Contributor Author

Also there is #7054 which shouldn't be merged for the reasons given above

@jimfb
Copy link
Contributor

jimfb commented Jul 6, 2016

What exactly do you think the desired behavior should be? As far as I can tell, React currently behaves about as reasonably as could be expected, which is why the proposal was to add a warning.

Given the options:

<option value="fruit">banana</option>
<option value="vegetable">broccoli</option>
<option value="fruit">orange</option>
<option value="vegetable">tomato</option>

And the selected value specified to be vegetable, what do you think React SHOULD do?

@chicoxyzzy
Copy link
Contributor Author

@jimfb there is an example in comment I mentioned above #6959 (comment)

So I think React should do the same — select proper option with value of that option

@aweary
Copy link
Contributor

aweary commented Jul 6, 2016

@chicoxyzzy your example shows the behavior when a user selects an option manually. What is React supposed to do if value is set to this.state.value and someone calls

setState({ value: 'vegetable' }

Should it set it to "broccoli" or "tomato"? It seems reasonable to me to just select the first matching option and warn, since this is likely not be the behavior the user wants.

@chicoxyzzy
Copy link
Contributor Author

chicoxyzzy commented Jul 6, 2016

@aweary exactly the same what DOM does:
http://jsbin.com/ruxekefova/1/edit?html,output

@aweary
Copy link
Contributor

aweary commented Jul 6, 2016

So specifically you're asking that it selects the first item from the list of <options /> with equal values, rather than selecting the last item, which is what React seems to do?

That's reasonable, but I don't see how that change would make a warning any less helpful.

@chicoxyzzy
Copy link
Contributor Author

So you propose to add warning and continue to violate consistency just because React doesn't follow the spec when it can (and should)?

@chicoxyzzy
Copy link
Contributor Author

React should select valid options on mouse and keybord events

@aweary
Copy link
Contributor

aweary commented Jul 6, 2016

So you propose to add warning and continue to violate consistency just because React doesn't follow the spec when it can (and should)?

I said it was reasonable for React to select the first item, just like your DOM example does. How would that be inconsistent? Adding a warning about using duplicate values in a controlled component is totally reasonable, value should be unique in that case.

@chicoxyzzy
Copy link
Contributor Author

@aweary try to change select by mouse or keyboard in both examples

@chicoxyzzy
Copy link
Contributor Author

chicoxyzzy commented Jul 6, 2016

value should be unique in that case

it shouldn't. It's ok to have non-unique values in options in markup

@aweary
Copy link
Contributor

aweary commented Jul 6, 2016

it shouldn't. It's ok to have non-unique values in markup

I'm talking about controlled React components. You can render an uncontrolled <select /> component with multiple <option />s with duplicate values and mouse/button events work.

@jimfb
Copy link
Contributor

jimfb commented Jul 6, 2016

@chicoxyzzy I think you're talking past Aweary, instead of trying to understand what he is saying.

Everyone is in agreement that HTML allows non-unique values for uncontrolled inputs. The problem occurs when you try to specify one of those values using a controlled component. As per my comment: #7203 (comment)

You said:

So I think React should do the same — select proper option with value of that option

This does not make sense. There is no "proper option". There is only a set of possible options that specify a matching value. So you never actually answered my question: what do you believe React SHOULD do when the user specifies the value "vegetable"?

@aweary
Copy link
Contributor

aweary commented Jul 6, 2016

React has to manage the value of the <select /> element in JavaScript, you have to think about it in that context. Imagine trying to do that in vanilla JS

https://jsfiddle.net/kcbbxdej/

  <select id="lol">
    <option value="a">1</option>
    <option value="a">2</option>
    <option value="b">3</option>
  </select>
var id = document.getElementById('lol')
id.addEventListener('change', function(event) {
  id.value = event.target.value
})

How is the change event listener supposed to identify which option is the "right" option? If you run that example, you'll see it doesn't work. This is analogous to a React controlled component, while your example is analogous to an uncontrolled component.

@chicoxyzzy
Copy link
Contributor Author

OK. Shouldn't it warn only in case when <select> is controlled element (i.e. if it has value prop) and it has duplicate options?

Duplicate options could be desired business logic. In some cases warning will be emitted for proper (uncontrolled) select and not emitted for potentially dangerous controlled select (because in #7054 warning will be shown only once)

@jimfb
Copy link
Contributor

jimfb commented Jul 6, 2016

https://github.com/facebook/react/pull/7054/files#diff-66f0da1e178fdfa217ecf74282c8bdf7R40

@chicoxyzzy
Copy link
Contributor Author

OK

this makes sense than. Thank you. I'll close this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants