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

Size prop is not attached to the DOM #12694

Closed
airamrguez opened this issue Apr 26, 2018 · 8 comments · Fixed by #12702
Closed

Size prop is not attached to the DOM #12694

airamrguez opened this issue Apr 26, 2018 · 8 comments · Fixed by #12702

Comments

@airamrguez
Copy link
Contributor

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

Bug

What is the current behavior?

The size prop can only be a number and I think this should be true for inputs and selects but not necessarily true for the rest of HTML tags or web components.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

Sorry, it's a CodePen. This example uses a framework called OnsenUI that internally uses web components. In the OnsenUI framework there is an icon web component that transforms every size prop value into a string which is not attached to the DOM.

I know that the example has more dependencies than React, BUT there is also a simple div (as simple as this <div size="40px" />) which is also affected by this issue.

https://codepen.io/airamrguez/pen/bMEgEP

What is the expected behavior?

The size prop should be added to the DOM when it is not a number on HTML tags that aren't inputs and selects.

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

The affected versions are >= 16.3
It works ok in versions <= 16.2
Browsers: All

@gaearon
Copy link
Collaborator

gaearon commented Apr 26, 2018

The size prop can only be a number and I think this should be true for inputs and selects but not necessarily true for the rest of HTML tags or web components.

What are the HTML tags that have a size attribute except inputs and selects?

With web components I am not seeing the issue:

https://jsfiddle.net/bpyx02yj/

screen shot 2018-04-26 at 7 51 53 pm

@aweary
Copy link
Contributor

aweary commented Apr 26, 2018

@airamrguez the size attribute is an HTML attribute, not something that React provides. According to the HTML spec, size is only allowed on input and select elements

https://html.spec.whatwg.org/#attributes-3:attr-select-size

@airamrguez
Copy link
Contributor Author

@gaearon I don't know why I wrote "rest of HTML tags", my bad 😅. The example you provided works ok, but if I add something that doesn't seems like a valid size then it doesn't work. For example:

Value Works
100 Yes
10px No
10e1 Yes
-1 No

Something changed in 16.3.

Version 16.3
https://jsfiddle.net/sa30541v/

captura de pantalla 2018-04-26 a las 21 36 43

Version 16.2
https://jsfiddle.net/7umnLj0c/2/

captura de pantalla 2018-04-26 a las 21 37 10

@aweary I suppose that it's fine to filter props when not passing the appropriate values when using HTML tags but I suppose that in web components the size attribute can be anything. Am I wrong?

What do you think?

@gaearon
Copy link
Collaborator

gaearon commented Apr 26, 2018

The web component code path shouldn't reject any attributes. If it does it's a bug.

@gaearon
Copy link
Collaborator

gaearon commented Apr 26, 2018

The bug is here.
Before the switch we should add something like

if (isCustomComponentTag) {
  return false;
}

Please send a PR :-)

@airamrguez
Copy link
Contributor Author

Ok, I will work on this tomorrow. I'm heading to bed. 🙏🏽Thanks for your quick response.

@airamrguez
Copy link
Contributor Author

Couldn't wait

@gaearon
Copy link
Collaborator

gaearon commented May 24, 2018

Should be fixed in React 16.4.
https://reactjs.org/blog/2018/05/23/react-v-16-4.html

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

Successfully merging a pull request may close this issue.

3 participants