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

Boolean attributes on Web Components #9230

Closed
nickdima opened this issue Mar 21, 2017 · 21 comments · Fixed by #24541
Closed

Boolean attributes on Web Components #9230

nickdima opened this issue Mar 21, 2017 · 21 comments · Fixed by #24541

Comments

@nickdima
Copy link

When setting boolean attributes on Web Components
<x-search somebool name={this.props.name} />
they get rendered as attribute/value pairs instead
<x-search somebool="true" name={this.props.name}>

This causes problems with some AMP components, for eg. <amp-iframe /> which has a resizable attribute that gets rendered as resizable="true". This results in non valid AMP content and developers crying with blood tears on their keyboards :)

The attribute 'resizable' in tag 'amp-iframe' is set to the invalid value 'true'. (see https://www.ampproject.org/docs/reference/components/amp-iframe)

Is there any reason for the current behaviour? Will changing this break something else?

@krizzu
Copy link

krizzu commented Mar 22, 2017

Hey @nickdima

This behavior is described in Docs (old jsx gotchas) here.

So basically, if You pass a prop without a value, it defaults to "True"


Quick edit:

Digging deeper, this behavior (React Docs also point that) is standard behavior of html5 boolean attributes. Simply put, when you have an input element:
<input type="checkbox" checked />

the presence of checked attribute, represents the true value. Without it, checked would fall back to false.

@nickdima
Copy link
Author

nickdima commented Mar 22, 2017

Digging deeper, this behavior (React Docs also point that) is standard behavior of html5 boolean attributes.

Well yeah, but React is doing the contrary in this case, id doesn't render <some-tag somebool /> but <some-tag somebool="true />.
I know it works for known tags like input for disabled and checked but it doesn't work for unknown tags and attributes.

@aweary
Copy link
Contributor

aweary commented Mar 22, 2017

Thanks for the report @nickdima! Current React just passes attributes through if the element is a custom element. So if you pass somebool={true} the attribute will get set via setAttribute('somebool', true) which will set the attribute value to "true".

You can verify that's exepcted behavior with:

var element = document.createElement('some-tag');
element.setAttribute('somebool', true);
// <some-tag somebool="true"></some-tag>

According to the HTML5 spec on boolean attributes

If the attribute is present, its value must either be the empty string or a value that is a case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace.

So as a workaround you should be able to do <some-tag somebool=""></some-tag> and it should render as <some-tag somebool></some-tag>.

I'm not sure if that will accomplish what you want, as it probably depends on how AMP is reading these attributes. Attributes like checked or disabled are reflected properties, so element.setAttribute('checked', '') sets element.checked === true . I'm assuming that Web Components should take care of that for you then?

React can potentially be smarter about this and coerce true to '' to comply with the boolean attribute spec.

@nickdima
Copy link
Author

Thanks @aweary! The workaround <some-tag somebool=""></some-tag> seems to do the trick!
Should we document this somewhere?

@aweary
Copy link
Contributor

aweary commented Mar 22, 2017

@nickdima glad to hear it! I'm not sure yet, it will depend on whether we'll want to just coerce true to '' internally for custom element attributes. cc @spicyj what do you think?

@sophiebits
Copy link
Collaborator

I don't remember all the history here, but I think we assume that custom elements are string-typed, not boolean-typed. For all string-typed attributes we just coerce to strings (even for boolean values). So like <a href={true} /> makes href="true". I'm inclined to say we'll leave this current behavior.

@aweary
Copy link
Contributor

aweary commented Mar 22, 2017

@spicyj Is there any reason to assume booleans should be coerced to strings? Are there any cases where attribute={true} should be rendered as attribute="true" and not attribute?

If we leave the current behavior then we should definitely document how to render boolean attributes.

@aweary
Copy link
Contributor

aweary commented Apr 5, 2017

ping @spicyj, what do you think about ^?

@robdodson
Copy link

I think there may be an issue here because if the boolean is false, React will set <my-element somebool="false">. Typically that still evaluates to true. For instance, <input type="checkbox" checked="false"> will still produce a checked checkbox. I think removing the boolean attribute is the typical practice.

I guess the distinction to be drawn here is between enumerated attributes (which expect "true"/"false") and boolean attributes. Some examples of enumerated attributes are things like aria-hidden="true" and as @syranide pointed out in #9779, spellcheck="true".

Taking a bigger step back, I think switching to passing properties to custom elements would be really nice as it sort of sidesteps some of these issues. But it doesn't resolve what to do in the AMP case if AMP requires an attribute instead of a property :\

@robdodson
Copy link

I looked at what Preact does here and I think their solution is pretty decent. If the value is a boolean it will add or remove the attribute. If the value is a string it will set the attribute to the string. Example:

let val;

val = true;
<x-foo bool={val}></x-foo>
// output: <x-foo bool="true"></x-foo>

val = false;
<x-foo bool={val}></x-foo>
// output: <x-foo></x-foo>

val = "true";
<x-foo bool={val}></x-foo>
// output: <x-foo bool="true"></x-foo>

val = "false";
<x-foo bool={val}></x-foo>
// output: <x-foo bool="false"></x-foo>

This seems like it would allow you to support boolean attributes (checked, disabled) and enumerable attributes (spellcheck, aria-hidden).

@jessepinho
Copy link

For TypeScript users, I noticed that TypeScript complains if I pass an empty string as the value. So I'm getting around that by passing undefined:

    <Helmet>
      <script
        async={undefined}
        custom-element="amp-bodymovin-animation"
        src="https://cdn.ampproject.org/v0/amp-bodymovin-animation-0.1.js"
      />
    </Helmet>

This renders as <script data-react-helmet="true" async custom-element="amp-bodymovin-animation" src="https://cdn.ampproject.org/v0/amp-bodymovin-animation-0.1.js"></script>. Hope this helps!

TheDadi referenced this issue in axa-ch-webhub-cloud/pattern-library May 26, 2018
@itsmelion
Copy link

itsmelion commented Jul 4, 2018

I think boolean attributes should be allowed as they were declared.
my css framework detects custom attributes, and having to type attr="" instead of attr is kinda annoying.

take a look:
<section row flex nowrap align="around center" />
<section row="" flex="" nowrap="" align="around center" />

@BillSchumacher
Copy link

Yall broke working code. I should be able to pass false in a component property and not throw an error like it used to, very annoying.

@morewry
Copy link

morewry commented Jul 9, 2019

So...hold up. You're telling me there's no way to get React to call .removeAttribute()?

@MichaelDimmitt
Copy link

MichaelDimmitt commented Dec 7, 2019

Issue is still open +1

@Flammae
Copy link

Flammae commented Dec 17, 2021

hey, so it looks like the fix was merged and removed latter on. the problem is still there. passing booleanAttr={false} to a custom components still resolves to component being rendered with booleanAttr="false", which then fails the Element.hasAttribute("booleanAttr") test. doing something like booleanAttr={condition ? true : ""} is a good workaround, but is not consistent with React's behavior with regular elements and would create confusion with developers who did not write custom component but are using it in react

@andrewcovenant
Copy link
Contributor

@gaearon Can I work on this as a first contribution?

@horuskol
Copy link

horuskol commented Apr 10, 2022

hey, so it looks like the fix was merged and removed latter on. the problem is still there. passing booleanAttr={false} to a custom components still resolves to component being rendered with booleanAttr="false", which then fails the Element.hasAttribute("booleanAttr") test. doing something like booleanAttr={condition ? true : ""} is a good workaround, but is not consistent with React's behavior with regular elements and would create confusion with developers who did not write custom component but are using it in react

I think you'll find that booleanAttr={condition ? true : ""} won't pass - even setting the attribute to an empty string will leave the attribute in the markup.

I found that booleanAttr={condition ? '' : null} is the way - though it would be better if this was just handled by booleanAttr={condition}

@gaearon
Copy link
Collaborator

gaearon commented Apr 10, 2022

Has this been fixed in the @experimental releases? Can somebody check please?

@eps1lon
Copy link
Collaborator

eps1lon commented May 10, 2022

Has this been fixed in the @experimental releases? Can somebody check please?

It's still adding attributes with the stringified value: https://codesandbox.io/s/web-components-boolean-props-lgbhp0?file=/src/index.js

@eps1lon
Copy link
Collaborator

eps1lon commented May 11, 2022

Re: #9230 (comment)

val = true;
<x-foo bool={val}></x-foo>
// output: <x-foo bool="true"></x-foo>

@robdodson Wouldn't this still be problematic with the initial case of <amp-iframe resizeable /> where resizeable should be "" not "true" like Preact does?

For enumerated attributes like aria-hidden you should then render <custom-element aria-hidden="true" /> instead of <custom-element aria-hidden />.

eps1lon added a commit to eps1lon/react that referenced this issue May 11, 2022
Following facebook#9230 (comment) except that `foo={true}` renders an empty string.
See facebook#9230 (comment) for rationale.
eps1lon added a commit to eps1lon/react that referenced this issue May 11, 2022
Following facebook#9230 (comment) except that `foo={true}` renders an empty string.
See facebook#9230 (comment) for rationale.
eps1lon added a commit to eps1lon/react that referenced this issue May 11, 2022
Following facebook#9230 (comment) except that `foo={true}` renders an empty string.
See facebook#9230 (comment) for rationale.
gaearon pushed a commit that referenced this issue May 20, 2022
* Log unexpected warnings when testing with ReactDOMServerIntegrationTestUtils

* Add test

Following #9230 (comment) except that `foo={true}` renders an empty string.
See #9230 (comment) for rationale.

* Match Preact behavior for boolean props on custom elements

* Poke CircleCI
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.