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] consider preserving invalid selectors in :is() and :where() #8356

Open
emilio opened this issue Jan 25, 2023 · 11 comments
Open

[selectors] consider preserving invalid selectors in :is() and :where() #8356

emilio opened this issue Jan 25, 2023 · 11 comments
Labels

Comments

@emilio
Copy link
Collaborator

emilio commented Jan 25, 2023

In #7972 we resolved to preserve invalid selectors that contain an & as-is. It seems it'd be more consistent to do that for all invalid :is() / :where() branches like :is(:unknown). That's how @media and @supports behave.

@Loirooriol
Copy link
Contributor

If invalid selectors are no longer dropped, :is() without arguments does no longer seem needed.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 13, 2023
After the pending test fixes (bug 1843349) we only fail tests due to two
reasons:

 * :has() being unsupported. Not related to the feature itself.

 * The nesting selector being inside another (invalid) forgiving
   selector, e.g. :is(!&) not being preserved.

The later is pretty much in edge-case-land, and I think
w3c/csswg-drafts#8356 would be more consistent
rather than doing it only when an & is found (which is very very weird).

So I wouldn't want to block on that. Let's let it ride the trains.

Differential Revision: https://phabricator.services.mozilla.com/D183519
@tabatkins
Copy link
Member

Yeah I think I'm fine with this. It certainly makes things more consistent, which is nice.

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Jul 16, 2023
After the pending test fixes (bug 1843349) we only fail tests due to two
reasons:

 * :has() being unsupported. Not related to the feature itself.

 * The nesting selector being inside another (invalid) forgiving
   selector, e.g. :is(!&) not being preserved.

The later is pretty much in edge-case-land, and I think
w3c/csswg-drafts#8356 would be more consistent
rather than doing it only when an & is found (which is very very weird).

So I wouldn't want to block on that. Let's let it ride the trains.

Differential Revision: https://phabricator.services.mozilla.com/D183519

UltraBlame original commit: 88d3fbc7589e6b57adc0946fe197ada285ac84b7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Jul 16, 2023
After the pending test fixes (bug 1843349) we only fail tests due to two
reasons:

 * :has() being unsupported. Not related to the feature itself.

 * The nesting selector being inside another (invalid) forgiving
   selector, e.g. :is(!&) not being preserved.

The later is pretty much in edge-case-land, and I think
w3c/csswg-drafts#8356 would be more consistent
rather than doing it only when an & is found (which is very very weird).

So I wouldn't want to block on that. Let's let it ride the trains.

Differential Revision: https://phabricator.services.mozilla.com/D183519

UltraBlame original commit: 88d3fbc7589e6b57adc0946fe197ada285ac84b7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jul 16, 2023
After the pending test fixes (bug 1843349) we only fail tests due to two
reasons:

 * :has() being unsupported. Not related to the feature itself.

 * The nesting selector being inside another (invalid) forgiving
   selector, e.g. :is(!&) not being preserved.

The later is pretty much in edge-case-land, and I think
w3c/csswg-drafts#8356 would be more consistent
rather than doing it only when an & is found (which is very very weird).

So I wouldn't want to block on that. Let's let it ride the trains.

Differential Revision: https://phabricator.services.mozilla.com/D183519

UltraBlame original commit: 88d3fbc7589e6b57adc0946fe197ada285ac84b7
@cdoublev
Copy link
Collaborator

cdoublev commented Jul 19, 2023

Preserving invalid/unknown <media-query> instead of replacing them with not all would also makes things more consistent.

edit: nevermind, this has already been resolved.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [selectors] consider preserving invalid selectors in :is() and :where(), and agreed to the following:

  • RESOLVED: preserve invalid selectors inside forgiving selector lists (such as :is() and :where()) regardless of whether there's an & in them
The full IRC log of that discussion <dbaron> emilio: For nesting, when you have an & in a nested selector, we resolved to preserve the invalid selector inside so that you could properly track whether there was an &. Makes sense. But inconsistent, in the sense that it's a weird special case. Would be more consistent IMO to preserve all invalid selectors. That way you don't special case nesting. It's also more consistent with @supports and @media, with <general-enclosed>.
<dbaron> emilio: ... It feels more generally consistent. I don't think it matters particularly... I don't expect compat issues. The question is... as Oriol mentioned, this also makes the empty selector kind of silly.
<dbaron> TabAtkins: As a selectors editor, I agree, would be good to keep it ??? defined.
<dbaron> TabAtkins: not sure why we decided to drop the invalid selector arguments in :is()
<dbaron> emilio: implementation-wise, it's easier to just drop it, but if we need to preserve it, easier to preserve always rather than sometimes.
<ntim_> q+
<dbaron> emilio: So if nobody objects I think this would be better than the current specified behavior.
<bramus> +1
<Rossen_> ack ntim_
<dbaron> ntim_: Isn't there something about forgiving parsing?
<dbaron> emilio: That's the behavior of dropping invalid selector.
<dbaron> TabAtkins: nesting cares about whether there's an & at all, so you don't have a newer/older browser behavior switch.
<bramus> dbaron: so you would still be treating invalid selectors the same
<dbaron> dbaron: ... just preserving them in the OM.
<bramus> … but just not drop m
<dbaron> TabAtkins: yes
<dbaron> emilio: Proposed resolution: preserve invalid selectors inside :is() and :where() regardless of whether there's an & token.
<dbaron> ?: do for all forgiving selector lists?
<astearns> s/?/matthieudubet
<dbaron> TabAtkins: I think that's all of them for now, but we should say for all forgiving selector lists.
<dbaron> RESOLVED: preserve invalid selectors inside forgiving selector lists (such as :is() and :where()) regardless of whether there's an & in them

@cdoublev
Copy link
Collaborator

cdoublev commented Jul 26, 2023

If invalid selectors are no longer dropped, :is() without arguments does no longer seem needed.

:is(), valid {} would become invalid, isn't it? Is it ok regarding back-compatibility?


TabAtkins: not sure why we decided to drop the invalid selector arguments in :is()

I think a good reason is that the author can see which selectors are invalid.


emilio: implementation-wise, it's easier to just drop it, but if we need to preserve it, easier to preserve always rather than sometimes

Isn't it also easier to serialize all selectors as is?

You parse :is(<declaration-value>#) and then you evaluate each <declaration-value> with parse a comma-separated list according to a CSS grammar (<complex-selector>). This could be applied to <media-query-list>.

edit: nope, sorry, <declaration-value> matches ,.

@emilio
Copy link
Collaborator Author

emilio commented Jul 26, 2023

If invalid selectors are no longer dropped, :is() without arguments does no longer seem needed.

:is(), valid {} would become invalid, isn't it? Is it ok regarding back-compatibility?

Probably, since the only reasonable use-case for :is() is "all the other selectors were dropped". But we could keep :is() working, if needed for compat.

Isn't it also easier to serialize all selectors as is?

Not really, because then you need to preserve the specified string. Also it would be a pretty big behavior change (e.g. .foo /* whatever */ .bar would serialize as-is rather than with whitespace normalized and comments removed.

@cdoublev
Copy link
Collaborator

To be clear, I was suggesting to serialize the specified string of all selectors only in :is()/:where().

On second thought, I also think it is easier to only serialize invalid selectors as-is, which I interpret as with the specified string, rather than the component values.

Is it possible to serialize :is(##invalid, valid) without a whitespace between # and #invalid, without the specified string?

@tabatkins
Copy link
Member

Is it possible to serialize :is(##invalid, valid) without a whitespace between # and #invalid, without the specified string?

Yeah, assuming you're reserializing from the token stream, that's the behavior you'd get by default. There's no whitespace token there, and the two tokens don't need a separator to re-parse successfully.

@cdoublev
Copy link
Collaborator

cdoublev commented Aug 9, 2023

To parse as a forgiving selector list given an input input:

  1. Parse a list of <complex-real-selector>s from input, and let selector list be the result.
  2. Remove all failure items from selector list, and all items that are invalid selectors, then return a <selector-list> representing the remaining items in selector list. (This might be empty.)

Does preserving invalid selectors mean that step 2 will be removed and that items in selector list represent both the input and the result from its parsing, or that <forgiving-selector-list> will no longer be parsed with parse a comma-separated list according to a CSS grammar?

webkit-commit-queue pushed a commit to mdubet/WebKit that referenced this issue Aug 10, 2023
https://bugs.webkit.org/show_bug.cgi?id=258688
rdar://111861948

Reviewed by Antti Koivisto and Darin Adler.

For the serialization round-trip to work with the presence of invalid
selectors containing a nesting selector, the spec mandates that
invalid selectors inside forgiving selector list (such as :is(..) or :where(...))
should be kept in serialization (and be considered when deciding whether a selector
is "nest-containing" or not).

w3c/csswg-drafts#8356 (comment)

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/css/css-nesting/cssom-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving-expected.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving-ref.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-scoping/slotted-parsing.html:
* LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-where-error-recovery.html:
* LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-where-parsing.html:
* LayoutTests/imported/w3c/web-platform-tests/css/selectors/parsing/parse-has-disallow-nesting-has-inside-has.html:
* LayoutTests/imported/w3c/web-platform-tests/css/selectors/parsing/parse-has.html:
* Source/WebCore/css/CSSSelector.cpp:
(WebCore::simpleSelectorSpecificity):
(WebCore::CSSSelector::selectorText const):
(WebCore::CSSSelector::hasExplicitNestingParent const):
* Source/WebCore/css/CSSSelector.h:
* Source/WebCore/css/SelectorChecker.cpp:
(WebCore::canMatchHoverOrActiveInQuirksMode):
(WebCore::SelectorChecker::checkOne const):
* Source/WebCore/css/parser/CSSSelectorParser.cpp:
(WebCore::CSSSelectorParser::consumeForgivingSelectorList):
* Source/WebCore/css/parser/CSSSelectorParser.h:
* Source/WebCore/cssjit/SelectorCompiler.cpp:
(WebCore::SelectorCompiler::constructFragmentsInternal):
* Source/WebCore/style/RuleSet.cpp:
(WebCore::Style::RuleSet::addRule):
* Source/WebCore/style/RuleSetBuilder.cpp:
(WebCore::Style::RuleSetBuilder::resolveSelectorListWithNesting):

Canonical link: https://commits.webkit.org/266762@main
@tabatkins
Copy link
Member

Neither actually - we need to preserve even things that fail to be a complex-real-selector at all (currently those will return failure). So I'll have to write something a little differently.

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 3, 2023
As per resolution in w3c/csswg-drafts#8356

Safari already does this. Expand the tests they tweaked to be useful
again (test that when non-forgiving parsing is used, then the selector
is invalid).

Differential Revision: https://phabricator.services.mozilla.com/D189666

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1856018
gecko-commit: 21db15048e15cf9e6583d468f53dcac567c47157
gecko-reviewers: dshin
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 4, 2023
As per resolution in w3c/csswg-drafts#8356

Safari already does this. Expand the tests they tweaked to be useful
again (test that when non-forgiving parsing is used, then the selector
is invalid).

Differential Revision: https://phabricator.services.mozilla.com/D189666
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 4, 2023
As per resolution in w3c/csswg-drafts#8356

Safari already does this. Expand the tests they tweaked to be useful
again (test that when non-forgiving parsing is used, then the selector
is invalid).

Differential Revision: https://phabricator.services.mozilla.com/D189666

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1856018
gecko-commit: 2cb6f84a4847fb9df32a8f37fe14deb1ae5602a5
gecko-reviewers: dshin
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 5, 2023
As per resolution in w3c/csswg-drafts#8356

Safari already does this. Expand the tests they tweaked to be useful
again (test that when non-forgiving parsing is used, then the selector
is invalid).

Differential Revision: https://phabricator.services.mozilla.com/D189666
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Oct 5, 2023
As per resolution in w3c/csswg-drafts#8356

Safari already does this. Expand the tests they tweaked to be useful
again (test that when non-forgiving parsing is used, then the selector
is invalid).

Differential Revision: https://phabricator.services.mozilla.com/D189666
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Oct 5, 2023
As per resolution in w3c/csswg-drafts#8356

Safari already does this. Expand the tests they tweaked to be useful
again (test that when non-forgiving parsing is used, then the selector
is invalid).

Differential Revision: https://phabricator.services.mozilla.com/D189666
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 5, 2023
As per resolution in w3c/csswg-drafts#8356

Safari already does this. Expand the tests they tweaked to be useful
again (test that when non-forgiving parsing is used, then the selector
is invalid).

Differential Revision: https://phabricator.services.mozilla.com/D189666

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1856018
gecko-commit: 2cb6f84a4847fb9df32a8f37fe14deb1ae5602a5
gecko-reviewers: dshin
cookiecrook pushed a commit to cookiecrook/wpt that referenced this issue Oct 11, 2023
As per resolution in w3c/csswg-drafts#8356

Safari already does this. Expand the tests they tweaked to be useful
again (test that when non-forgiving parsing is used, then the selector
is invalid).

Differential Revision: https://phabricator.services.mozilla.com/D189666

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1856018
gecko-commit: 2cb6f84a4847fb9df32a8f37fe14deb1ae5602a5
gecko-reviewers: dshin
@sesse
Copy link
Contributor

sesse commented Oct 27, 2023

A few questions that I hope becomes clear when this actually gets a spec: How do these things re-serialize?

:is(,,,) (do we add spaces at opportune points?)
:host(:is(.a, .b+.c, .d)) (do we keep things that are not parse errors per se, merely disallowed, and do we serialize them back with spaces as if they were fully parsed?)
:has(:is(:has( .x ))) (very similar issue)

Lightning00Blade pushed a commit to Lightning00Blade/wpt that referenced this issue Dec 11, 2023
As per resolution in w3c/csswg-drafts#8356

Safari already does this. Expand the tests they tweaked to be useful
again (test that when non-forgiving parsing is used, then the selector
is invalid).

Differential Revision: https://phabricator.services.mozilla.com/D189666

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1856018
gecko-commit: 2cb6f84a4847fb9df32a8f37fe14deb1ae5602a5
gecko-reviewers: dshin
github-actions bot pushed a commit to Loirooriol/stylo that referenced this issue Mar 17, 2024
As per resolution in w3c/csswg-drafts#8356

Safari already does this. Expand the tests they tweaked to be useful
again (test that when non-forgiving parsing is used, then the selector
is invalid).

Differential Revision: https://phabricator.services.mozilla.com/D189666
github-actions bot pushed a commit to Loirooriol/stylo that referenced this issue Mar 17, 2024
As per resolution in w3c/csswg-drafts#8356

Safari already does this. Expand the tests they tweaked to be useful
again (test that when non-forgiving parsing is used, then the selector
is invalid).

Differential Revision: https://phabricator.services.mozilla.com/D189666
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants