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

Add custom state pseudo class #8467

Merged
merged 13 commits into from
Dec 24, 2023
Merged

Add custom state pseudo class #8467

merged 13 commits into from
Dec 24, 2023

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Nov 2, 2022

This is based on the WICG draft spec here: https://wicg.github.io/custom-state-pseudo-class

Here are some spec issues where this feature has been discussed:
w3ctag/design-reviews#428
w3c/csswg-drafts#4805

Corresponding CSS spec PR: w3c/csswg-drafts#8213

(See WHATWG Working Mode: Changes for more details.)


/custom-elements.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/semantics-other.html ( diff )

This is based on the WICG draft spec here:
https://wicg.github.io/custom-state-pseudo-class

Here are some spec issues where this feature has been discussed:
w3ctag/design-reviews#428
w3c/csswg-drafts#4805
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Nice work!

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
@@ -71047,6 +71051,153 @@ dictionary <dfn dictionary>ValidityStateFlags</dfn> {

</div>

<h4>Custom state pseudo class</h4>
Copy link
Member

Choose a reason for hiding this comment

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

To follow existing patterns in the custom elements section, I think we should split this up.

The introduction should go as a new section under https://whatpr.org/html/8467/custom-elements.html#custom-elements-intro . (Maybe "Exposing a custom element's states") And the normative stuff should go as a new section under https://whatpr.org/html/8467/custom-elements.html#element-internals ("Custom pseudo-classes").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I tried moving it around. Did i get it right?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@josepharhar
Copy link
Contributor Author

Weird, the conformance checker is showing errors in the build here but when I run it locally via vnu-jar it doesn't produce any errors...

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Looking good!

Weird, the conformance checker is showing errors in the build here but when I run it locally via vnu-jar it doesn't produce any errors...

This is due to whatwg/whatwg.org#401 . Not your fault, don't worry about it, and we'll fix ASAP.

If you're curious, you can probably get the errors to reproduce locally if you update your validator download to the latest release.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Nov 15, 2022

Where is the corresponding Selectors PR? And this should probably also reference Selectors for the pseudo-classes?

@josepharhar
Copy link
Contributor Author

Where is the corresponding Selectors PR? And this should probably also reference Selectors for the pseudo-classes?

There is no PR yet. I'll try to make one next week unless @tabatkins is interested in making one

@josepharhar
Copy link
Contributor Author

I have created a csswg selectors PR: w3c/csswg-drafts#8213

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This LGTM. However note I pushed one fix which is pretty important: I changed the return value of add() to be a DOMString, to match the prose which returns the added value, and also match the requirements for all setlikes.

I can't find any test for the return value of add() in https://wpt.fyi/results/custom-elements/state/tentative/ElementInternals-states.html?label=experimental&label=master&aligned , so please work on adding that (and fixing Chromium, if necessary).

I'm not sure how important it is to land something in selectors, so I'd be happy merging this ASAP. But I guess we haven't fully confirmed second-implementer interest, and @annevk seems concerned about the selectors PR, so I think his sign-off is what is most needed at this point.

@annevk
Copy link
Member

annevk commented Jan 11, 2023

It would be good if @tabatkins or @fantasai could have a look. We've had a multitude of issues due to Selectors including something that got then implemented without HTML saying anything about when it matches. I'd rather not add to that.

I also think that the mismatch with ::part() is rather unfortunate as this is supposed to be the psuedo-class counterpart to that. See WebKit/standards-positions#56 for background on that.

@domenic
Copy link
Member

domenic commented Jan 12, 2023

We've had a multitude of issues due to Selectors including something that got then implemented without HTML saying anything about when it matches. I'd rather not add to that.

Yeah, but this PR defines the full matching algorithm in HTML. I'm not sure the selectors definitions really add much, given that we have that... they're kind of interesting as a theoretical summary of how a selector might behave in a non-browser user agent, but I think they're less important.

@annevk
Copy link
Member

annevk commented Jan 12, 2023

I don't think that's true. Something needs to define that :--blah is not a syntax error. Currently :root, :--blah { background:lime } is thrown out.

@rniwa
Copy link

rniwa commented Jan 13, 2023

This feature will only work with custom elements. That seems different from "customness" of CSS variables / properties. Custom CSS variables and properties aren't things that custom elements define but rather what a page author uses across multiple (custom or not) elements to stylize their web page. To mirror this, custom pseudo class should be something author can define on any element independent of custom elements for the use in their own web pages. For example, maybe we want :--happpy pseudo class to indicate happy images of cats. No reason why img or picture element needs to be wrapped in a custom element just to support this custom pseudo state. Yet we also want a state API for custom elements with good encapsulation as well. I believe :state is the right syntax for such a feature for custom elements as it nicely mirrors ::part. :--state should be reserved for future addition of more generic version of custom pseudo class (e.g. Element.prototype.customPseudo) that can be defined outside of web components.

@annevk annevk added the agenda+ To be discussed at a triage meeting label Jul 15, 2023
@annevk
Copy link
Member

annevk commented Jul 15, 2023

(I won't be able to attend the meeting anytime soon, but it would be good to get the above feedback addressed.)

@emilio
Copy link
Contributor

emilio commented Jul 24, 2023

I tend to agree that the restriction to custom elements feels arbitrary. I don't have strong opinions on the syntax tho.

@josepharhar
Copy link
Contributor Author

I believe :state is the right syntax for such a feature for custom elements as it nicely mirrors ::part. :--state should be reserved for future addition of more generic version of custom pseudo class (e.g. Element.prototype.customPseudo) that can be defined outside of web components.

So if I wanted to define a custom pseudo class "foo" are you saying it should be like :state(foo)? And that we should just rename :--foo to :state(foo) and continue with this feature?

@rniwa
Copy link

rniwa commented Jul 25, 2023

I believe :state is the right syntax for such a feature for custom elements as it nicely mirrors ::part. :--state should be reserved for future addition of more generic version of custom pseudo class (e.g. Element.prototype.customPseudo) that can be defined outside of web components.

So if I wanted to define a custom pseudo class "foo" are you saying it should be like :state(foo)? And that we should just rename :--foo to :state(foo) and continue with this feature?

Yes. That's more consistent with :part feature for web components.

source Outdated
Comment on lines 73526 to 73532
<dt><dfn selector noexport data-x="selector-custom">Custom state pseudo-class</dfn></dt>
<dd>
<p>The <code data-x="selector-custom">:state()</code> pseudo-class takes an argument and must
match all <span>custom element</span>s whose <span>states set</span>'s <span>set entries</span>
contains a string matching the argument passed to <code
data-x="selector-custom">:state()</code>.</p>
</dd>
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the drive-by review, but is this the right formatting for a :state(arg) pseudo-class? I see why it happened while the pseudo-class was spelled :--arg, but once it's formatted as a function call like :dir(ltr), I'd expect something like <dfn selector noexport><code data-x="selector-state">:state(<var>identifier</var>)</code></dfn></dt>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is much easier to read, so I did it. Thanks!

@keithamus
Copy link
Contributor

keithamus commented Oct 28, 2023

I'm curious about invalid states, and how they're approached. Right now Chrome's implementation checks that a string begins -- which I imagine will be relaxed. The existing WPTs (via idl tests) also check for a missing argument. But should there be additional validation of the given string? No validation allows for stuff like:

.states.set('@foo')
 // `:state(@foo)` is an invalid selector (at-keyword)
 // `:state(\@foo)` will match.

.states.set('1rem')
 // `:state(1rem)` is an invalid selector (dimension)
 // `:state(\1rem)` won't match
 // `:state(\31rem)` will match (a unicode escape sequence).

.states.set('.foo')
 // `:state(.foo)` is an invalid selector (delim + ident)
 // `:state(\.foo)` will match.

Maybe these are fine? I imagine some will be a little surprising to users. Having both JS and CSS silently fail could make this difficult to debug.

I also think no validation creates a discrepancy with .matches() given the above. so .states.set('@foo') won't throw but .matches(':state(@foo)') will.

@annevk
Copy link
Member

annevk commented Oct 28, 2023

But matches(':state(\@foo)') won't, right? This seems fine and on par with how IDs and class names work.

@josepharhar
Copy link
Contributor Author

Yeah I'm not sure if it would be easy or not for us to sort of run css parsing on the string passed in as if it were used inside :state() to find out if it would match or not.

Having both JS and CSS silently fail could make this difficult to debug.

imo, the silent failing of CSS selector parsing is always going to be hard. We had a bug in chrome recently because a CSS selector in the user agent stylesheet was invalid and nobody noticed 😅

@keithamus
Copy link
Contributor

keithamus commented Oct 31, 2023

FWIW classList, being a DOMTokenList, checks for empty strings and spaces. Right now Chrome's implementation of CustomStateSet (which uses dashed idents and not :state()) expects a <dashed-ident> production and checks each character is a valid name code point: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/custom/custom_state_set.cc;l=63-72;drc=abbfc06e9b070c9ea64af2b233624646cef00252?q=customstateset&ss=chromium%2Fchromium%2Fsrc.

@annevk
Copy link
Member

annevk commented Oct 31, 2023

The reason classList does that is because it's a space-separated list. If it didn't check that you'd run into serialization issues. The empty string check I'm less sure about. getElementById() doesn't throw for an empty string, it just results in a non-match. Following that seems better.

@keithamus
Copy link
Contributor

keithamus commented Dec 6, 2023

This is in Firefox 122 [tracking issue] behind the dom.element.customstateset.enabled flag, and in WebKit's main branch, under the CustomStateSetEnabled flag. Both use the :state() syntax. There's also fairly good coverage in WPT including some robust parser tests, and api tests.

@josepharhar
Copy link
Contributor Author

Thanks @keithamus !!!
Is there anything else I can do to get this merged? I can file an MDN issue if we are confident that this will get shipped as specced in this PR.

@lukewarlow
Copy link
Member

WebKit now has this shipped by default on the main branch. Just as an fyi. Firefox appears to need some style invalidation work doing but is otherwise following this new spec (from what I can tell)

@annevk annevk merged commit 9e72bf9 into whatwg:main Dec 24, 2023
2 checks passed
@annevk
Copy link
Member

annevk commented Dec 25, 2023

Thanks @josepharhar for making this feature happen and thanks @keithamus for providing the implementation needed to get this over the finish line!

(Just noticed I forgot to leave this comment when I merged.)

tidoust added a commit to w3c/browser-specs that referenced this pull request Dec 26, 2023
The WICG spec moved to actual standardization with two parts:

1. The IDL part was moved to HTML in whatwg/html#8467
2. The CSS part is to be integrated in Selectors, see w3c/csswg-drafts#4805

This flags the spec as obsoleted by these two specs, using `selectors-4` for
the CSS part. The CSS WG resolution mentions Selectors 5 but it does not exist
yet.
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 31, 2024
The spec was merged here, so the tests should no longer be tentative:
whatwg/html#8467

Bug: 1514397
Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 1, 2024
The spec was merged here, so the tests should no longer be tentative:
whatwg/html#8467

Bug: 1514397
Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
aarongable pushed a commit to chromium/chromium that referenced this pull request Feb 1, 2024
The spec was merged here, so the tests should no longer be tentative:
whatwg/html#8467

Bug: 1514397
Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5151685
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1255129}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 1, 2024
The spec was merged here, so the tests should no longer be tentative:
whatwg/html#8467

Bug: 1514397
Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5151685
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1255129}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 1, 2024
The spec was merged here, so the tests should no longer be tentative:
whatwg/html#8467

Bug: 1514397
Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5151685
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1255129}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 5, 2024
…tate() WPTs, a=testonly

Automatic update from web-platform-tests
Consolidate and remove tentative from :state() WPTs

The spec was merged here, so the tests should no longer be tentative:
whatwg/html#8467

Bug: 1514397
Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5151685
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1255129}

--

wpt-commits: 9b7754b0e2d92e6a303376c24e8c6b20800de902
wpt-pr: 43804
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Feb 5, 2024
…tate() WPTs, a=testonly

Automatic update from web-platform-tests
Consolidate and remove tentative from :state() WPTs

The spec was merged here, so the tests should no longer be tentative:
whatwg/html#8467

Bug: 1514397
Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5151685
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1255129}

--

wpt-commits: 9b7754b0e2d92e6a303376c24e8c6b20800de902
wpt-pr: 43804
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Feb 8, 2024
…tate() WPTs, a=testonly

Automatic update from web-platform-tests
Consolidate and remove tentative from :state() WPTs

The spec was merged here, so the tests should no longer be tentative:
whatwg/html#8467

Bug: 1514397
Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5151685
Reviewed-by: Mason Freed <masonfchromium.org>
Commit-Queue: Joey Arhar <jarharchromium.org>
Cr-Commit-Position: refs/heads/main{#1255129}

--

wpt-commits: 9b7754b0e2d92e6a303376c24e8c6b20800de902
wpt-pr: 43804

UltraBlame original commit: 07970253da92c06ee91b5aee1424d11463cf9040
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Feb 8, 2024
…tate() WPTs, a=testonly

Automatic update from web-platform-tests
Consolidate and remove tentative from :state() WPTs

The spec was merged here, so the tests should no longer be tentative:
whatwg/html#8467

Bug: 1514397
Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5151685
Reviewed-by: Mason Freed <masonfchromium.org>
Commit-Queue: Joey Arhar <jarharchromium.org>
Cr-Commit-Position: refs/heads/main{#1255129}

--

wpt-commits: 9b7754b0e2d92e6a303376c24e8c6b20800de902
wpt-pr: 43804

UltraBlame original commit: 07970253da92c06ee91b5aee1424d11463cf9040
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Feb 8, 2024
…tate() WPTs, a=testonly

Automatic update from web-platform-tests
Consolidate and remove tentative from :state() WPTs

The spec was merged here, so the tests should no longer be tentative:
whatwg/html#8467

Bug: 1514397
Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5151685
Reviewed-by: Mason Freed <masonfchromium.org>
Commit-Queue: Joey Arhar <jarharchromium.org>
Cr-Commit-Position: refs/heads/main{#1255129}

--

wpt-commits: 9b7754b0e2d92e6a303376c24e8c6b20800de902
wpt-pr: 43804

UltraBlame original commit: 07970253da92c06ee91b5aee1424d11463cf9040
mbrodesser-Igalia pushed a commit to mbrodesser-Igalia/wpt that referenced this pull request Feb 19, 2024
The spec was merged here, so the tests should no longer be tentative:
whatwg/html#8467

Bug: 1514397
Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5151685
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1255129}
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 23, 2024
The spec was merged here, so the tests should no longer be tentative:
whatwg/html#8467

Bug: 1514397
Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5151685
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1255129}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.