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

Fix DOMProperty bitmask checking #2113

Merged
merged 1 commit into from
Sep 9, 2014

Conversation

BinaryMuse
Copy link

[Was previously "Fix HAS_POSITIVE_NUMERIC_VALUE properties" — code and title updated to fix underlying problem]

The bitmask checks in DOMProperty do not properly handle masks with more than one bit set; specifically, properties marked HAS_NUMERIC_VALUE are also passing tests for HAS_POSITIVE_NUMERIC_VALUE, introduced in 5c9d616.

This bug manifests when attempting to use the start attribute on an ordered list to make the list start at 0. The following test case (also available on JSFiddle demonstrates):

<div id="list-1"></div>
<div id="list-2"></div>
<div id="list-3"></div>
<p>HTML starting at 0:
    <ol start="0">
        <li>First</li>
        <li>Second</li>
        <li>Third</li>
    </ol>
</p>
/** @jsx React.DOM */

var List = React.createClass({
    render: function() {
        return (
            <p>React starting at {this.props.start}:
                <ol start={this.props.start}>
                    <li>First</li>
                    <li>Second</li>
                    <li>Third</li>
                </ol>
            </p>
        );
    }
});

React.renderComponent(<List start="1" />, document.getElementById("list-1"));
React.renderComponent(<List start="0" />, document.getElementById("list-2"));
React.renderComponent(<List start="5" />, document.getElementById("list-3"));

The output of the above is:

React starting at 1:

  1. First
  2. Second
  3. Third

React starting at 0:

  1. First
  2. Second
  3. Third

React starting at 5:

  5. First
  6. Second
  7. Third

HTML starting at 0:

  0. First
  1. Second
  2. Third

@BinaryMuse BinaryMuse changed the title Fix HAS_POSITIVE_NUMERIC_VALUE properties Fix DOMProperty bitmask checking Sep 2, 2014
@zpao
Copy link
Member

zpao commented Sep 2, 2014

Seems like a good move. cc @yungsters

zpao added a commit that referenced this pull request Sep 9, 2014
@zpao zpao merged commit 9919104 into facebook:master Sep 9, 2014
@zpao
Copy link
Member

zpao commented Sep 9, 2014

Thanks!

josephsavona added a commit that referenced this pull request May 15, 2024
This is the optimization mentioned in #2113. When we merge scopes, often the 
declarations from the first scope become unnecessary, since those values are 
only consumed by the subsequent, now-merged scope. There's no point emitting 
those values as outputs of the merged scope since no one can consume them. 

Thanks to the logic in #2116 we now know the last place each identifier is used. 
We use that again here, to prune declarations that aren't used past the end of 
the merged scope. This is a pretty dramatic win on cache slots used.
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

Successfully merging this pull request may close these issues.

2 participants