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

Remove hidden from booleanAttributes #4099

Merged
merged 3 commits into from
Mar 21, 2024

Conversation

IHIutch
Copy link
Contributor

@IHIutch IHIutch commented Mar 18, 2024

Fixes: #4086

This fix will allow you to pass strings to :hidden which will allow you to use hidden="until-found".

More information on hidden until found.

@SimoTod
Copy link
Collaborator

SimoTod commented Mar 18, 2024

The feature is experimental and not supported by some of the major browsers. It might be too early. https://caniuse.com/mdn-html_global_attributes_hidden_until-found_value

@IHIutch
Copy link
Contributor Author

IHIutch commented Mar 18, 2024

True, but this also isn't a breaking change. Boolean values will still work as expected

@SimoTod
Copy link
Collaborator

SimoTod commented Mar 18, 2024

I think it could be a breaking change. Not sure how it behaves with a falsy values. I believe Alpine currently removes the attribute.

@SimoTod
Copy link
Collaborator

SimoTod commented Mar 18, 2024

Maybe I'm wrong, with a quick codeen, it seems to work in the same way. You probably need to add some tests, though

@IHIutch
Copy link
Contributor Author

IHIutch commented Mar 18, 2024

Correct, it is removed when falsy because its not apart of attributeShouldntBePreservedIfFalsy

function attributeShouldntBePreservedIfFalsy(name) {
return ! ['aria-pressed', 'aria-checked', 'aria-expanded', 'aria-selected'].includes(name)
}

Happy to add some tests, if you like.

@SimoTod
Copy link
Collaborator

SimoTod commented Mar 18, 2024

It's more for the maintainer when he'll review the PR. It's more likely to be merged if there are tests showing that there are no breaking changes.

@ekwoka
Copy link
Contributor

ekwoka commented Mar 19, 2024

Definitely second including tests just to be more sure :)

@IHIutch
Copy link
Contributor Author

IHIutch commented Mar 19, 2024

Okay, I added to 2 existing tests. The first checks the hidden attribute specifically to ensure backward compatibility. Then I updated to the aria-pressed/checked/expanded/selected test, which seemed like the inverse concern, but only tested 1 of the 4 attributes in attributeShouldntBePreservedIfFalsy. (Which, thinking about now, is a weird name for attributes that are preserved 😉)

@calebporzio
Copy link
Collaborator

Thanks for adding tests.

I'm sure people rely on the existence of <div hidden> in their elements (toggled using Alpine).

Specifically for CSS like this:

div[hidden] {
    display: none;
}

So my biggest question is: does this PR break the above scenario?

First, to be crystal clear about that changes in this PR:

Before this PR change:

<div x-bind:hidden="true">
<!-- Renders to: -->
<div x-bind:hidden="true" hidden="hidden">

<div x-bind:hidden="false">
<!-- Renders to: -->
<div x-bind:hidden="false">

<div x-bind:hidden="'until-found'">
<!-- Renders to: -->
<div x-bind:hidden="'until-found'" hidden="hidden">

After this PR change:

<div x-bind:hidden="true">
<!-- Renders to: -->
<div x-data="" x-bind:hidden="true" hidden="true">

<div x-bind:hidden="false">
<!-- Renders to: -->
<div x-data="" x-bind:hidden="false">

<div x-bind:hidden="'until-found'">
<!-- Renders to: -->
<div x-bind:hidden="'until-found'" hidden="until-found">

After evaluating each, I think this is a safe change.

I'll leave this here for @SimoTod and @ekwoka to weigh in on before merging.

Thanks!

@ekwoka
Copy link
Contributor

ekwoka commented Mar 20, 2024

Is that the result?

It should still render hidden for true since true is not a canonical spec-compliant value.

It's same result, though, for now.

But it should all be safe, and highly unlikely to break anyone's code regardless.

It's unfortunate that in the DOM interfaces there is not a way to just programmatically identify these things.

It has to be hard coded...

@SimoTod
Copy link
Collaborator

SimoTod commented Mar 20, 2024

I think it's "safe enough". Minor thing but it's worth knowing it, 'hidden="true"' won't pass the W3C validator and I remember some people complaining about that for other "non-standard" things in the past.

@calebporzio
Copy link
Collaborator

Good points. I agree, it's "safe enough" @SimoTod. Interesting that hidden="true" won't pass a validator. So that's a bummer, but I suppose that's a further problem after this one (because we should at least make a change like this to allow custom values for "hidden").

I'm going to merge this.

@calebporzio calebporzio merged commit 137f8bb into alpinejs:main Mar 21, 2024
1 check passed
@IHIutch
Copy link
Contributor Author

IHIutch commented Mar 21, 2024

Thanks all!

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.

4 participants