Checkbox attribute unnecessarily removed from DOM on every render

[akre54]

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

Rendering a list of controlled checkboxes causes the reconciler to unnecessarily update (add or remove) the checked attribute on every render, regardless of whether the value has changed.

I have a table with ~300-500 rows that needs to add a "highlighted" class to the rows based on some user interaction on another part of the screen. I'd expect that the table renderer would get called on every interaction, but I was seeing ~40ms render times in the Perf tool. I dug into it and it looks like every render the reconciler removes the "checked" attribute (see JSFiddle). The checked attribute on the row datum never changes; is always false.

Extracting the rows into their own component and using componentShouldUpdate fixes the slowness, but this seems like a bug.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/reactjs/69z2wepo/).

Here are two JSFiddles demonstrating the issue:

Minimal repro: https://jsfiddle.net/69z2wepo/49455/

Full repro: https://jsfiddle.net/69z2wepo/49466/

What is the expected behavior?

The reconciler shouldn't be touching the DOM for attributes that haven't changed.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Mac OSX 10.11. Chrome 51, Safari 9.1, Firefox 44

React Version 15.0.1.

[barockok]

I just see your jsfiddle, I thought your Table component should look like this, isn't it ?

var Table = React.createClass({
  render: function() {
    const { data, highlighted } = this.props
    return (
        <div>
            {data.map(d => (
            <div key={d.id} className={`${highlighted && highlighted.id == d.id ? 'highlighted' : ''} row`}>
            <input type="checkbox" checked={d.checked} />
            {d.id}
          </div>
        ))}
        </div>
    )
  }
})
``

[akre54]

Ah yeah good catch. Was in a rush when posting this. Fixed link above.

[akre54]

Ok I did some digging. It looks like the shouldIgnoreValue from setValueForProperty is always true when value is false, and always false when value is true (due to this line -- hasBooleanValue is true for <input type='checkbox' />.)

This means that in the false value case it is always deleting the attribute. I seem to recall old browsers needing fancy handling around checkboxes. Maybe this is why?

In the true value case, it's always triple-equals checking the node.checked property (a boolean) against the value cast to a string ('' + value). Even if node.checked is true and value is true, this check will return false. This seems incorrect to me.

I haven't looked into v16alpha yet, but I see this logic was rewritten.

[akre54]

It looks like the behavior changed for the true branch on master, but is still wrong on the false branch.

The original change is here: 77a137a, the latest change is #6987. Without investigating too deeply it looks like moving the mustUseProperty branch to above the shouldIgnoreValue or guarding against doing extra work in deleteValueForProperty should be sufficient.

[gaearon]

Can you confirm if this is still happening with React 16?

[akre54]

Looks like Perf is gone in 16 so the fiddle doesn't run the same, but running the minimal example on RequireBin (link) seems to be fixed.

[gaearon]

Thanks for checking. (Yea, we haven't figured out how to recreate Perf with the new engine yet. Its assumptions don't really hold true in the async case.)

[gaearon]

Also thanks for your Backbone packages! They were some of the first open source JS packages I used in my life. :-)

[akre54]

Glad to help! I'm a huge fan of redux so that means a lot