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

[selectors-5] Add custom state pseudo-classes #8213

Merged
merged 12 commits into from
Aug 13, 2024
Merged

Conversation

josepharhar
Copy link
Contributor

We resolved to add the custom state pseudo-classes to selectors 5 here: #4805

We resolved to add the custom state pseudo-classes to selectors 5 here:
w3c#4805
@josepharhar
Copy link
Contributor Author

I'm not sure how to link to the definition which will be added to the HTML spec and I'm not sure how much detail we should add, I started with the bare minimum

Co-authored-by: Keith Cirkel <[email protected]>

The grammar of the '':state()'' pseudo-class is:

<pre class=prod>:state( <ident> )</pre>
Copy link
Member

Choose a reason for hiding this comment

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

Existing WPTs & impls make this case sensitive so:

Suggested change
<pre class=prod>:state( <ident> )</pre>
<pre class=prod>:state( <custom-ident> )</pre>

Copy link
Member

Choose a reason for hiding this comment

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

I suspect we don't want the exclusion of CSS-wide keywords that <custom-ident> implies; I think we have at least some amount of consensus that such exclusion was a bad idea in the first place and places that needed it should be using <dashed-ident> instead, and that <custom-ident> was a mistake. But on the other hand we should clearly document the case-sensitivity. I'm not sure what the current best practice is.

(I can't find any reference for this longer than #7431 (comment) but there might be a more detailed explanation somewhere.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so should we do <dashed-ident> for now...? or keep it as <ident>?

Copy link
Member

Choose a reason for hiding this comment

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

<dashed-ident> is definitely wrong as this can take any ident. I suppose <ident> is right from a strictly grammar standpoint because, as @dbaron points out, it doesn’t exclude css wide keywords. <ident> claims case insensitivity though. Perhaps we could make a new token type here like <cased-ident>. Otherwise we should keep it as <ident> and either explain (or remove) the implementation handles cases discretely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what is best here. @tabatkins can you advise?

Copy link
Member

Choose a reason for hiding this comment

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

I can’t find the reference to case insensitivity now.

The string given to customstateset can be dashed so should this be <ident> | <dashed-ident>?

Copy link
Member

Choose a reason for hiding this comment

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

No, <dashed-ident> is a subset of <ident>, so just <ident> is sufficient.

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 so since i'm currently using <ident> can we merge this?

Copy link
Member

Choose a reason for hiding this comment

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

I think you should add a sentence saying it's case-sensitive. (unless @tabatkins disagrees, at least.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I amended the first sentence to say that the argument is case sensitive

Co-authored-by: Keith Cirkel <[email protected]>
@dbaron dbaron self-requested a review March 18, 2024 17:31
Copy link
Member

@dbaron dbaron left a comment

Choose a reason for hiding this comment

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

This seems close to ready to merge, though I have a few comments below.

It's also not clear to me how useful this deferred-for-level-5 file is -- at some point we should have a real draft up on drafts.csswg.org.

(Did you make any attempt to run bikeshed on what you added here?)

selectors-4/deferred-for-level-5 Outdated Show resolved Hide resolved

The grammar of the '':state()'' pseudo-class is:

<pre class=prod>:state( <ident> )</pre>
Copy link
Member

Choose a reason for hiding this comment

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

I think you should add a sentence saying it's case-sensitive. (unless @tabatkins disagrees, at least.)

@astearns astearns removed the Agenda+ label Mar 26, 2024
@astearns
Copy link
Member

@keithamus I am unclear why the agenda+ tag was added here (and we often miss tagged PRs for the agenda). If there is a question that needs resolving could you open an issue?

@keithamus
Copy link
Member

I added it regarding #8213 (comment) but that has since been resolved. Apologies for the noise.

@josepharhar
Copy link
Contributor Author

It's also not clear to me how useful this deferred-for-level-5 file is -- at some point we should have a real draft up on drafts.csswg.org.

Good point, I moved it to overview.bs

(Did you make any attempt to run bikeshed on what you added here?)

I just did and it works

@dbaron
Copy link
Member

dbaron commented Jun 18, 2024

It's also not clear to me how useful this deferred-for-level-5 file is -- at some point we should have a real draft up on drafts.csswg.org.

Good point, I moved it to overview.bs

I don't think this is what we want either -- the resolution was to add to level 5, not to the (substantially more stable) level 4. We should probably get an actual selectors-5 draft going -- I'm not sure whether it should contain all or some or none of the material currently in the deferred-for-level-5 file.

(I admit that this makes getting the change landed more complicated!)

@josepharhar
Copy link
Contributor Author

I don't think this is what we want either -- the resolution was to add to level 5, not to the (substantially more stable) level 4.

Thanks, I put it back in the deferred for level 5 file

We should probably get an actual selectors-5 draft going -- I'm not sure whether it should contain all or some or none of the material currently in the deferred-for-level-5 file.

What is the process for this? Can I do it in a separate PR?

@dbaron
Copy link
Member

dbaron commented Jul 31, 2024

What is the process for this? Can I do it in a separate PR?

A separate PR would probably be good. I think it would make sense to move the deferred-for-level-5 file to be an actual level 5 draft. (Some of the material might need notes about its current state/status.)

@josepharhar
Copy link
Contributor Author

A separate PR would probably be good. I think it would make sense to move the deferred-for-level-5 file to be an actual level 5 draft. (Some of the material might need notes about its current state/status.)

Great! Is there anything left to do to merge this?

@dbaron dbaron merged commit 4ee0078 into w3c:main Aug 13, 2024
1 check passed
dbaron added a commit that referenced this pull request Aug 13, 2024
…evel-5 file.

As part of the resolution in #4805 and as a followup to #8213, this
moves the deferred-for-level-5 file from the selectors-4 directory into
a new selectors-5 delta spec.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Unsorted regular
Development

Successfully merging this pull request may close these issues.

5 participants