Select option empty value regression in 15.0.0-rc.1

[chicoxyzzy]

React 0.14.7: <option value="">empty</option> emits empty string on change
http://jsbin.com/pisita/10/edit?html,js,console,output
React 15.0.0-rc.1<option value="">empty</option> emits string with value "empty" on change
http://jsbin.com/bemaze/12/edit?html,js,console,output
This is because React 15.0.0-rc.1 cuts off value="" from option tag

[chicoxyzzy]

I don't know why jsbin doesn't save my changes...

[chicoxyzzy]

html for 0.14.7

<script src="https://fb.me/react-with-addons-0.14.7.min.js"></script>
<script src="https://fb.me/react-dom-0.14.7.min.js"></script>
<div id="app"></div>

html for 15.0.0-rc.1

<script src="https://fb.me/react-with-addons-15.0.0-rc.1.min.js"></script>
<script src="https://fb.me/react-dom-15.0.0-rc.1.min.js"></script>
<div id="app"></div>

jsx for both

function log(e) {
  console.log(e.target.value);
}

const Select = (
  <select onChange={log}>
    <option value="">empty</option>
    <option value="filled">filled</option>
  </select>
);

ReactDOM.render(
  Select,
  document.getElementById("app")
);

[chicoxyzzy]

I want to fix this. Can you point me to right place? I believe this is ReactDOM issue because Virtual DOM node looks ok.

[jimfb]

Yeah, I spent a few minutes looking for the PR that I suspect is responsible for this, but don't see it at the moment.

Without having actually dug into the code, my intuition is to look at https://github.com/facebook/react/blob/master/src/renderers/dom/client/wrappers/ReactDOMSelect.js and https://github.com/facebook/react/blob/master/src/renderers/dom/client/wrappers/ReactDOMOption.js

But don't rely on those tips too heavily, I don't want to send you off on a wild goose chase.

[everdimension]

I believe this commit introduces the bug (changes to file DOMPropertyOperations.js): 77a137a#diff-f04a397aa63ac69c28dab46e9863bb90L171

This code is responsible for setting attributes to the nodes being created.

It's not a straightforward fix:
— the older version checked the mustUseAttribute (which has now been removed) property and was pretty much setting the attributes in any case;
— current version checks for mustUseProperty property, but only sets the value attribute if it is not falsy.

It's questionable whether the current behavior is correct. Basically it checks whether the attribute is classified as HAS_SIDE_EFFECTS which is only true (at least for now) for the value attribute.
And then it does this quirky comparison ('' + node[propName]) !== ('' + value) to set the node property.

The option element is in a sort of unique position: If no value attribute value is provided, then the tag's content is used as value. If value attribute exists, even if it is empty, then the attribute is used as the value.

But, for example, the input[type=checkbox] (same for radio) also behaves similar in these cases:

• it returns on as its value if no value attribute is provided
• it returns the attribute's value if value exists, even if empty

The thing is, when the abovementioned check is done, before value attribute is applied, node.value for checkbox returns "on" and that's why the attribute is set as expected for checkboxes and radios, but not set for the option element.

Above all that, this behavior (returning option's content if no value is provided) doesn't seem to be mentioned by w3c, but is still implemented in browsers and is expected.

Anyway, to summarize, option element looks unique in the sense that it needs the value attribute explicitly set even if it is an empty string.

This PR #6228 fixes the issue.

[troydemonbreun]

@jimfb Was #1510 the PR you were looking for?

[syranide]

#1510 (comment) and #6119. However, it should be noted that this bug applies to input as well, it's just that you don't really notice it. The problem is due to the new renderer and HAS_SIDE_EFFECTS for value means that the first time value is set it's now set using the DOMPropertyOperations-logic instead which means the HAS_SIDE_EFFECTS case is applied, where previously it would be set as an attribute the first time regardless and HAS_SIDE_EFFECTS was thus not taken into consideration. Hence, this is actually a bug of the new renderer in a sense. If we're going with the solution in #6119 then that fixes this too as properties then goes away entirely and is handled in the wrappers instead.

cc @spicyj

[everdimension]

@syranide
I think we should set on the terms whether stripping out empty value attribute is a "bug" in general.

option element "needs" it because of specific browser handling, so do checkboxes and radios. So in these cases empty value should be left, because they might be user inteded.

In other cases though, as I understand it, only attributes classified as HAS_BOOLEAN_VALUE should be left. Other attributes, if empty, should be stripped.

So we either handle the value attribute as a special case, or we handle the option element as a special case.

[syranide]

@everdimension There's semantic/technical differences between <div title=""> and <div> even though there's no visual difference, the same applies to inputs. I don't really see a reason to special-case value for input here, but that's my opinion and I don't really mind either. Regardless, the problem and final solution is the same (#6119), if ReactDOMInput wants to special-case it in the end then that seems fine to me. Anyway, I leave it up to @spicyj or whoever.

[chicoxyzzy]

yep value="" have no any reasons to be stripped in any case

[everdimension]

Ok, I agree. I thought that perhaps not setting empty value for tags that don't use it (like divs) was a performance thing.

In that case it may be a good idea to have something like isInputLike() check after which, if it passes, value="" should be set.

For now I updated the pull request to handle all value="" cases.

[chicoxyzzy]

One can put value="" into any tag and that could be intended (but really weird tho). Let's say some jQuery plugin uses value attribute on divs to make it interactive or something

[jimfb]

FWIW, if you mount into something that was server side rendered, you get the value="" as desired:

document.getElementById("container").innerHTML = (ReactDOMServer.renderToString(Select));
ReactDOM.render(Select, document.getElementById("container"));

A potential solution here would be to stop treating value as a special property, and just set it as an attribute, which means this could also be solved by #5680 (I think).

[chyzwar]

@chicoxyzzy value="" is permitted only for handful of elements according to HTML spec, you can use data-* custom attributes. https://developer.mozilla.org/en/docs/Web/Guide/HTML/Using_data_attributes