Skip to content

Compose HTML: add an AttrsScope<*>.attr extension function with a Boolean parameter - #5178

Open
Yongshun Ye (ShreckYe) wants to merge 4 commits into
JetBrains:masterfrom
huanshankeji:attr-boolean
Open

Compose HTML: add an AttrsScope<*>.attr extension function with a Boolean parameter#5178
Yongshun Ye (ShreckYe) wants to merge 4 commits into
JetBrains:masterfrom
huanshankeji:attr-boolean

Conversation

@ShreckYe

Copy link
Copy Markdown
Contributor

The AttrsScope.attr member function supports bolean attributes via "For boolean attributes cast boolean value to String and pass it as value.". This feature turns out to be frequently used in this compose-html-material project I have been working on. So I'd like to propose to add this to the core library.

* @see AttrsScope.attr
*/
fun AttrsScope<*>.attr(attr: String, value: Boolean = true) =
attr(attr, value.toString())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason making it an extension function?
What do you think about adding this function to the AttrsScope itself?

Would you like to add a test for this new function in AttributesTests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for replying. Adding it as a member function looks good to me too. And sure I'd love to add some tests for this.

@ShreckYe

Yongshun Ye (ShreckYe) commented Dec 11, 2024

Copy link
Copy Markdown
Contributor Author

Hi, I have finished the requested changes. However, I have noticed something else that the required attribute extension function as well as some others don't stick to the documented way of passing boolean attributes, but pass an empty string to indicate the attribute is true instead:

@Deprecated(
message = "Please use `required()` without parameters. Use if..else.. if conditional behaviour required.",
replaceWith = ReplaceWith("required()", "org.jetbrains.compose.web.attributes.required"),
level = DeprecationLevel.WARNING
)
fun AttrsScope<HTMLInputElement>.required(value: Boolean = true) =
attr("required", value.toString())
fun AttrsScope<HTMLInputElement>.required() =
attr("required", "")

Is this the new recommended way that's not documented yet? Shall we update the KDocs and this added boolean attr function to not take an optional value parameter and pass an empty string instead then?

@eymar

Oleksandr Karpovich (eymar) commented Dec 17, 2024

Copy link
Copy Markdown
Member

Hi, I have finished the requested changes. However, I have noticed something else that the required attribute extension function as well as some others don't stick to the documented way of passing boolean attributes, but pass an empty string to indicate the attribute is true instead:

@Deprecated(
message = "Please use `required()` without parameters. Use if..else.. if conditional behaviour required.",
replaceWith = ReplaceWith("required()", "org.jetbrains.compose.web.attributes.required"),
level = DeprecationLevel.WARNING
)
fun AttrsScope<HTMLInputElement>.required(value: Boolean = true) =
attr("required", value.toString())
fun AttrsScope<HTMLInputElement>.required() =
attr("required", "")

Is this the new recommended way that's not documented yet? Shall we update the KDocs and this added boolean attr function to not take an optional value parameter and pass an empty string instead then?

Yongshun Ye (@ShreckYe) , Good question!
I didn't pay attention to that, but now I recall why it's made that way: https://developer.mozilla.org/en-US/docs/Glossary/Boolean/HTML

Note: The strings "true" and "false" are invalid values. To set the attribute to false, the attribute should be omitted altogether. Though modern browsers treat any string value as true, you should not rely on that behavior.

So passing "false" doesn't really make it "false". And the current implementation you made won't be correct.
According to documentation, if we want the attribute value to be false we need to ommit the attribute itself. I guess adding an extra check in your new function would make it correct. Something like: if (value) attr(attr, "").


Also here: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes#boolean_attributes

HTML defines restrictions on the allowed values of boolean attributes: If the attribute is present, its value must either be the empty string (equivalently, the attribute may have an unassigned value), or a value that is an ASCII case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace.
...
To be clear, the values "true" and "false" are not allowed on boolean attributes. To represent a false value, the attribute has to be omitted altogether.

@ShreckYe

Copy link
Copy Markdown
Contributor Author

Thanks for replying and the references you provide. It explains everything.

I guess adding an extra check in your new function would make it correct. Something like: if (value) attr(attr, "").

For this, what I am thinking is just removing the value parameter in this function just like required does. For users, they can add a boolean variable themselves when needed and Compose will react to the changes. This makes things simpler. What do you think of this?

@eymar

Oleksandr Karpovich (eymar) commented Jul 27, 2026

Copy link
Copy Markdown
Member

For this, what I am thinking is just removing the value parameter in this function just like required does. For users, they can add a boolean variable themselves when needed and Compose will react to the changes. This makes things simpler. What do you think of this?

It's already possible to achieve this by if (flag) attr("readonly", "")

But there is a problem in the KDoc - it contradicts the official doc about boolean attributes - https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attributes:

For boolean attributes cast boolean value to String and pass it as value.

I think that adding an attrs overload without a value could bring some risks where IDE auto-suggest a simpler method when someone expects a method with a value.

WDYT about this:

/**
 * Conditionally sets a boolean HTML attribute based on a condition.
 */
fun <TElement : Element> AttrsScope<TElement>.attr(
    name: String, 
    condition: Boolean
): AttrsScope<TElement> {
    if (condition) {
        attr(name, "")
    }
    return this
}

P.S. I apologise for no replies here.

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