-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Conversation
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
There was a problem hiding this 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
@@ -71047,6 +71051,153 @@ dictionary <dfn dictionary>ValidityStateFlags</dfn> { | |||
|
|||
</div> | |||
|
|||
<h4>Custom state pseudo class</h4> |
There was a problem hiding this comment.
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").
There was a problem hiding this comment.
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?
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... |
There was a problem hiding this 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.
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 |
I have created a csswg selectors PR: w3c/csswg-drafts#8213 |
There was a problem hiding this 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.
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 |
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. |
I don't think that's true. Something needs to define that |
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 |
(I won't be able to attend the meeting anytime soon, but it would be good to get the above feedback addressed.) |
I tend to agree that the restriction to custom elements feels arbitrary. I don't have strong opinions on the syntax tho. |
So if I wanted to define a custom pseudo class "foo" are you saying it should be like |
Yes. That's more consistent with |
source
Outdated
<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> |
There was a problem hiding this comment.
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>
.
There was a problem hiding this comment.
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!
I'm curious about invalid states, and how they're approached. Right now Chrome's implementation checks that a string begins
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 |
But |
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
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 😅 |
FWIW |
The reason |
This is in Firefox 122 [tracking issue] behind the |
Thanks @keithamus !!! |
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) |
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.) |
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.
The spec was merged here, so the tests should no longer be tentative: whatwg/html#8467 Bug: 1514397 Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
The spec was merged here, so the tests should no longer be tentative: whatwg/html#8467 Bug: 1514397 Change-Id: I121e5e734145ff4cb122fb1bc2f7a5bc146dafca
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 <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1255129}
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 <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1255129}
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 <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1255129}
…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 <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1255129} -- wpt-commits: 9b7754b0e2d92e6a303376c24e8c6b20800de902 wpt-pr: 43804
…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 <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1255129} -- wpt-commits: 9b7754b0e2d92e6a303376c24e8c6b20800de902 wpt-pr: 43804
…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
…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
…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
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 <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1255129}
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 <masonf@chromium.org> Commit-Queue: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1255129}
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 )