compose-multiplatform icon indicating copy to clipboard operation
compose-multiplatform copied to clipboard

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

Open ShreckYe opened this issue 1 year ago • 3 comments

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.

ShreckYe avatar Dec 09 '24 13:12 ShreckYe

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:

https://github.com/JetBrains/compose-multiplatform/blob/e9597e71931dc38de8171605ce701efe8a6f4d8c/html/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/Attrs.kt#L197-L206

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?

ShreckYe avatar Dec 11 '24 17:12 ShreckYe

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:

https://github.com/JetBrains/compose-multiplatform/blob/e9597e71931dc38de8171605ce701efe8a6f4d8c/html/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/Attrs.kt#L197-L206

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?

@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.

eymar avatar Dec 17 '24 09:12 eymar

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?

ShreckYe avatar Dec 17 '24 12:12 ShreckYe