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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ShreckYe
Copy link
Contributor

@ShreckYe ShreckYe commented Dec 9, 2024

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.

@MatkovIvan MatkovIvan requested review from eymar and Schahen December 9, 2024 13:46
* @see AttrsScope.attr
*/
fun AttrsScope<*>.attr(attr: String, value: Boolean = true) =
attr(attr, value.toString())
Copy link
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
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
Copy link
Contributor Author

ShreckYe commented Dec 11, 2024

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
Copy link
Member

eymar commented Dec 17, 2024

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?

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

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