Bug 178237 - Support ::before and ::after pseudo elements after ::slotted
Summary: Support ::before and ::after pseudo elements after ::slotted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antti Koivisto
URL:
Keywords: InRadar
Depends on:
Blocks: 148695 223814
  Show dependency treegraph
 
Reported: 2017-10-12 15:13 PDT by Rob Dodson
Modified: 2021-10-28 04:03 PDT (History)
21 users (show)

See Also:


Attachments
Patch (41.16 KB, patch)
2021-10-27 09:49 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
Patch (41.24 KB, patch)
2021-10-27 11:36 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
Patch (41.67 KB, patch)
2021-10-27 23:59 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Dodson 2017-10-12 15:13:29 PDT
As discussed in https://github.com/w3c/webcomponents/issues/655 you should be able to use pseudo-element selectors after ::slotted()
Comment 1 diegocardoso 2020-04-27 02:57:58 PDT
Just checked that this issue is still opened.

Linking the code to reproduce it taken from the one of the webcomponents issue: https://jsfiddle.net/webpadawan/v3ksqhcx/
Comment 2 Stephen Belovarich 2021-03-09 21:46:25 PST
This issue blocks my ability as a library author to create a custom element called radio-group that supports slotted <input type="radio"> and style those radio buttons from the context of the radio-group.
Comment 3 Antti Koivisto 2021-10-27 09:49:55 PDT
Created attachment 442599 [details]
Patch
Comment 4 Antti Koivisto 2021-10-27 11:36:23 PDT
Created attachment 442615 [details]
Patch
Comment 5 Emilio Cobos Álvarez (:emilio) 2021-10-27 17:14:09 PDT
Comment on attachment 442615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442615&action=review

Looks sensible to me. non-reviewer r+ I guess :-)

> Source/WebCore/css/SelectorChecker.cpp:431
> +            slot = slot->assignedSlot();

Do you need to bail out if `scopeDepth` is not one after the loop?

> Source/WebCore/css/SelectorChecker.cpp:1171
> +            if (auto* subselector = context.selector->selectorList()->first()) {

Hmm, can this really be null?

> Source/WebCore/css/parser/CSSSelectorParser.cpp:347
> +        // FIXME: A WPT indicates ::is/::where should be parsed but reduce to nothing as their content is not legal in the context.

nit: Only one colon before :is/:where

> Source/WebCore/css/parser/CSSSelectorParser.cpp:372
> +    case CSSSelector::PseudoElementMarker:

Gecko also includes ::placeholder and ::file-selector-button, fwiw: https://searchfox.org/mozilla-central/rev/f8576fec48d866c5f988baaf1fa8d2f8cce2a82f/servo/components/style/gecko/pseudo_element.rs#40-41

> Source/WebCore/style/ElementRuleCollector.cpp:-352
> -    ASSERT(is<HTMLSlotElement>(element()));

It's nice to see this code gone fwiw.

> Source/WebCore/style/ElementRuleCollector.cpp:445
>          auto* selectorForMatching = selector;

You might want to remove this temporary and just use `selector` everywhere?
Comment 6 Antti Koivisto 2021-10-27 21:56:27 PDT
Comment on attachment 442615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442615&action=review

>> Source/WebCore/css/SelectorChecker.cpp:431
>> +            slot = slot->assignedSlot();
> 
> Do you need to bail out if `scopeDepth` is not one after the loop?

(or 0). Yeah, probably.

>> Source/WebCore/css/SelectorChecker.cpp:1171
>> +            if (auto* subselector = context.selector->selectorList()->first()) {
> 
> Hmm, can this really be null?

Not really, should have failed parsing.

>> Source/WebCore/css/parser/CSSSelectorParser.cpp:347
>> +        // FIXME: A WPT indicates ::is/::where should be parsed but reduce to nothing as their content is not legal in the context.
> 
> nit: Only one colon before :is/:where

wonder if anyone gets these consistently right

>> Source/WebCore/css/parser/CSSSelectorParser.cpp:372
>> +    case CSSSelector::PseudoElementMarker:
> 
> Gecko also includes ::placeholder and ::file-selector-button, fwiw: https://searchfox.org/mozilla-central/rev/f8576fec48d866c5f988baaf1fa8d2f8cce2a82f/servo/components/style/gecko/pseudo_element.rs#40-41

We don't have dedicated enum values for those for some reason. I'll add a comment and figure out what to do with them separately.

>> Source/WebCore/style/ElementRuleCollector.cpp:445
>>          auto* selectorForMatching = selector;
> 
> You might want to remove this temporary and just use `selector` everywhere?

true

> LayoutTests/imported/w3c/web-platform-tests/css/css-scoping/slotted-parsing-expected.txt:23
> +FAIL "::slotted(*):is()" should be a valid selector The string did not match the expected pattern.
> +FAIL "::slotted(*):is(:hover)" should be a valid selector The string did not match the expected pattern.
> +FAIL "::slotted(*):is(#id)" should be a valid selector The string did not match the expected pattern.
> +FAIL "::slotted(*):where()" should be a valid selector The string did not match the expected pattern.
> +FAIL "::slotted(*):where(:hover)" should be a valid selector The string did not match the expected pattern.
> +FAIL "::slotted(*):where(#id)" should be a valid selector The string did not match the expected pattern.

Do you happen to know what is the logic/spec text behind these?
Comment 7 Antti Koivisto 2021-10-27 23:59:16 PDT
Created attachment 442680 [details]
Patch
Comment 8 Ryosuke Niwa 2021-10-28 00:50:07 PDT
Comment on attachment 442680 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442680&action=review

> Source/WebCore/style/ElementRuleCollector.cpp:-350
> -std::unique_ptr<RuleSet::RuleDataVector> ElementRuleCollector::collectSlottedPseudoElementRulesForSlot()

Nice!

> Source/WebCore/style/RuleFeature.cpp:91
> +            // FIXME: Implement accurate invalidation.
> +            return matchElement;

Do we have a test for this?
Can we add revalidation tests since we've had a bunch of style revalidation bugs in this area in the past?
Comment 9 Alan Davalos 2021-10-28 01:22:12 PDT
Just a note on the side, I think this other issue: ( https://bugs.webkit.org/show_bug.cgi?id=223814 ) is a duplicate of sorts of this one, you might want to close it once the work here is done
Comment 10 Antti Koivisto 2021-10-28 01:50:06 PDT
> Do we have a test for this?
> Can we add revalidation tests since we've had a bunch of style revalidation
> bugs in this area in the past?

There is a related invalidation WPT that we are failing. I'll look into this in that context.
Comment 11 EWS 2021-10-28 01:59:10 PDT
Committed r284973 (243623@main): <https://commits.webkit.org/243623@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442680 [details].
Comment 12 Radar WebKit Bug Importer 2021-10-28 02:00:25 PDT
<rdar://problem/84748055>
Comment 13 Emilio Cobos Álvarez (:emilio) 2021-10-28 04:03:10 PDT
(In reply to Antti Koivisto from comment #6)
> > LayoutTests/imported/w3c/web-platform-tests/css/css-scoping/slotted-parsing-expected.txt:23
> > +FAIL "::slotted(*):is()" should be a valid selector The string did not match the expected pattern.
> > +FAIL "::slotted(*):is(:hover)" should be a valid selector The string did not match the expected pattern.
> > +FAIL "::slotted(*):is(#id)" should be a valid selector The string did not match the expected pattern.
> > +FAIL "::slotted(*):where()" should be a valid selector The string did not match the expected pattern.
> > +FAIL "::slotted(*):where(:hover)" should be a valid selector The string did not match the expected pattern.
> > +FAIL "::slotted(*):where(#id)" should be a valid selector The string did not match the expected pattern.
> 
> Do you happen to know what is the logic/spec text behind these?

Hmm, so I changed that test in https://github.com/web-platform-tests/wpt/commit/6f085e0f00f64f40b6066edef20344eac8211ab4 because after other pseudos where pseudo-classes are allowed it makes sense to allow :where(). That said, spec-wise I guess it's not clear, I filed https://github.com/w3c/csswg-drafts/issues/5093 a while ago, and there seems to be consensus about that behavior... bug 212049 is relevant here.

For ::slotted(), where other pseudo-classes after it aren't allowed anywhere, I doubt it matters much either way.