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-4] Consider disallowing logical combination pseudo-classes inside :has() #6952

Closed
anttijk opened this issue Jan 14, 2022 · 21 comments

Comments

@anttijk
Copy link

anttijk commented Jan 14, 2022

That is disallow nested :has()

:has(:has(foo))

and

:has(:not(foo))
:has(:is(foo))
:has(:where(foo))

These cases complicate style invalidation in the engine too much while also being difficult to reason about and generally not very useful.

It is also easy to make mistakes with :has(:not(foo)) since it may be non-obvious it is not the same as :not(:has(foo)).

@Loirooriol
Copy link
Contributor

Loirooriol commented Jan 14, 2022

I disagree with "generally not very useful":

  • :has() + :not(): something like :has(#foo:not(.bar)) seems very useful to me.
  • :has() + :is(): useful for complex selectors, e.g. .foo:has(.bar .baz) requires .bar to be a descendant of .foo, but .foo:has(:is(.bar .baz)) doesn't. Sure, I could rewrite it as
    .foo:has(.bar .baz), .foo.bar:has(.baz), .bar .foo:has(.baz)
    but in more complex cases the combinatorial explosion of cases can be cumbersome.
  • :has() + :where(): same

:has() + :has() could have its uses but yeah, generally it seems less useful.

@jensimmons
Copy link
Contributor

jensimmons commented Jan 14, 2022

I'd like to provide some context for those web developers commenting or reacting that they don't like this idea and would prefer every possible combination of selector syntax be allowed.

The reason browsers did not implement a parent selector many years ago is because there are some pretty alarming performance concerns. It could have been very easy to choke a browser from the 2000s, making a web page slow to an unusable state. In fact, even in the first half of 2021, we were still not sure it was possible to implement :has(). Folks at Igalia and Apple worked hard to figure out how to make :has() possible at all, given performance considerations. We shipped an early version in Safari Technology Preview, so we could identify & fix bugs and further test performance.

Everyone would love to implement :has() with no limitations. But that may be impossible. We have to make sure this new CSS selector doesn't give web developers a way to trash the performance of their website.

This issue is for the CSSWG to discuss, as we figure out a way to create a selector that does what most developers need most of the time, without any bad consequences.

Please don't give us a hard time about it. What we wish for and what we need to do because of reality don't always perfectly match. And it is looking terrific for an incredibly useful selector finally becoming reality, after years of pent up desire.

@SelenIT
Copy link
Collaborator

SelenIT commented Jan 14, 2022

I’d be fine with these limitations for :has() in L4 if it helps all browsers implement it sooner. It’s still infinitely better than restricting it to scripting only, as it was proposed earlier. To me, even limiting the argument to one combinator plus one compound selector (instead of arbitrary complex selectors) would be way better than nothing for many practical cases, and if it would allow to get rid of the “optional” status of :has() in the spec, I would accept that trade-off. Further extensions and improvements of this selector could be deferred to L5.

@SelenIT
Copy link
Collaborator

SelenIT commented Jan 16, 2022

One of use cases for :not() inside :has() seems to be telling apart elements containing text from those that contain only other elements (e.g. see this Twitter tread). There would be no need to employ this combination for this task if there would be a dedicated selector for elements with text, e.g. :has-text. Are there any plans to introduce something like this?

@Loirooriol
Copy link
Contributor

To be clear, while I disagreed with these combinations not being useful, I'd fine with the limitations, for the reasons stated above.

But, regarding :not(), I wonder if it could be allowed in its basic L3 form: :not(<simple-selector>), excluding nesting.

@SelenIT
Copy link
Collaborator

SelenIT commented Jan 18, 2022

I guess it's more important to get :has() implemented in browsers sooner, the way it possibly can be implemented. If some reasonable limitation can help it while still allowing to check for dynamic pseudo-classes and DOM modifications inside :has(), let it be so. Something would be better than nothing (and yes, limiting :has() to scripting only was virtually equal to nothing from CSS engineers perspective).

It's another question whether invalidating :not() inside :has() would be that much more complicated than, e.g. invalidating :nth-last-child() (and the latter case would be definitely useful, the ability to switch the container from block to grid and back depending on the number of items is what developers often ask for). But implementers surely know better, let's listen to them!

Also, I guess the question relates basically to all functional pseudo-classes accepting selector lists, so, if the limitation is needed, :nth-child(... of S) inside :has() would be disallowed as well?

webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Jan 18, 2022
https://bugs.webkit.org/show_bug.cgi?id=235231

Reviewed by Dean Jackson.

LayoutTests/imported/w3c:

* web-platform-tests/css/selectors/has-argument-with-explicit-scope.tentative-expected.txt:
* web-platform-tests/css/selectors/has-basic-expected.txt:
* web-platform-tests/css/selectors/parsing/parse-has-expected.txt:

Source/WebCore:

Nested case ':has(:has(foo))' adds no meaningful capability and would complicate invalidation.
See w3c/csswg-drafts#6952 for more details.

* css/parser/CSSSelectorParser.cpp:
(WebCore::CSSSelectorParser::consumePseudo):

Also set m_resistDefaultNamespace like other logical combination pseudo-classes.

* css/parser/CSSSelectorParser.h:



Canonical link: https://commits.webkit.org/246125@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@288111 268f45cc-cd09-0410-ab3c-d52691b4dbfc
annulen pushed a commit to qtwebkit/qtwebkit that referenced this issue Jan 19, 2022
https://bugs.webkit.org/show_bug.cgi?id=235231

Reviewed by Dean Jackson.

LayoutTests/imported/w3c:

* web-platform-tests/css/selectors/has-argument-with-explicit-scope.tentative-expected.txt:
* web-platform-tests/css/selectors/has-basic-expected.txt:
* web-platform-tests/css/selectors/parsing/parse-has-expected.txt:

Source/WebCore:

Nested case ':has(:has(foo))' adds no meaningful capability and would complicate invalidation.
See w3c/csswg-drafts#6952 for more details.

* css/parser/CSSSelectorParser.cpp:
(WebCore::CSSSelectorParser::consumePseudo):

Also set m_resistDefaultNamespace like other logical combination pseudo-classes.

* css/parser/CSSSelectorParser.h:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@288111 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@anttijk
Copy link
Author

anttijk commented Jan 20, 2022

The nested case :has(:has(foo)) is now disallowed in WebKit but the other logical combination pseudo-classes are still allowed. The performance of using those is not optimal but we can probably live with that for now.

I think the nested case should be disallowed by the spec. No one seems to have came up with any real uses for that.

JonWBedard pushed a commit to WebKit/WebKit that referenced this issue Jan 25, 2022
    [:has() pseudo-class] Disallow nested :has()
    https://bugs.webkit.org/show_bug.cgi?id=235231

    Reviewed by Dean Jackson.

    LayoutTests/imported/w3c:

    * web-platform-tests/css/selectors/has-argument-with-explicit-scope.tentative-expected.txt:
    * web-platform-tests/css/selectors/has-basic-expected.txt:
    * web-platform-tests/css/selectors/parsing/parse-has-expected.txt:

    Source/WebCore:

    Nested case ':has(:has(foo))' adds no meaningful capability and would complicate invalidation.
    See w3c/csswg-drafts#6952 for more details.

    * css/parser/CSSSelectorParser.cpp:
    (WebCore::CSSSelectorParser::consumePseudo):

    Also set m_resistDefaultNamespace like other logical combination pseudo-classes.

    * css/parser/CSSSelectorParser.h:

    Canonical link: https://commits.webkit.org/246125@main
    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@288111 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Canonical link: https://commits.webkit.org/245886.30@safari-613-branch
git-svn-id: https://svn.webkit.org/repository/webkit/branches/safari-613-branch@288512 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@fantasai fantasai added the selectors-4 Current Work label Jan 26, 2022
JonWBedard pushed a commit to WebKit/WebKit that referenced this issue Feb 4, 2022
    [:has() pseudo-class] Disallow nested :has()
    https://bugs.webkit.org/show_bug.cgi?id=235231

    Reviewed by Dean Jackson.

    LayoutTests/imported/w3c:

    * web-platform-tests/css/selectors/has-argument-with-explicit-scope.tentative-expected.txt:
    * web-platform-tests/css/selectors/has-basic-expected.txt:
    * web-platform-tests/css/selectors/parsing/parse-has-expected.txt:

    Source/WebCore:

    Nested case ':has(:has(foo))' adds no meaningful capability and would complicate invalidation.
    See w3c/csswg-drafts#6952 for more details.

    * css/parser/CSSSelectorParser.cpp:
    (WebCore::CSSSelectorParser::consumePseudo):

    Also set m_resistDefaultNamespace like other logical combination pseudo-classes.

    * css/parser/CSSSelectorParser.h:

    Canonical link: https://commits.webkit.org/246125@main
    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@288111 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Canonical link: https://commits.webkit.org/245886.30@safari-613-branch
git-svn-id: https://svn.webkit.org/repository/webkit/branches/safari-613-branch@288512 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@tabatkins
Copy link
Member

I'm fine with disallowing nesting. We've done similar in other non-selector cases to limit complexity. There are definitely use-cases for it, but they're indeed minimal.

If at all possible I'd like to avoid disallowing the other selectors, even if the perf isn't ideal. We have much better tools these days to expose and analyze selector performance than we did a decade ago.

I presume this means that nested :has() is disallowed even when "shielded" by another selector, like :has(:not(:has(.foo))), right?

@SelenIT
Copy link
Collaborator

SelenIT commented Feb 5, 2022

Since :has() has proven to be implementable and this one limitation helps keep its performance at least acceptable for most practical cases, maybe it's time to remove its "optional" status and "at risk" mark as well?

@anttijk
Copy link
Author

anttijk commented Feb 8, 2022

Correct. Any nesting, direct or indirect, is disallowed.

@romainmenke
Copy link
Member

romainmenke commented Feb 11, 2022

Can it be clarified if the outer or inner :has is invalid?

a:has(b, :has(> b)) c {
  color: red
}

Is the entire selector invalid in this case or only :has(> b) ?

If only the inner is invalid, this selector would essentially become :

a:has(b) c {
  color: red
}

@tabatkins
Copy link
Member

I'd render the entire selector invalid.

@romainmenke
Copy link
Member

I'd render the entire selector invalid.

I think that would be most clear for stylesheet authors.
A mistake would be noticed quicker.

@anttijk
Copy link
Author

anttijk commented Feb 16, 2022

:has() uses forgiving parsing. Are there existing examples of a failure in forgiving parsing not being forgiven?

@tabatkins
Copy link
Member

It's forgiving for the selectors inside of it, yeah, but the invalidity here is the outer :has(), since it contains a nested :has().

But I could be convinced otherwise, I suppose.

@romainmenke
Copy link
Member

Not sure how much look ahead is needed and allowed, but that could be a factor?
If the inner is invalid it wouldn't require any.

@anttijk
Copy link
Author

anttijk commented Feb 17, 2022

It is simpler to just make :has() invalid inside :has() and then follow the usual error handling rules, both in terms of logic and code.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [selectors-4] Consider disallowing logical combination pseudo-classes inside :has(), and agreed to the following:

  • RESOLVED: Disallow nesting :has() inside :has()
The full IRC log of that discussion <fantasai> Topic: [selectors-4] Consider disallowing logical combination pseudo-classes inside :has()
<fantasai> github: https://github.com/w3c/csswg-drafts/issues/6952https://github.com//issues/6952
<fantasai> futhark: I think there is agreement to allow combinations like :is() and :where()
<fantasai> futhark: But implementors want to disallow :has() inside :has()
<fantasai> bkardell_: I think that's my impression as well. I think Safari implements this way as well
<fantasai> futhark: Implement in chrome and safari, but buggy
<fantasai> futhark: Don't think need to change the spec at this point
<fantasai> futhark: or to add any limitations, other than :has()
<fantasai> TabAtkins: I support this
<TabAtkins> github:
<TabAtkins> github: https://github.com//issues/6952
<fantasai> fantasai: proposed then that :has() cannot be nested inside :has()
<jensimmons> I've already found a useful usecase for figure:not(:has(:not(img))) { ... }
<fantasai> oriol: I think other combinations seem useful, but :has() inside :has() doesn't seem that useful
<fantasai> bkardell_: Originally wanted to disallow all of them, but based on feedback went back and made them work
<fantasai> bkardell_: said some perf implications, but seem livable-with
<fantasai> bkardell_: so let's just prohibit :has() inside :has()
<fantasai> astearns: Other opinions?
<fantasai> RESOLVED: Disallow nesting :has() inside :has()
<TabAtkins> I mean, I'm sure there are use-cases for nested :has(), but they're not as obvious and clearly worth the complexity cost, so I'm happy with this.

@romainmenke
Copy link
Member

romainmenke commented Jun 7, 2022

document.querySelector(":has(:has(body))");	
// vs. 
document.querySelector(":has(:has(body), body)");

In Safari (15.5) the first query throws with SyntaxError: The string did not match the expected pattern.
In Chrome (104) it returns null

The second query returns html in both browsers.

I am unsure which behaviour is correct, but I suspect this is a bug in Webkit if I compare with the behaviour of document.querySelector(":is(:invalid-thing)"); which returns null.

WPT also still lacks tests for error recovery in :has() : web-platform-tests/wpt#32200

byung-woo added a commit to byung-woo/csswg-drafts that referenced this issue Jun 9, 2022
byung-woo added a commit to byung-woo/csswg-drafts that referenced this issue Jun 13, 2022
byung-woo added a commit to byung-woo/csswg-drafts that referenced this issue Jun 13, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 13, 2022
Remove nesting :has() cases from the existing wpt tests and add a
tentative wpt test to check disallowing nesting :has() inside :has().

w3c/csswg-drafts#6952 (comment)

Bug: 669058, 1334631
Change-Id: I549ee9d5b1ca17d22f7f8982d0e9ff96df6937df
aarongable pushed a commit to chromium/chromium that referenced this issue Jun 13, 2022
Remove nesting :has() cases from the existing wpt tests and add a
tentative wpt test to check disallowing nesting :has() inside :has().

w3c/csswg-drafts#6952 (comment)

Bug: 669058, 1334631
Change-Id: I549ee9d5b1ca17d22f7f8982d0e9ff96df6937df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3697654
Commit-Queue: Byungwoo Lee <blee@igalia.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1013385}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 13, 2022
Remove nesting :has() cases from the existing wpt tests and add a
tentative wpt test to check disallowing nesting :has() inside :has().

w3c/csswg-drafts#6952 (comment)

Bug: 669058, 1334631
Change-Id: I549ee9d5b1ca17d22f7f8982d0e9ff96df6937df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3697654
Commit-Queue: Byungwoo Lee <blee@igalia.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1013385}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 13, 2022
Remove nesting :has() cases from the existing wpt tests and add a
tentative wpt test to check disallowing nesting :has() inside :has().

w3c/csswg-drafts#6952 (comment)

Bug: 669058, 1334631
Change-Id: I549ee9d5b1ca17d22f7f8982d0e9ff96df6937df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3697654
Commit-Queue: Byungwoo Lee <blee@igalia.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1013385}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 14, 2022
…2 : disallow nesting :has(), a=testonly

Automatic update from web-platform-tests
Apply the resolution of csswg issue #6952 : disallow nesting :has()

Remove nesting :has() cases from the existing wpt tests and add a
tentative wpt test to check disallowing nesting :has() inside :has().

w3c/csswg-drafts#6952 (comment)

Bug: 669058, 1334631
Change-Id: I549ee9d5b1ca17d22f7f8982d0e9ff96df6937df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3697654
Commit-Queue: Byungwoo Lee <blee@igalia.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1013385}

--

wpt-commits: 74410eadebedbb07ed1ce8031243ac2f7493bf19
wpt-pr: 34385
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jun 16, 2022
…2 : disallow nesting :has(), a=testonly

Automatic update from web-platform-tests
Apply the resolution of csswg issue #6952 : disallow nesting :has()

Remove nesting :has() cases from the existing wpt tests and add a
tentative wpt test to check disallowing nesting :has() inside :has().

w3c/csswg-drafts#6952 (comment)

Bug: 669058, 1334631
Change-Id: I549ee9d5b1ca17d22f7f8982d0e9ff96df6937df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3697654
Commit-Queue: Byungwoo Lee <blee@igalia.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1013385}

--

wpt-commits: 74410eadebedbb07ed1ce8031243ac2f7493bf19
wpt-pr: 34385
@tabatkins
Copy link
Member

Yes, that's a bug in Safari. The tests I just merged from you cover that case, tho. ^_^

mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Remove nesting :has() cases from the existing wpt tests and add a
tentative wpt test to check disallowing nesting :has() inside :has().

w3c/csswg-drafts#6952 (comment)

Bug: 669058, 1334631
Change-Id: I549ee9d5b1ca17d22f7f8982d0e9ff96df6937df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3697654
Commit-Queue: Byungwoo Lee <blee@igalia.com>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1013385}
NOKEYCHECK=True
GitOrigin-RevId: e4a649a2f73af3e9d9445e6b27522ba18572d740
@ByteEater-pl
Copy link

I wonder if scoping and nesting can emulate nested :has(). Specifically, how to rewrite A x B:has(C y D:has(E) F) z G, where capital letters are <relative-selector>s (single for now) and small letters are combinators. Reaching for a workaround would indicate that the author probably does know what it may imply for performance and is convinced it wouldn't hurt in his case.

Or is it impossible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests