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.

Sign up for GitHub

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

Jump to bottom

Boolean attribute doesn't behave as might by expected for web component #162

Open
jrencz opened this issue Jan 11, 2024 · 4 comments · May be fixed by #164
Open

Boolean attribute doesn't behave as might by expected for web component #162

jrencz opened this issue Jan 11, 2024 · 4 comments · May be fixed by #164

Comments

@jrencz
Copy link

jrencz commented Jan 11, 2024

Hi!

I noticed that the chosen behaviour for boolean transform is

react-to-web-component/packages/core/src/transforms/boolean.ts

Lines 3 to 6 in 1a6886a

const string: Transform<boolean> = {
stringify: (value) => (value ? "true" : "false"),
parse: (value) => /^[ty1-9]/i.test(value),
}

which is:

it's true if value of attribute starts with t, y or a non-0 number.

and it's tested:

react-to-web-component/tests/react-to-webcomponent.test.jsx

Lines 352 to 353 in 002a007

true-prop="true"
false-prop="false"

On the other hand HTML boolean (https://developer.mozilla.org/en-US/docs/Glossary/Boolean/HTML) attribute is considered true just because it's present:

If an HTML tag contains a boolean attribute - no matter the value of that attribute - the attribute is set to true on that element. If an HTML tag does not contain the attribute, the attribute is set to false.

That leads to interesting behaviour - if attribute is present, but not assigned a value (i.e. it's HTML Boolean), it's cast to false (not just undefined but actually the polar opposite)

I detected it wile using with Lit booleans (https://lit.dev/docs/templates/expressions/#boolean-attribute-expressions) - Lit implements HTML boolean.

I think it can be resolved in a backwards compatible way.

I see that transforms are given the attribute name as 2nd argument

So parse could be made into

  parse: (value, attribute) => 
    // Check if it's HTML boolean
    value === undefined
    || value === ''
    || value === attribute
    /// Check if it's boolean by value
    || /^[ty1-9]/i.test(value),

I don't have an idea for stringify, though. It would have to be a choice of the author (i.e. differently named transform) I guess
@justinbmeyer
Copy link
Member

fwiw, I agree we should match the default behavior of the DOM ... attribute existence is true

jrencz added a commit to jrencz/react-to-web-component that referenced this issue Jan 12, 2024
@jrencz
HTML Boolean
e87434b 
Borrows some ideas from bitovi#123

Fixes bitovi#162
@jrencz jrencz linked a pull request Jan 12, 2024 that will close this issue
Open
@jrencz
Copy link
Author

jrencz commented Jan 12, 2024

-_-'

My bad - I didn't look through the open PRs: there's #123 addressing the same issue I raised, yet in a different way than I proposed (absence of value is checked not at the level of Transform['parse'] as I suggested, but before it's called)

Some of the ideas from #123 (namely - the access to attribute name in Transform['parse']) were introduced by #146, but besides it also has a proposed deprecation

Anyway: I created #164 that aims to resolve this issue

@christopherjbaker
Copy link
Contributor

I agree that it should match the DOM behaviour very closely, but we didn't realize this until after the 2.0 release and modifying it now would be a backwards-breaking change.

The most confusing parts to me of DOM behaviour is that checked="" and checked="false" are both treated as true. Those would be the two exceptions I might argue for, though I'm not sure how much we should break the behaviour without releasing 3.0.

@jrencz
Copy link
Author

jrencz commented Jan 19, 2024 •
edited

In #164 "false" is preserved a true (which is BTW not en exception - as per spec true is only either "", "checked" or lack of value in case you mention) but making "" a false contradicts the behavior compatible with Lit https://lit.dev/docs/templates/expressions/#boolean-attribute-expressions getting to which is my primary aim

Maybe then, for 2.x there could be a second dom_boolean which would allow the author to decide how the custom element behaves?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

HTML Boolean jrencz/react-to-web-component
3 participants
@justinbmeyer @jrencz @christopherjbaker