WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
178237
Support ::before and ::after pseudo elements after ::slotted
https://bugs.webkit.org/show_bug.cgi?id=178237
Summary
Support ::before and ::after pseudo elements after ::slotted
Rob Dodson
Reported
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()
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
diegocardoso
Comment 1
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/
Stephen Belovarich
Comment 2
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.
Antti Koivisto
Comment 3
2021-10-27 09:49:55 PDT
Created
attachment 442599
[details]
Patch
Antti Koivisto
Comment 4
2021-10-27 11:36:23 PDT
Created
attachment 442615
[details]
Patch
Emilio Cobos Álvarez (:emilio)
Comment 5
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?
Antti Koivisto
Comment 6
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?
Antti Koivisto
Comment 7
2021-10-27 23:59:16 PDT
Created
attachment 442680
[details]
Patch
Ryosuke Niwa
Comment 8
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?
Alan Davalos
Comment 9
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
Antti Koivisto
Comment 10
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.
EWS
Comment 11
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]
.
Radar WebKit Bug Importer
Comment 12
2021-10-28 02:00:25 PDT
<
rdar://problem/84748055
>
Emilio Cobos Álvarez (:emilio)
Comment 13
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug