[selectors] selector list with invalid selector #14442
Conversation
If a selector is invalid, the entire selector list is invalid. https://drafts.csswg.org/selectors-4/#grouping
wpt-pr-bot
requested review from
fantasai,
frivoal,
plinss,
tabatkins and
tantek
| color: red; | ||
| } | ||
|
|
||
| .c:is(*, :nonsense) { |
There was a problem hiding this comment.
Based on w3c/csswg-drafts#3264 (comment), this one should actually match.
| .c:not(*, :nonsense) { | ||
| color: red; | ||
| } | ||
| .c:where(*, :nonsense) { |
There was a problem hiding this comment.
As for :is, based on w3c/csswg-drafts#3264 (comment), this one should match.
| .c:is(*, :nonsense) { | ||
| color: red; | ||
| } | ||
| .c:not(*, :nonsense) { |
There was a problem hiding this comment.
There's remaining discussion in w3c/csswg-drafts#3264 (comment) about what should happen to :not() with invalid arguments. As currently resolved, it should match (so the test would be wrong), but maybe we'll change that, so I would wait for the dust to settle before landing this PR, as this may or may not need fixing.
| } | ||
|
|
||
| /* Pseudo-elements cannot be represented by the matches-any pseudo-class; they are not valid within :is(). */ | ||
| .b, :is(::before) { |
There was a problem hiding this comment.
As the edits for w3c/csswg-drafts#3264 (comment) haven't been done yet and the discussion is still ongoing, it's not entirely clear what should happen to :is() with a single invalid argument. I would expect that the new invalidation rules would mean that since ::before is not allowed in :is() it gets discarded, but that this merely cause that argument of :is() (in this case its only argument) to be discarded, and the :is() construct as a whole remains valid, even if it matches nothing. So the whole rule is not treated as a syntax error, .b matches, and the element turns red. Which makes the test wrong.
| .b, :not(::before) { | ||
| color: red; | ||
| } | ||
| .b, :where(::before) { |
There was a problem hiding this comment.
| .b, :is(::before) { | ||
| color: red; | ||
| } | ||
| .b, :not(::before) { |
There was a problem hiding this comment.
Sort of the same comment as https://github.com/web-platform-tests/wpt/pull/14442/files#r240427239, except that there's still ongoing discussion in that issue about how invalidation of :not() arguments should work. We should wait until the dust settles on that.
There was a problem hiding this comment.
There's no inherent dependency on Javascript, so I'd prefer if this was a (series of?) reftests, but that's just a preference, not a show stopper.
Other than that, it seems to me this test is largely incorrect, due to w3c/csswg-drafts#3264 (comment). See individual comments.
|
This PR is blocked on the required "Travis CI - Pull Request" check after #14499. In order to trigger it, I will close and reopen this PR. |
@frivoal, I just came here to clean up my Travis mess, but am curious about this. Is it not preferable to use CSSOM and things like |
|
I was planning to update this test as part of the Blink CL implementing error recovery when the spec updates. I can instead implement that now (without merging into Blink). I prefer having a dozen asserts to having a dozen reftests or a single reftest with a dozen diffs.
|
|
@ewilligers that makes sense to me. |
@foolip Answering this question way late, but given that there exists CSS implementations that do not support JS, it seems better when features that could either be tested in a reftest or in a testharness test are tested in a reftest, without depending on JS. For browsers it doesn't make a difference, but for other implementations it does, and the more common tests we have, the better the interop and the lower the risk of an accidental fork in the platform. |
@ewilligers Any plan on fixing this (or telling me why I'm wrong), or should we close this? |
If a selector is invalid, the entire selector list is invalid.
https://drafts.csswg.org/selectors-4/#grouping