Bug 178237

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/

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.

Created attachment 442599 [details] Patch

Created attachment 442615 [details] Patch

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 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?

Created attachment 442680 [details] Patch

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?

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

> 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.

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].

<rdar://problem/84748055>

(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.