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

[css-shadow-parts] are rules for which pseudo-classes and pseudo-elements work after ::part() parse-time or match-time? #10786

Open
dbaron opened this issue Aug 27, 2024 · 4 comments

Comments

@dbaron
Copy link
Member

dbaron commented Aug 27, 2024

When I read the spec for ::part(), it describes rules about allowing pseudo-elements and pseudo-classes after ::part() in selectors. Those rules seem to say that all pseudo-classes and pseudo-elements are syntactically allowed, but that some of them never match.

I believe implementations across Chromium, Gecko, and WebKit have instead implemented these restrictions as parse-time restrictions that make the selectors not be syntactically valid. I know this is true in Chromium (I was working on the relevant code today), and I just added a test to WPT that appears to confirm that WebKit and Gecko agree (unless I'm misunderstanding things).

(I plan to open a separate issue, also related to the above test, about the fact that those tests depend on somewhat-vague wording that might be interpreted in different ways; see for example #9795 for previous confusion.)

How intentional was that wording in the spec? Should the spec change? Or is there sufficient justification to attempt to change multiple somewhat-interoperable engines here?

@dbaron dbaron changed the title are rules for which pseudo-classes and pseudo-elements work after ::part() parse-time or match-time? [css-shadow-parts] are rules for which pseudo-classes and pseudo-elements work after ::part() parse-time or match-time? Aug 27, 2024
@cdoublev
Copy link
Collaborator

I think parse-time validation is not ideal for backward compatibility, if there is a chance that a restriction will be removed later.

@tabatkins
Copy link
Member

I strongly prefer not putting more parse-time restrictions on Selectors than absolutely necessary, given the terrible behavior of Selectors wrt validity. So I'd really prefer that everything be allowed at parse time, and ones that aren't applicable just never match.

I can live with it going either way, tho.

@astearns astearns moved this to TPAC/FTF agenda items in CSSWG Agenda TPAC 2024 Sep 13, 2024
@dbaron
Copy link
Member Author

dbaron commented Sep 27, 2024

I think another argument for parse-time is that it's consistent with other restrictions on combinations of pseudo-elements. For example, this testcase is interoperably (Chrome, Firefox, Safari) red:

<!DOCTYPE html>
<style>
  body { color: red }
  body, body::before::before { color: green }
</style>
  Is this red or green?

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-shadow-parts] are rules for which pseudo-classes and pseudo-elements work after ::part() parse-time or match-time?, and agreed to the following:

  • RESOLVED: reject these invalid selectors at parse time
The full IRC log of that discussion <dholbert> dbaron: I filed a series of pseudo-element issues. this first one was a case where spec was written after impls/tests were written
<dholbert> dbaron: impls are interoperable and disagree with spec
<dholbert> dbaron: spec says some pseudo elements / classes work after ::part
<dholbert> dbaron: but all of them are supposed to be syntactically allowed at parse time, and the ones that don't work just don't match
<dholbert> dbaron: but impls reject at parse time the ones that don't work
<dholbert> dbaron: TabAtkins prefers what the spec says
<dholbert> dbaron: TabAtkins thinks parse-time selector restrictions haven't been very successful and we shouldn't add more of them
<TabAtkins> +1
<dholbert> dbaron: counterargument: it's already implemented across 3 engines, so changing impls might have some level of risk
<keithamus> q+
<dholbert> dbaron: Also, it's more consistent with other pseudo-elements
<florian> q+
<dholbert> dbaron: other pseudos that don't do anything are invalid at parse time
<emilio> q+
<dholbert> dbaron: [gives an example with two ::before elements]
<dholbert> dbaron: my inclination is to do easy thing and change spec to match the impls + tests
<lea> q+ to ask, it sounds like there's a clear upside to doing the validation at match time. What's the downside?
<dholbert> dbaron: this is widely tested in WPTs. We could just change the spec and say we've got it already implemented and tested
<kbabbitt> s/two ::before elements/::before::before selector, which is invalid/
<dholbert> dbaron: I'd like us to do that, or to hear a convincing argument in the other direction
<Rossen6> ack keithamus
<dholbert> ???: another benefit is that impls can highlight this in devtools
<lea> We can address that head-on by providing a method to test whether selectors are valid
<dbaron> s/???/keithamus/
<dholbert> keithamus: that's another argument for the parse-time restriction
<Rossen6> ack florian
<dholbert> florian: people who put tests in WPTs are welcome to change the spec & file spec bugs
<dholbert> florian: we should generally be more careful about this
<dholbert> dbaron: in this particular case, the tests were mostly testing other things. They were testing the rules for what wasn't allowed, and they didn't realize that there was stuff about it being a parse-time restriction
<dholbert> florian: there's a significant difference between things that are currently invalid are strongly expected to stay invalid forever, vs. if it may someday change
<dholbert> florian: if we treat a selector as a parse-time error and we want to make it valid in the future, that'll be painful
<dholbert> florian: But if we know these are going to be invalid forever, I have an easier time agreeing with your position
<Rossen6> ack emilio
<dholbert> emilio: I think parse-time is generally better; it works better with @supports
<dholbert> emilio: (and CSS.supports, etc)
<astearns> +1 to emilio and keithamus
<dholbert> emilio: I was going to also argue that impl-wise it's better not to have to deal with sort-of valid states, but we have to deal with that with nesting anyways
<dholbert> emilio: but yeah, @supports makes parse-time checking very valuable
<dholbert> emilio: if you want match-time restriction, you can use :is or :where
<dholbert> emilio: given we have a way of doing both things, I lean towards fixing the spec to match reality
<dholbert> florian: I'd be more ??? if we had selector invalidation at each ??? [agreeing I think]
<dholbert> dbaron: given the later issues I want to discuss, there might be a few strange cases of restrictions where we want to relax in the future
<dholbert> dbaron: but in most cases the restrictions are solid because it just doesn't make sense
<dholbert> lea: it sounds like there are upsides & downsides
<Rossen6> ack lea
<Zakim> lea, you wanted to ask, it sounds like there's a clear upside to doing the validation at match time. What's the downside?
<florian> s/I'd be more ???/I'd be more comfortable with the proposal
<dholbert> lea: sounds like arguments for parse-time checking are for testing if something's supported
<kbabbitt> q+
<dholbert> lea: is there a way we could add a way to check for if something is valid that would account for this case, and if we can, are there remaining arguments?
<florian> s/ if we had selector invalidation at each ???/ if we had selector error recovery at each comma, but especially given the availability of :is and :where, I think that's fine.
<Rossen6> ack kbabbitt
<dholbert> kbabbitt: does document.querySelector throw on an invalid selector, and that could be used to check for validity?
<dholbert> lea: only throws at parse time
<dholbert> lea: all our tools right now check for validity at parse-time
<dholbert> kbabbitt: you're looking for a way to check whether a selector that's valid at parse time is valid when used?
<dholbert> lea: sort of, yes
<dholbert> dbaron: we just listed 3 different ways for checking whether selectors are valid, and they're all the parse-time version
<dholbert> dbaron: it'd be confusing to add another way that checks at a different time
<dholbert> lea: who was arguing for making it invalid at match time?
<dholbert> [various]: Tab
<dholbert> dbaron: TabAtkins is OK either way but would prefer match time
<dholbert> Rossen6: we're leaning towards fixing the spec & going with parse time
<Rossen6> q?
<dholbert> Rossen6: objections?
<dholbert> RESOLVED: reject these invalid selectors at parse time
<dholbert> (fixing the spec to match reality)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Friday afternoon
Development

No branches or pull requests

5 participants