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

should validate attributes by element #5963

Closed
yiminghe opened this issue Feb 2, 2016 · 5 comments
Closed

should validate attributes by element #5963

yiminghe opened this issue Feb 2, 2016 · 5 comments

Comments

@yiminghe
Copy link
Contributor

yiminghe commented Feb 2, 2016

We mistakenly used required attribute to style label element, such as

<label required>
<label>

label[required] {
  color: red;
}

Soon we found that label's attribute can not be removed in 0.14.x:

ReactDOM.render(<label required>, container);
// update immediately
ReactDOM.render(<label>, container);

After running the above code, The required attribute is still on label element.

Then I realize required is not an attribute of label by standard, just an attribute of some other elements(like input, etc..), so I think required should not be allowed to render at component mount phase like other arbitrary names(such as xyz), else it will cause people confusion.

And I notice that 0.15.x solves this problem by a flag(MUST_USE_PROPERTY), so required is rendered at component mount and will be removed correctly, but is inconsistent with other attributes(select/checked) which is not rendered at all.

So should react validate attributes by element like validateDOMNesting?

@jimfb
Copy link
Contributor

jimfb commented Feb 2, 2016

We have a whitelist of attributes, but don't check to ensure they're used on the corresponding tags. With regards to validation, are probably headed in the opposite direction with issues like #140.

However, this may be a reason to set the attribute even if we are already setting the property.

@yiminghe
Copy link
Contributor Author

yiminghe commented Feb 2, 2016

I have tested it...

required will be removed by removeAttribute in 0.15.x, but in 0.14.x it will run node.required=undefined to keep required.

@jimfb
Copy link
Contributor

jimfb commented Feb 2, 2016

Ah, you are correct. FWIW, we also don't specify the other attributes such as checked/selected in 0.15, unless you are using server side rendering.

@yiminghe
Copy link
Contributor Author

yiminghe commented Feb 2, 2016

My mistake, I have edited...

checked/selected are MUST_USE_PROPERTY in 0.15.x, so they are not rendered at all (use node.xx to set)

@jimfb
Copy link
Contributor

jimfb commented Feb 2, 2016

I'm going to close this out in favor of #5966, since that's the only actionable part of this issue, as far as I can tell.

@jimfb jimfb closed this as completed Feb 2, 2016
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

2 participants