http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#selecting-nodes-distributed-to-insertion-points We need CSS4 combinar syntax to support this.
Let me try to implement this.
Created attachment 170083 [details] WIP. update a parser.
Let me explain the current status and the stragety of implementation what's in my mind. This is the first CSS rule which crosses a shadow boundary. Current CSS implementation assumes every final target is applied in current scope. But shadow '/select/' reference combinator is defined in shadow subtree but the node who is matched in the rule is outside of the shadow subtree. This is the first case in the history of CSS, I guess. That makes the implementation too hard. To overcome this, I am now thinking the following approach: In parsing CSS rule: - Done (in the WIP patch). After parsing: - When adding a rule which includes a '/select/' reference combinator, we'll add that to the global scope. ie. Make StyleResolver have m_selectReferenceSelectorCombnatorRuleSet. - Preprocess a selector if it includes a '/select/' reference combinator. For example, suppose we have the following tree and shadow subtrees: - H ---------[SR1] - A - C ---------------------------[SR2] - B - D - F - E (<content selct=A>) - G (<content select=D) Also suppose a selector, 'F > G /select/ D > E /select/ A > B', is defined in the shadow DOM subtree of SR2. In this case, we'll add a psuedo combinator, which is used only internally, at the left-most position of the selector like: SR2 x F > G /select/ D > E /select/ A > B The benefit of this pre-processing is we can check that F is defined in the SR2 scope easily. Note that things are not so simple. We have to check SR2's applyAuthorStyle also. When matching: - Try to match each CSS Selector right-to-left as usual. - If we encounter a '/select' reference combinator, we have to check the all *midpoint* insertion points to where a node is distributed either directly or indirectly. Continue matching for each recursively. If one of them matches, the selector matches.
Created attachment 172054 [details] WIP. A lot of debug code.
It seems the WIP patch contains a issue of life cycle of *something*. Maybe it's StyleRule, I guess. It causes a crash in V8 garbage collection when reloading. Let me continue to investigate the cause. (In reply to comment #4) > Created an attachment (id=172054) [details] > WIP. A lot of debug code.
Created attachment 172889 [details] WIP. No crash.
Created attachment 175365 [details] WIP. Clean up and update a test.
Created attachment 175404 [details] Ready for review. Let me file another bugs later.
This should be announced on webkit-dev first as it's a new feature. Was this discussed in a standards body somewhere? I'm not sure "select" is the best term for this. It's very generic for something so specific. I'm not opposed to implementing it. I'd just like to make sure we ship something that won't change on us.
Comment on attachment 175404 [details] Ready for review. Let me file another bugs later. View in context: https://bugs.webkit.org/attachment.cgi?id=175404&action=review I agree with Ojan, a webkit-dev post would be a good idea. The standardization status of this is also unclear to me. How much review has this received? > Source/WebCore/css/CSSGrammar.y.in:1012 > +reference_combinator: > + '/' IDENT '/' maybe_space { > + CSSParserString& str = $2; > + if (parser->m_context.isHTMLDocument) > + str.lower(); > + $$ = str; > + } > + ; Please put this feature fully behind a feature flag, ENABLE(SHADOW_DOM) probably.
I agree. Let me announce this in webkit-dev. (In reply to comment #9) > This should be announced on webkit-dev first as it's a new feature. > > Was this discussed in a standards body somewhere? I'm not sure "select" is the best term for this. It's very generic for something so specific. The term of 'select' is chosen since it's attribute name of <content> element. There has been some discussions about this reference combinator in W3C's Shadow DOM specification's bugzilla. The current spec is the conclusion of the discussions. > > I'm not opposed to implementing it. I'd just like to make sure we ship something that won't change on us.
Comment on attachment 175404 [details] Ready for review. Let me file another bugs later. View in context: https://bugs.webkit.org/attachment.cgi?id=175404&action=review >> Source/WebCore/css/CSSGrammar.y.in:1012 >> + ; > > Please put this feature fully behind a feature flag, ENABLE(SHADOW_DOM) probably. I agree. Let me update the patch so that this feature is fully behind a SHADOW_DOM flag.
(In reply to comment #11) > The term of 'select' is chosen since it's attribute name of <content> element. > There has been some discussions about this reference combinator in W3C's Shadow DOM specification's bugzilla. The current spec is the conclusion of the discussions. I see. No objection from me on this.
Created attachment 175593 [details] guard by SHADOW_DOM flag
Created attachment 176437 [details] Update. CSSSelectorList is now correctly divided into each RuleSet.
I announced that in webkit-dev a week ago and there is no concerns so far. https://lists.webkit.org/pipermail/webkit-dev/2012-November/022950.html I think it's okay to be reviewed. (In reply to comment #10) > (From update of attachment 175404 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175404&action=review > > I agree with Ojan, a webkit-dev post would be a good idea. The standardization status of this is also unclear to me. How much review has this received? > > > Source/WebCore/css/CSSGrammar.y.in:1012 > > +reference_combinator: > > + '/' IDENT '/' maybe_space { > > + CSSParserString& str = $2; > > + if (parser->m_context.isHTMLDocument) > > + str.lower(); > > + $$ = str; > > + } > > + ; > > Please put this feature fully behind a feature flag, ENABLE(SHADOW_DOM) probably.
(In reply to comment #16) > I announced that in webkit-dev a week ago and there is no concerns so far. > https://lists.webkit.org/pipermail/webkit-dev/2012-November/022950.html > > I think it's okay to be reviewed. Tab started a thread on www-style to make double-darn sure that this is the right approach. Can we give it a couple of days and ensure that we're using the right combinator? http://lists.w3.org/Archives/Public/www-style/2012Nov/thread.html#msg428
Created attachment 176698 [details] Add test. A select reference combinator is used in selecor list.
Yeah, let me keep watch on that. (In reply to comment #17) > (In reply to comment #16) > > I announced that in webkit-dev a week ago and there is no concerns so far. > > https://lists.webkit.org/pipermail/webkit-dev/2012-November/022950.html > > > > I think it's okay to be reviewed. > > Tab started a thread on www-style to make double-darn sure that this is the right approach. Can we give it a couple of days and ensure that we're using the right combinator? > > http://lists.w3.org/Archives/Public/www-style/2012Nov/thread.html#msg428
Comment on attachment 176698 [details] Add test. A select reference combinator is used in selecor list. View in context: https://bugs.webkit.org/attachment.cgi?id=176698&action=review > Source/WebCore/css/CSSSelector.h:265 > + QualifiedName m_reference; // used for reference combinator Could you hide this behind the ifdef? Having empty/dummy API for non-SHADOW_DOM port is OK, but allocating extra memory might not. > Source/WebCore/css/CSSSelectorList.cpp:219 > + return selector->isReferenceCombinator() && selector->reference() == "select"; Let's define static QualifiedName instead of comparing against string literal. > Source/WebCore/css/RuleSet.cpp:286 > +#if ENABLE(SHADOW_DOM) Do you thins it is good idea to move this ifdef in StyleResolver::addStyleRule() ? I guess the ifdef might be narrower without hurting simplicity much. > Source/WebCore/css/SelectorChecker.cpp:593 > + if (context.selector->reference() != "select") Ditto.
Comment on attachment 176698 [details] Add test. A select reference combinator is used in selecor list. View in context: https://bugs.webkit.org/attachment.cgi?id=176698&action=review > Source/WebCore/css/CSSGrammar.y.in:1115 > + fprintf(stderr, "Reference Combinator!\n"); Let's remove this. > Source/WebCore/css/SelectorChecker.cpp:596 > + Vector<InsertionPoint*> insertionPoints; Let's use Vector<InsertionPoint*, 8> or something so as not to allocate heap memory much.
Created attachment 176709 [details] Addressed comments
Comment on attachment 176698 [details] Add test. A select reference combinator is used in selecor list. Thank you for the review. View in context: https://bugs.webkit.org/attachment.cgi?id=176698&action=review >> Source/WebCore/css/CSSGrammar.y.in:1115 >> + fprintf(stderr, "Reference Combinator!\n"); > > Let's remove this. Done. >> Source/WebCore/css/CSSSelector.h:265 >> + QualifiedName m_reference; // used for reference combinator > > Could you hide this behind the ifdef? Having empty/dummy API for non-SHADOW_DOM port is OK, but allocating extra memory might not. Done. >> Source/WebCore/css/CSSSelectorList.cpp:219 >> + return selector->isReferenceCombinator() && selector->reference() == "select"; > > Let's define static QualifiedName instead of comparing against string literal. Let me use HTMLNames::selectAttr. >> Source/WebCore/css/RuleSet.cpp:286 >> +#if ENABLE(SHADOW_DOM) > > Do you thins it is good idea to move this ifdef in StyleResolver::addStyleRule() ? I guess the ifdef might be narrower without hurting simplicity much. Done. That's moved. >> Source/WebCore/css/SelectorChecker.cpp:593 >> + if (context.selector->reference() != "select") > > Ditto. Done. >> Source/WebCore/css/SelectorChecker.cpp:596 >> + Vector<InsertionPoint*> insertionPoints; > > Let's use Vector<InsertionPoint*, 8> or something so as not to allocate heap memory much. Done.
Comment on attachment 176709 [details] Addressed comments Attachment 176709 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15058006
Created attachment 176878 [details] Fix a mac build.
Created attachment 176892 [details] Minor update.
The bug in the spec side was updated: https://www.w3.org/Bugs/Public/show_bug.cgi?id=19684
I've changed the subject of this issue, respecting the recent discussion on the spec. (In reply to comment #27) > The bug in the spec side was updated: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=19684
Created attachment 180316 [details] Implement a ::distributed pseudo elements
I am aware that we have to update the spec at first, but early feedbacks are welcome for the patch. (In reply to comment #29) > Created an attachment (id=180316) [details] > Implement a ::distributed pseudo elements
Comment on attachment 180316 [details] Implement a ::distributed pseudo elements Attachment 180316 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15418917
Comment on attachment 180316 [details] Implement a ::distributed pseudo elements Attachment 180316 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15449035
Comment on attachment 180316 [details] Implement a ::distributed pseudo elements Attachment 180316 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15446071
Comment on attachment 180316 [details] Implement a ::distributed pseudo elements View in context: https://bugs.webkit.org/attachment.cgi?id=180316&action=review > Source/WebCore/ChangeLog:17 > + This patch implements a select reference combinator as the first > + reference combinator implemented in WebKit. Since WebKit has not > + supported reference combinators so far, CSS grammer and parser are > + updated to support the new syntax used in Selector Level4's > + reference combinators. This is not necessary anymore, right? > Source/WebCore/html/shadow/InsertionPoint.h:183 > +// FIXME: Collects only HTMLContentElement since a select reference combinator can be > +// applied to only <content> elements. What is here to fix? It's unclear from the comment. Better yet, provide a link to a bug.
Comment on attachment 180316 [details] Implement a ::distributed pseudo elements Attachment 180316 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15457208
Created attachment 181474 [details] Add more tests. Update ChangeLog. Make it compilable without SHADOW_DOM flag. Support Reprojection.
Thank you for the review. (In reply to comment #34) > (From update of attachment 180316 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180316&action=review > > > Source/WebCore/ChangeLog:17 > > + This patch implements a select reference combinator as the first > > + reference combinator implemented in WebKit. Since WebKit has not > > + supported reference combinators so far, CSS grammer and parser are > > + updated to support the new syntax used in Selector Level4's > > + reference combinators. > > This is not necessary anymore, right? I've updated ChangeLog. > > > Source/WebCore/html/shadow/InsertionPoint.h:183 > > +// FIXME: Collects only HTMLContentElement since a select reference combinator can be > > +// applied to only <content> elements. > > What is here to fix? It's unclear from the comment. Better yet, provide a link to a bug. This FIXME comment is valid only for select' reference combinator. I've removed the FIXME comment.
Created attachment 181476 [details] Remove rule_bison.py from the previous patch.
Comment on attachment 181476 [details] Remove rule_bison.py from the previous patch. Attachment 181476 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15740645 New failing tests: scrollbars/scrollbar-orientation.html scrollbars/disabled-scrollbar.html inspector/styles/region-style-crash.html fast/forms/textarea-metrics.html scrollbars/scrollbar-buttons.html scrollbars/scrollbars-on-positioned-content.html scrollbars/overflow-scrollbar-combinations.html scrollbars/basic-scrollbar.html scrollbars/scrollbar-drag-thumb-with-large-content.html
Comment on attachment 181476 [details] Remove rule_bison.py from the previous patch. View in context: https://bugs.webkit.org/attachment.cgi?id=181476&action=review > Source/WebCore/ChangeLog:25 > + of each RuleSet mapped by a scope where a '::distributed()' pseudo > + element is originally defined. This scope is passed to it's not a pseudo element, right? It's a function. > Source/WebCore/ChangeLog:27 > + exists in the scope where the rule is originally defined. I think using the word "last" here is disorienting, since it's hard to understand what "last" means in this context. If anything, it's "first", since the selectors are resolved backwards. > Source/WebCore/css/CSSParser.cpp:10608 > +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers) The name of the function is now misleading. You're not only updating specifiers, but also occasionally change the specifiers. > Source/WebCore/css/CSSParser.cpp:10629 > + end->setTagHistory(elementNameSelector.release()); > + end->setRelation(CSSSelector::ShadowDistributed); Is this necessary? Can we not just store these like we do in -webkit-any? > Source/WebCore/css/CSSSelector.h:80 > + ShadowDistributed, I am not convinced we need to treat "distributed" function as a relation. > Source/WebCore/css/SelectorChecker.cpp:289 > + return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally; Why do you store the element, when all you need is its scope? > Source/WebCore/css/StyleResolver.cpp:454 > +void StyleResolver::addStyleRule(StyleRule* rule, ContainerNode* scope, RuleSet* ruleSet, AddRuleFlags addRuleFlags) Ugh, what a nasty detour back into StyleResolver. Is this just because we need to populate the map?
Comment on attachment 181476 [details] Remove rule_bison.py from the previous patch. Attachment 181476 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15737930 New failing tests: platform/chromium/compositing/layout-width-change.html platform/chromium/compositing/render-surface-alpha-blending.html fast/text-autosizing/constrained-then-position-absolute-ancestors.html platform/chromium/compositing/3d-corners.html platform/chromium/compositing/video-frame-size-change.html platform/chromium/compositing/perpendicular-layer-sorting.html platform/chromium/compositing/child-layer-3d-sorting.html platform/chromium/compositing/accelerated-drawing/svg-filters.html fast/css/sticky/sticky-top-zoomed.html fast/scrolling/scrollbar-tickmarks-styled.html compositing/scrollbar-painting.html fast/repaint/fixed-right-in-page-scale.html platform/chromium/compositing/accelerated-drawing/alpha.html fast/events/scale-and-scroll-window.html fast/loader/text-document-wrapping.html compositing/overflow/clip-content-under-overflow-controls.html fast/events/scale-and-scroll-body.html fast/forms/textarea-metrics.html fast/text-autosizing/constrained-and-overflow-scroll-ancestor.html platform/chromium/compositing/backface-visibility-transformed.html fast/frames/transparent-scrollbar.html fast/repaint/fixed-right-bottom-in-page-scale.html fast/events/scale-and-scroll-iframe-body.html fast/events/scale-and-scroll-iframe-window.html fast/loader/javascript-url-in-object.html fast/repaint/fixed-in-page-scale.html platform/chromium/compositing/img-layer-grow.html
Comment on attachment 181476 [details] Remove rule_bison.py from the previous patch. Attachment 181476 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15741880 New failing tests: fast/text-autosizing/constrained-then-position-absolute-ancestors.html fast/forms/textarea-metrics.html fast/css/sticky/sticky-top-zoomed.html fast/scrolling/scrollbar-tickmarks-styled.html compositing/scrollbar-painting.html fast/repaint/fixed-right-in-page-scale.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbars-on-positioned-content.html fast/events/scale-and-scroll-window.html fast/loader/text-document-wrapping.html fast/text/international/hindi-spacing.html compositing/overflow/clip-content-under-overflow-controls.html fast/events/scale-and-scroll-iframe-body.html fast/text-autosizing/constrained-and-overflow-scroll-ancestor.html fast/frames/transparent-scrollbar.html fast/repaint/fixed-right-bottom-in-page-scale.html fast/events/scale-and-scroll-body.html fast/events/scale-and-scroll-iframe-window.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/listbox-scrollbar-combinations.html fast/repaint/fixed-in-page-scale.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/basic-scrollbar.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbar-orientation.html fast/loader/javascript-url-in-object.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbar-buttons.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/custom-scrollbar-table-cell.html platform/chromium/virtual/gpu/compositedscrolling/overflow/clip-content-under-overflow-controls.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/disabled-scrollbar.html
Comment on attachment 181476 [details] Remove rule_bison.py from the previous patch. Thank you for the review. I don't have a new patch yet. Let me reply your comments for now. View in context: https://bugs.webkit.org/attachment.cgi?id=181476&action=review >> Source/WebCore/ChangeLog:25 >> + element is originally defined. This scope is passed to > > it's not a pseudo element, right? It's a function. '::distributed(..)' seems the first pseudo element which can take parameters. I think we should follow the same naming convention which is used for pseudo classes. http://dev.w3.org/csswg/selectors4/#pseudo-classes We should call it 'a functional pseudo element', shouldn't we? >> Source/WebCore/ChangeLog:27 >> + exists in the scope where the rule is originally defined. > > I think using the word "last" here is disorienting, since it's hard to understand what "last" means in this context. If anything, it's "first", since the selectors are resolved backwards. That's the 'last' element actually in that sense. That's the left-most element in the selector. Using 'lastly' instead of 'last' makes sense? Let me update the ChangeLog. The scope is passed to SelectorChecker and is used to check whether the lastly matched element exists in the scope where the rule is originally defined. >> Source/WebCore/css/CSSParser.cpp:10608 >> +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers) > > The name of the function is now misleading. You're not only updating specifiers, but also occasionally change the specifiers. Hmm. Could you clarify a difference between update and change in this context? I am not confident how we should name this function. ShadowDecentRelation seems to do the similar things to what I added to this function already. >> Source/WebCore/css/CSSParser.cpp:10629 >> + end->setRelation(CSSSelector::ShadowDistributed); > > Is this necessary? Can we not just store these like we do in -webkit-any? As long as we use Relation, there is no such 'generic' relation which we can reuse with a parameter of '-webkit-any-xx', The current enum Relation is defined as: enum Relation { Descendant = 0, Child, DirectAdjacent, IndirectAdjacent, SubSelector, ShadowDescendant, ShadowDistributed, }; Does this answer makes sense? Are you suggesting we should not use Relation here? >> Source/WebCore/css/CSSSelector.h:80 >> + ShadowDistributed, > > I am not convinced we need to treat "distributed" function as a relation. The makes an implementation much simpler. Let me explain. Let's assume the following selector is given: A > content::distributed(B > content(distributed::(C > D))) In this selector, D is the element which should be matched and should be checked at first. If we convert this selector to a selector which uses a relation internally like: A > content (distributed>) B > content (distributed>) C > D SelectorChecker::checkSelector can receive 'D' at first. It's right-most element. We cau re-use the current implementation of SelectorChecker (right-to-left, backwards) in this case. If we don't convert '::distributed()' to the relation, SelectorChecker::checkSelector will receive a 'content' (in 'A > content') as the first element which should be checked. I am afraid that makes an implementation of checking complicated. We have to retrieve the inner-most element, 'D', somehow, and start matching from 'D'. And we have to go backword from 'selector given as a parameter' to a insertion point somehow. I've abandonded this idea at early stage since converting it to the relation makes an implementation much simpler. >> Source/WebCore/css/SelectorChecker.cpp:289 >> + return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally; > > Why do you store the element, when all you need is its scope? This is a due to a historical reason in this patch. As FIXME says, this part is WIP. I have to fix this expression, line 289. To check precisely, we need an element instead of treeScope here. I have not implemented that yet. For example, <shadow root> <div class='A' <div class='B'> <content></content> </div> </div> <div id='C'> <div id='D'> <style scoped> .A .B content distributed::(*) { color: green } </style> </div> </div> I've not tried, but I guess this style rule matches wrongly. Since in this expression, > return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally context.scopeOfLastElement is 'D' and context.elemetn is 'A'. The treeScopes of both are same. So we need to store ContainerNode, 'D', instead of its treeScope. Let me try to implement checking correctly using a given element in the following patch. >> Source/WebCore/css/StyleResolver.cpp:454 >> +void StyleResolver::addStyleRule(StyleRule* rule, ContainerNode* scope, RuleSet* ruleSet, AddRuleFlags addRuleFlags) > > Ugh, what a nasty detour back into StyleResolver. Is this just because we need to populate the map? I feel the same thing. Agreed that's ugly. > Is this just because we need to populate the map? Yes. That's the reason. I am hoping we have a better way.
Created attachment 181656 [details] Update ChangeLog. Check scopeOfLastElement using isDescendantof
Created attachment 181659 [details] Add a test for ::distributed() used in scoped style.
Comment on attachment 181659 [details] Add a test for ::distributed() used in scoped style. Attachment 181659 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15740987 New failing tests: scrollbars/scrollbar-orientation.html scrollbars/disabled-scrollbar.html inspector/styles/region-style-crash.html fast/forms/textarea-metrics.html scrollbars/scrollbar-drag-thumb-with-large-content.html scrollbars/scrollbars-on-positioned-content.html scrollbars/overflow-scrollbar-combinations.html scrollbars/basic-scrollbar.html scrollbars/scrollbar-buttons.html
Comment on attachment 181659 [details] Add a test for ::distributed() used in scoped style. View in context: https://bugs.webkit.org/attachment.cgi?id=181659&action=review > Source/WebCore/css/RuleSet.cpp:291 > if (rule->isStyleRule()) > - addStyleRule(static_cast<StyleRule*>(rule), addRuleFlags); > + resolver->addStyleRule(static_cast<StyleRule*>(rule), const_cast<ContainerNode*>(scope), this, addRuleFlags); RuleSet calls to StyleResolver::addStyleRule which then calls back to RuleSet::addStyleRule. This is bit of spaghetti. Please factor differently (for example by adding accessors for getting m_shadowDistributedRuleSetMap here). > Source/WebCore/css/SelectorChecker.cpp:394 > + > + case CSSSelector::ShadowDistributed: Please use SHADOW_DOM guards consistently.
Created attachment 181830 [details] factor addStyleRule.
Comment on attachment 181659 [details] Add a test for ::distributed() used in scoped style. Thank you for the review. View in context: https://bugs.webkit.org/attachment.cgi?id=181659&action=review >> Source/WebCore/css/RuleSet.cpp:291 >> + resolver->addStyleRule(static_cast<StyleRule*>(rule), const_cast<ContainerNode*>(scope), this, addRuleFlags); > > RuleSet calls to StyleResolver::addStyleRule which then calls back to RuleSet::addStyleRule. This is bit of spaghetti. Please factor differently (for example by adding accessors for getting m_shadowDistributedRuleSetMap here). Done. I've removed StyleResolver::addStyleRule. Most of code came back to RuleSet::addRulesFromSheet. I've introduced a StyleResolver::addShadowDistributedRule() which is called from there. >> Source/WebCore/css/SelectorChecker.cpp:394 >> + case CSSSelector::ShadowDistributed: > > Please use SHADOW_DOM guards consistently. Done.
Comment on attachment 181830 [details] factor addStyleRule. Attachment 181830 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15754564
Comment on attachment 181830 [details] factor addStyleRule. Attachment 181830 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15771312
Let me fix a build without SHADOW_DOM flag. (In reply to comment #50) > (From update of attachment 181830 [details]) > Attachment 181830 [details] did not pass qt-ews (qt): > Output: http://queues.webkit.org/results/15754564
Comment on attachment 181830 [details] factor addStyleRule. Attachment 181830 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15757360
Comment on attachment 181830 [details] factor addStyleRule. Attachment 181830 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15775198
Comment on attachment 181830 [details] factor addStyleRule. Attachment 181830 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15758388
Comment on attachment 181830 [details] factor addStyleRule. Attachment 181830 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15755414
Comment on attachment 181830 [details] factor addStyleRule. Attachment 181830 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15756419
Comment on attachment 181830 [details] factor addStyleRule. View in context: https://bugs.webkit.org/attachment.cgi?id=181830&action=review > Source/WebCore/ChangeLog:8 > + Implements a '::distributed()' pseudo element. Is this a pseudo-element or a pseudo-class? > Source/WebCore/ChangeLog:11 > + http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#selecting-nodes-distributed-to-insertion-points There seems to be a gap between this spec—/select/—and what you’ve implemented here—::distributed(). Who is responsible for resolving this inconsistency? > Source/WebCore/ChangeLog:13 > + A '::distributed()' pseudo element is the first CSS rule which Great explanation of why this is special. > Source/WebCore/ChangeLog:26 > + SelectorChecker and is used to check whether the lastly matched element I think you should give an example in concrete syntax to make this clearer (and avoid "lastly".) I think you mean A::distributed(B) This scope is passed to SelectorChecker and is used to check whether A exists in the scope where the rule is defined. > Source/WebCore/ChangeLog:31 > + from a distrirbuted pseudo element to a ShadowDistributed is done Spelling distributed > Source/WebCore/css/CSSParser.cpp:9504 > + else if (isEqualToCSSIdentifier(name, "distributed")) Should this be webkit- prefixed for now? > Source/WebCore/css/CSSParser.cpp:10608 > +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers) Would it be less error-prone to leave this returning void and make it take CSSParserSelector**? Perhaps this should be renamed, for example rewriteSpecifiersWtihElementName > Source/WebCore/css/CSSParserValues.h:194 > + bool isPseudoDistributedElement() const { return m_selector->isDistributedPseudoElement(); } It seems needlessly inconsistent to have isPseudoDistributedElement and isDistributedPseudoElement. > Source/WebCore/css/CSSSelector.h:318 > + Nit: Delete this blank line. > Source/WebCore/css/StyleResolver.cpp:368 > + m_shadowDistributedRuleSetMap.clear(); Is this enough to stop m_shadowDistributedRuleSetMap growing unboundedly in common uses of web components? For example, if we add and remove a lot of components over time, will that reset styles and cause this map to shrink? Who owns the key to the map? What stops the key outliving the actual node? > Source/WebCore/html/shadow/InsertionPoint.h:182 > +inline void collectAllMidpointInsertionPoints(const Node* projectedNode, Vector<InsertionPoint*, 8>& results) What does 'Midpoint' mean here? Maybe intermediate is a better word? Maybe collectAllInsertionPoints is even clearer? > Source/WebCore/html/shadow/InsertionPoint.h:184 > + InsertionPoint* insertionPoint = 0; Why do you need this variable? Just use the local variable scoped to the if. > LayoutTests/fast/dom/shadow/distributed-pseudo-element-nested.html:10 > +shadowStyle.innerHTML = 'div content::distributed(div content::distributed(div.green)) { color: green; }'; You should include a test where the thing directly to the left of ::distributed is neither content nor shadow, and does match (eg *) and does not match (eg span or some other non-insertion-point.)
Comment on attachment 181830 [details] factor addStyleRule. All these ifdefs and extra gobs of code in StyleResolver make me super-sad. There has got to be a better way.
Comment on attachment 181476 [details] Remove rule_bison.py from the previous patch. View in context: https://bugs.webkit.org/attachment.cgi?id=181476&action=review >>> Source/WebCore/ChangeLog:27 >>> + exists in the scope where the rule is originally defined. >> >> I think using the word "last" here is disorienting, since it's hard to understand what "last" means in this context. If anything, it's "first", since the selectors are resolved backwards. > > That's the 'last' element actually in that sense. That's the left-most element in the selector. Using 'lastly' instead of 'last' makes sense? Let me update the ChangeLog. > > The scope is passed to SelectorChecker and is used to check whether the lastly matched element > exists in the scope where the rule is originally defined. lastly sounds awkward. >>> Source/WebCore/css/CSSParser.cpp:10608 >>> +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers) >> >> The name of the function is now misleading. You're not only updating specifiers, but also occasionally change the specifiers. > > Hmm. Could you clarify a difference between update and change in this context? > I am not confident how we should name this function. > > ShadowDecentRelation seems to do the similar things to what I added to this function already. True, though ShadowDescendantRelation doesn't turn around and return a whole different selector to replace the one that's being processed as $$. >>> Source/WebCore/css/CSSParser.cpp:10629 >>> + end->setRelation(CSSSelector::ShadowDistributed); >> >> Is this necessary? Can we not just store these like we do in -webkit-any? > > As long as we use Relation, there is no such 'generic' relation which we can reuse with a parameter of '-webkit-any-xx', > > The current enum Relation is defined as: > > enum Relation { > Descendant = 0, > Child, > DirectAdjacent, > IndirectAdjacent, > SubSelector, > ShadowDescendant, > ShadowDistributed, > }; > > > Does this answer makes sense? Are you suggesting we should not use Relation here? I was suggesting not using relation, but now I realize I need to first fully comprehend whether it's possible. > Source/WebCore/css/CSSParserValues.h:194 > + bool isPseudoDistributedElement() const { return m_selector->isDistributedPseudoElement(); } Do we need this anywhere? I wonder if the "::distributed(" is not the name of the function, but rather the name of the relation. >>> Source/WebCore/css/CSSSelector.h:80 >>> + ShadowDistributed, >> >> I am not convinced we need to treat "distributed" function as a relation. > > The makes an implementation much simpler. Let me explain. Let's assume the following selector is given: > > A > content::distributed(B > content(distributed::(C > D))) > > In this selector, D is the element which should be matched and should be checked at first. If we convert this selector to a selector which uses a relation internally like: > > A > content (distributed>) B > content (distributed>) C > D > > SelectorChecker::checkSelector can receive 'D' at first. It's right-most element. We cau re-use the current implementation of SelectorChecker (right-to-left, backwards) in this case. > > If we don't convert '::distributed()' to the relation, SelectorChecker::checkSelector will receive a 'content' (in 'A > content') as the first element which should be checked. > I am afraid that makes an implementation of checking complicated. We have to retrieve the inner-most element, 'D', somehow, and start matching from 'D'. And we have to go backword from 'selector given as a parameter' to a insertion point somehow. > > I've abandonded this idea at early stage since converting it to the relation makes an implementation much simpler. I understand. We are introducing a whole new type of CSS functions into WebKit, the CSS pseudo element functions. Per discussion with Tab, this is just a first one (lucky us). >>> Source/WebCore/css/SelectorChecker.cpp:289 >>> + return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally; >> >> Why do you store the element, when all you need is its scope? > > This is a due to a historical reason in this patch. As FIXME says, this part is WIP. I have to fix this expression, line 289. > To check precisely, we need an element instead of treeScope here. I have not implemented that yet. > > For example, > > <shadow root> > <div class='A' > <div class='B'> > <content></content> > </div> > </div> > <div id='C'> > <div id='D'> > <style scoped> > .A .B content distributed::(*) { color: green } > </style> > </div> > </div> > > I've not tried, but I guess this style rule matches wrongly. Since in this expression, I see. You really need to expand on this. Also, we should have a type that's different from ContainerNode to denote a scoping element. Even if it's a typedef. > Source/WebCore/css/StyleResolver.cpp:838 > + collectMatchingRules(it->value.get(), result.ranges.firstAuthorRule, result.ranges.lastAuthorRule, options); Ideally, the data structure that deals with ::distributed rules should be completely factored out of the StyleResolver and its logic encapsulated. This collectMatchingRules call is what makes it difficult. I am wondering if it's time to decouple collecting rules for an element into a separate class from StyleResolver, and then allow passing around its instance to collect rules. This should enable creating ::distributed rules class.
Comment on attachment 181830 [details] factor addStyleRule. View in context: https://bugs.webkit.org/attachment.cgi?id=181830&action=review >> Source/WebCore/ChangeLog:13 >> + A '::distributed()' pseudo element is the first CSS rule which > > Great explanation of why this is special. It's also the first-ever pseudo element function in WebKit. >> Source/WebCore/css/CSSParser.cpp:10608 >> +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers) > > Would it be less error-prone to leave this returning void and make it take CSSParserSelector**? > > Perhaps this should be renamed, for example > > rewriteSpecifiersWtihElementName It seems to me this whole bit of machinery needs to be first cleaned up and named according to purpose. Stuffing more stuff in it just looks bad. > Source/WebCore/css/SelectorChecker.h:79 > + const ContainerNode* scopeOfLastElement; I am still unhappy about the name. If I have to think about what this means, imagine what a person who didn't write the Shadow DOM spec would. We must do better.
Created attachment 182063 [details] iteration. A lot of refinements.
Thank you for the review. Let me reply to only the comments I've addressed. There still remains to-do things. (In reply to comment #58) > (From update of attachment 181830 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181830&action=review > > > Source/WebCore/ChangeLog:8 > > + Implements a '::distributed()' pseudo element. > > Is this a pseudo-element or a pseudo-class? As discussed in another threads, it should be a pseudo element. > > > Source/WebCore/ChangeLog:11 > > + http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#selecting-nodes-distributed-to-insertion-points > > There seems to be a gap between this spec—/select/—and what you’ve implemented here—::distributed(). Who is responsible for resolving this inconsistency? > > > Source/WebCore/ChangeLog:13 > > + A '::distributed()' pseudo element is the first CSS rule which > > Great explanation of why this is special. > > > Source/WebCore/ChangeLog:26 > > + SelectorChecker and is used to check whether the lastly matched element > > I think you should give an example in concrete syntax to make this clearer (and avoid "lastly".) I think you mean > > A::distributed(B) > > This scope is passed to SelectorChecker and is used to check whether A exists in the scope where the rule is defined. I've updated the ChangeLog. > > > Source/WebCore/ChangeLog:31 > > + from a distrirbuted pseudo element to a ShadowDistributed is done > > Spelling distributed Done. > > > Source/WebCore/css/CSSParser.cpp:9504 > > + else if (isEqualToCSSIdentifier(name, "distributed")) > > Should this be webkit- prefixed for now? Done. Now it's a '-webkit-distributed::(...)' > > > Source/WebCore/css/CSSParser.cpp:10608 > > +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers) > > Would it be less error-prone to leave this returning void and make it take CSSParserSelector**? I don't think it's worth introducing CSSParseSelector** here. The current code is not so match bad so far and be consistent with updateSpecifiers() which also returns CSSParserSelector*. > > Perhaps this should be renamed, for example > > rewriteSpecifiersWtihElementName Agreed on renaming. Done. > > > Source/WebCore/css/CSSParserValues.h:194 > > + bool isPseudoDistributedElement() const { return m_selector->isDistributedPseudoElement(); } > > It seems needlessly inconsistent to have isPseudoDistributedElement and isDistributedPseudoElement. Done. > > > Source/WebCore/css/CSSSelector.h:318 > > + > > Nit: Delete this blank line. Done. > > > Source/WebCore/css/StyleResolver.cpp:368 > > + m_shadowDistributedRuleSetMap.clear(); > > Is this enough to stop m_shadowDistributedRuleSetMap growing unboundedly in common uses of web components? For example, if we add and remove a lot of components over time, will that reset styles and cause this map to shrink? > > Who owns the key to the map? What stops the key outliving the actual node? At the first glance, if a node of scope where style is defined is deleted, style node is also deleted as well. So eventually StyleResolver's reset() will be called. Let me investigate that. > > > Source/WebCore/html/shadow/InsertionPoint.h:182 > > +inline void collectAllMidpointInsertionPoints(const Node* projectedNode, Vector<InsertionPoint*, 8>& results) > > What does 'Midpoint' mean here? Maybe intermediate is a better word? Maybe collectAllInsertionPoints is even clearer? Done. Renamed it to collectAllInsertionPoints. > > > Source/WebCore/html/shadow/InsertionPoint.h:184 > > + InsertionPoint* insertionPoint = 0; > > Why do you need this variable? Just use the local variable scoped to the if. Right. This is a mistake. Done. > > > LayoutTests/fast/dom/shadow/distributed-pseudo-element-nested.html:10 > > +shadowStyle.innerHTML = 'div content::distributed(div content::distributed(div.green)) { color: green; }'; > > You should include a test where the thing directly to the left of ::distributed is neither content nor shadow, and does match (eg *) and does not match (eg span or some other non-insertion-point.) Done. I've added a few more tests.
Thank you for the review. (In reply to comment #60) > (From update of attachment 181476 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181476&action=review > > >>> Source/WebCore/ChangeLog:27 > >>> + exists in the scope where the rule is originally defined. > >> > >> I think using the word "last" here is disorienting, since it's hard to understand what "last" means in this context. If anything, it's "first", since the selectors are resolved backwards. > > > > That's the 'last' element actually in that sense. That's the left-most element in the selector. Using 'lastly' instead of 'last' makes sense? Let me update the ChangeLog. > > > > The scope is passed to SelectorChecker and is used to check whether the lastly matched element > > exists in the scope where the rule is originally defined. > > lastly sounds awkward. I've updated the ChangeLog. > > >>> Source/WebCore/css/CSSParser.cpp:10608 > >>> +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers) > >> > >> The name of the function is now misleading. You're not only updating specifiers, but also occasionally change the specifiers. > > > > Hmm. Could you clarify a difference between update and change in this context? > > I am not confident how we should name this function. > > > > ShadowDecentRelation seems to do the similar things to what I added to this function already. > > True, though ShadowDescendantRelation doesn't turn around and return a whole different selector to replace the one that's being processed as $$. > > >>> Source/WebCore/css/CSSParser.cpp:10629 > >>> + end->setRelation(CSSSelector::ShadowDistributed); > >> > >> Is this necessary? Can we not just store these like we do in -webkit-any? > > > > As long as we use Relation, there is no such 'generic' relation which we can reuse with a parameter of '-webkit-any-xx', > > > > The current enum Relation is defined as: > > > > enum Relation { > > Descendant = 0, > > Child, > > DirectAdjacent, > > IndirectAdjacent, > > SubSelector, > > ShadowDescendant, > > ShadowDistributed, > > }; > > > > > > Does this answer makes sense? Are you suggesting we should not use Relation here? > > I was suggesting not using relation, but now I realize I need to first fully comprehend whether it's possible. Okay. I think using relation is the best way to represent this *relation*. > > > Source/WebCore/css/CSSParserValues.h:194 > > + bool isPseudoDistributedElement() const { return m_selector->isDistributedPseudoElement(); } > > Do we need this anywhere? I wonder if the "::distributed(" is not the name of the function, but rather the name of the relation. It's used in parsing stage. After paring '::distributed() as a functional pseudo element, it is transformed to the relation. The ShadowDistributed relation does not have a name (as a string) internally. > > >>> Source/WebCore/css/CSSSelector.h:80 > >>> + ShadowDistributed, > >> > >> I am not convinced we need to treat "distributed" function as a relation. > > > > The makes an implementation much simpler. Let me explain. Let's assume the following selector is given: > > > > A > content::distributed(B > content(distributed::(C > D))) > > > > In this selector, D is the element which should be matched and should be checked at first. If we convert this selector to a selector which uses a relation internally like: > > > > A > content (distributed>) B > content (distributed>) C > D > > > > SelectorChecker::checkSelector can receive 'D' at first. It's right-most element. We cau re-use the current implementation of SelectorChecker (right-to-left, backwards) in this case. > > > > If we don't convert '::distributed()' to the relation, SelectorChecker::checkSelector will receive a 'content' (in 'A > content') as the first element which should be checked. > > I am afraid that makes an implementation of checking complicated. We have to retrieve the inner-most element, 'D', somehow, and start matching from 'D'. And we have to go backword from 'selector given as a parameter' to a insertion point somehow. > > > > I've abandonded this idea at early stage since converting it to the relation makes an implementation much simpler. > > I understand. We are introducing a whole new type of CSS functions into WebKit, the CSS pseudo element functions. Per discussion with Tab, this is just a first one (lucky us). Yay. Good luck to us :) > > >>> Source/WebCore/css/SelectorChecker.cpp:289 > >>> + return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally; > >> > >> Why do you store the element, when all you need is its scope? > > > > This is a due to a historical reason in this patch. As FIXME says, this part is WIP. I have to fix this expression, line 289. > > To check precisely, we need an element instead of treeScope here. I have not implemented that yet. > > > > For example, > > > > <shadow root> > > <div class='A' > > <div class='B'> > > <content></content> > > </div> > > </div> > > <div id='C'> > > <div id='D'> > > <style scoped> > > .A .B content distributed::(*) { color: green } > > </style> > > </div> > > </div> > > > > I've not tried, but I guess this style rule matches wrongly. Since in this expression, x > > I see. You really need to expand on this. Also, we should have a type that's different from ContainerNode to denote a scoping element. Even if it's a typedef. I've fixed this, though it's still ContainerNode. > > > Source/WebCore/css/StyleResolver.cpp:838 > > + collectMatchingRules(it->value.get(), result.ranges.firstAuthorRule, result.ranges.lastAuthorRule, options); > > Ideally, the data structure that deals with ::distributed rules should be completely factored out of the StyleResolver and its logic encapsulated. This collectMatchingRules call is what makes it difficult. > > I am wondering if it's time to decouple collecting rules for an element into a separate class from StyleResolver, and then allow passing around its instance to collect rules. This should enable creating ::distributed rules class. Let me try what you are suggesting. I've never tried that.
Thank you for the review again. (In reply to comment #61) > (From update of attachment 181830 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181830&action=review > > >> Source/WebCore/ChangeLog:13 > >> + A '::distributed()' pseudo element is the first CSS rule which > > > > Great explanation of why this is special. > > It's also the first-ever pseudo element function in WebKit. > > >> Source/WebCore/css/CSSParser.cpp:10608 > >> +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers) > > > > Would it be less error-prone to leave this returning void and make it take CSSParserSelector**? > > > > Perhaps this should be renamed, for example > > > > rewriteSpecifiersWtihElementName > > It seems to me this whole bit of machinery needs to be first cleaned up and named according to purpose. Stuffing more stuff in it just looks bad. > > > Source/WebCore/css/SelectorChecker.h:79 > > + const ContainerNode* scopeOfLastElement; > > I am still unhappy about the name. If I have to think about what this means, imagine what a person who didn't write the Shadow DOM spec would. We must do better. Okay. Let me think a better name. That's my bad.
Comment on attachment 182063 [details] iteration. A lot of refinements. Attachment 182063 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15795079 New failing tests: scrollbars/scrollbar-orientation.html scrollbars/disabled-scrollbar.html inspector/styles/region-style-crash.html fast/forms/textarea-metrics.html scrollbars/scrollbar-buttons.html scrollbars/scrollbars-on-positioned-content.html scrollbars/overflow-scrollbar-combinations.html scrollbars/basic-scrollbar.html scrollbars/scrollbar-drag-thumb-with-large-content.html
Created attachment 182077 [details] use enum CrossBoundary instead of scopeOfLastElement. Reuse scope.
I've removed the usage of scopeOfLastElement. Instead of it, I've decided to use enum, CrossBoundary. We can use an existing scope parameter to remember *scopeOfLastElement* since they are never mutually used. (In reply to comment #67) > Created an attachment (id=182077) [details] > use enum CrossBoundary instead of scopeOfLastElement. Reuse scope.
Created attachment 182079 [details] Remove DISTRIBUTED from RenderStyleConstants.h
(In reply to comment #64) > > > > > Source/WebCore/css/StyleResolver.cpp:838 > > > + collectMatchingRules(it->value.get(), result.ranges.firstAuthorRule, result.ranges.lastAuthorRule, options); > > > > Ideally, the data structure that deals with ::distributed rules should be completely factored out of the StyleResolver and its logic encapsulated. This collectMatchingRules call is what makes it difficult. > > > > I am wondering if it's time to decouple collecting rules for an element into a separate class from StyleResolver, and then allow passing around its instance to collect rules. This should enable creating ::distributed rules class. > > Let me try what you are suggesting. I've never tried that. I've tried that, but it turned out that it requires non-trivial refactoring. I think that should be done as another fix from a wider perspective. Maybe soon after this patch is landed?
(In reply to comment #70) > I've tried that, but it turned out that it requires non-trivial refactoring. I think that should be done as another fix from a wider perspective. Maybe soon after this patch is landed? There's no great hurry to land this patch. Let's do this right.
I'm okay to land this patch after refactoring. I think It depends on the urgency of the need of this feature for Web developers. (In reply to comment #70) > (In reply to comment #64) > > > > > > > > Source/WebCore/css/StyleResolver.cpp:838 > > > > + collectMatchingRules(it->value.get(), result.ranges.firstAuthorRule, result.ranges.lastAuthorRule, options); > > > > > > Ideally, the data structure that deals with ::distributed rules should be completely factored out of the StyleResolver and its logic encapsulated. This collectMatchingRules call is what makes it difficult. > > > > > > I am wondering if it's time to decouple collecting rules for an element into a separate class from StyleResolver, and then allow passing around its instance to collect rules. This should enable creating ::distributed rules class. > > > > Let me try what you are suggesting. I've never tried that. > > I've tried that, but it turned out that it requires non-trivial refactoring. I think that should be done as another fix from a wider perspective. Maybe soon after this patch is landed?
A). One benefit of landing this before refactoring is that it forces those who will refactor StyleResolver to consider how these cross-boundary CSS rules should be implemented in their refactoring process. B). Of course, landing this before refactoring will increase a maximum value of current StyleResolver's spaghetti a bit. I don't have a strong opinion, but A is worth than B, isn't it? (In reply to comment #72) > I'm okay to land this patch after refactoring. I think It depends on the urgency of the need of this feature for Web developers. > > (In reply to comment #70) > > (In reply to comment #64) > > > > > > > > > > > Source/WebCore/css/StyleResolver.cpp:838 > > > > > + collectMatchingRules(it->value.get(), result.ranges.firstAuthorRule, result.ranges.lastAuthorRule, options); > > > > > > > > Ideally, the data structure that deals with ::distributed rules should be completely factored out of the StyleResolver and its logic encapsulated. This collectMatchingRules call is what makes it difficult. > > > > > > > > I am wondering if it's time to decouple collecting rules for an element into a separate class from StyleResolver, and then allow passing around its instance to collect rules. This should enable creating ::distributed rules class. > > > > > > Let me try what you are suggesting. I've never tried that. > > > > I've tried that, but it turned out that it requires non-trivial refactoring. I think that should be done as another fix from a wider perspective. Maybe soon after this patch is landed?
(In reply to comment #73) > A). One benefit of landing this before refactoring is that it forces those who will refactor StyleResolver to consider how these cross-boundary CSS rules should be implemented in their refactoring process. That's true, but this is a classic paying back the technical debt scenario. You swipe the card, and then some time later "those who will refactor StyleResolver" will have to put up the cash. What I am suggesting is that we treat StyleResolver refactoring as a yak standing in font of us. And there's no way around shaving it.
Okay. I understand it. But I'd like to note that I meant that landing this before refactoring will be beneficial for us, who will refactor, instead of technical debt. We can get benefits from landed layout tests written for '::distributed()' in refactoring process. That will give us a confidence in refactoring process. I think how to implement crossing shadow boundary rules should be considered altogether in our refactoring process. (In reply to comment #74) > (In reply to comment #73) > > A). One benefit of landing this before refactoring is that it forces those who will refactor StyleResolver to consider how these cross-boundary CSS rules should be implemented in their refactoring process. > > That's true, but this is a classic paying back the technical debt scenario. You swipe the card, and then some time later "those who will refactor StyleResolver" will have to put up the cash. > > What I am suggesting is that we treat StyleResolver refactoring as a yak standing in font of us. And there's no way around shaving it.
(In reply to comment #75) > We can get benefits from landed layout tests written for '::distributed()' in refactoring process. That will give us a confidence in refactoring process. I think how to implement crossing shadow boundary rules should be considered altogether in our refactoring process. That is actually a pretty good point.
Created attachment 182283 [details] Refactor StyleResolver so that a class ShadowDistributedRules can be separataed.
I've done a small refactoring for StyleResolver so that we can have a ShadowDistributedRules as a separated class from StyleResolver. - Introduces a MatchRequest class, which replaces a MatchOptions. - Let ShadowDistributedRules return a set of MatchRequest so that we don't have to call StyleResolver::collectMatchRules() from there. WDYT? (In reply to comment #77) > Created an attachment (id=182283) [details] > Refactor StyleResolver so that a class ShadowDistributedRules can be separataed.
Created attachment 182293 [details] Update a ChangeLog
Comment on attachment 182293 [details] Update a ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=182293&action=review Now we're cooking with gas! This looks interesting. It seems the first yak to shave is to introduce MatchRequest. > Source/WebCore/css/StyleResolver.cpp:262 > +void ShadowDistributedRules::addShadowDistributedRule(StyleRule* rule, size_t selectorIndex, ContainerNode* scope, AddRuleFlags addRuleFlags) addRule > Source/WebCore/css/StyleResolver.cpp:479 > +void StyleResolver::addShadowDistributedRule(StyleRule* rule, size_t selectorIndex, ContainerNode* scope, AddRuleFlags addRuleFlags) > +{ > + m_shadowDistributedRules.addShadowDistributedRule(rule, selectorIndex, scope, addRuleFlags); > +} This is not needed anymore. RuleSet can add directly: styleResolver.shadowDistributedRules().addRule(...) > Source/WebCore/css/StyleResolver.cpp:850 > + m_shadowDistributedRules.collectMatchRequests(includeEmptyRules, matchRequests); > + for (size_t i = 0; i < matchRequests.size(); ++i) > + collectMatchingRules(matchRequests[i], result.ranges.firstAuthorRule, result.ranges.lastAuthorRule); This is where stuff falls apart somewhat. If there was a RuleCollector class factored out of StyleResolver, you could just give it to ShadowDistributedRules, so that it could use it directly, instead of queueing up requests. > Source/WebCore/css/StyleResolver.h:134 > +class MatchRequest { MatchRequest looks like the right data structure here. > Source/WebCore/css/StyleResolver.h:145 > + MatchRequest updated(RuleSet* newRuleSet) const { return MatchRequest(newRuleSet, scope, includeEmptyRules, crossBoundary); } This sounds like a copy constructor-style thing. "updated" is awkward.
Created attachment 182702 [details] iter
Comment on attachment 182293 [details] Update a ChangeLog Thank you for the review. View in context: https://bugs.webkit.org/attachment.cgi?id=182293&action=review >> Source/WebCore/css/StyleResolver.cpp:262 >> +void ShadowDistributedRules::addShadowDistributedRule(StyleRule* rule, size_t selectorIndex, ContainerNode* scope, AddRuleFlags addRuleFlags) > > addRule Done. >> Source/WebCore/css/StyleResolver.cpp:479 >> +} > > This is not needed anymore. RuleSet can add directly: styleResolver.shadowDistributedRules().addRule(...) Okay. It's like a WebKit style. Done. >> Source/WebCore/css/StyleResolver.cpp:850 >> + collectMatchingRules(matchRequests[i], result.ranges.firstAuthorRule, result.ranges.lastAuthorRule); > > This is where stuff falls apart somewhat. If there was a RuleCollector class factored out of StyleResolver, you could just give it to ShadowDistributedRules, so that it could use it directly, instead of queueing up requests. Actually I've tried that before sending the patch and decided not to include that in the patch :). It requires non-trivial refactoring since collectMatchingXXX families depend on a lot of member variables in StyleResolver and these variables seems to be used in other function in the StyleResolver. Let me retry that. I am aware that it is worth trying. >> Source/WebCore/css/StyleResolver.h:145 >> + MatchRequest updated(RuleSet* newRuleSet) const { return MatchRequest(newRuleSet, scope, includeEmptyRules, crossBoundary); } > > This sounds like a copy constructor-style thing. "updated" is awkward. Okay. I've renamed it to 'cloneWith'.
Comment on attachment 182702 [details] iter Attachment 182702 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15875825
Comment on attachment 182702 [details] iter Attachment 182702 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15845814
Comment on attachment 182702 [details] iter Attachment 182702 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15867767
Before proceeding further refactoring for StyleResolver, I am feeling that this patch might become big. So let me separate this patch into small patches, one of which only introduces MatchRequest in another bug. (In reply to comment #82) > (From update of attachment 182293 [details]) > Thank you for the review. > > View in context: https://bugs.webkit.org/attachment.cgi?id=182293&action=review > > >> Source/WebCore/css/StyleResolver.cpp:262 > >> +void ShadowDistributedRules::addShadowDistributedRule(StyleRule* rule, size_t selectorIndex, ContainerNode* scope, AddRuleFlags addRuleFlags) > > > > addRule > > Done. > > >> Source/WebCore/css/StyleResolver.cpp:479 > >> +} > > > > This is not needed anymore. RuleSet can add directly: styleResolver.shadowDistributedRules().addRule(...) > > Okay. It's like a WebKit style. Done. > > >> Source/WebCore/css/StyleResolver.cpp:850 > >> + collectMatchingRules(matchRequests[i], result.ranges.firstAuthorRule, result.ranges.lastAuthorRule); > > > > This is where stuff falls apart somewhat. If there was a RuleCollector class factored out of StyleResolver, you could just give it to ShadowDistributedRules, so that it could use it directly, instead of queueing up requests. > > Actually I've tried that before sending the patch and decided not to include that in the patch :). It requires non-trivial refactoring since collectMatchingXXX families depend on a lot of member variables in StyleResolver and these variables seems to be used in other function in the StyleResolver. > > Let me retry that. I am aware that it is worth trying. > > >> Source/WebCore/css/StyleResolver.h:145 > >> + MatchRequest updated(RuleSet* newRuleSet) const { return MatchRequest(newRuleSet, scope, includeEmptyRules, crossBoundary); } > > > > This sounds like a copy constructor-style thing. "updated" is awkward. > > Okay. I've renamed it to 'cloneWith'.
Comment on attachment 182702 [details] iter Attachment 182702 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15875829
Filed as a https://bugs.webkit.org/show_bug.cgi?id=106879. (In reply to comment #86) > Before proceeding further refactoring for StyleResolver, I am feeling that this patch might become big. > > So let me separate this patch into small patches, one of which only introduces MatchRequest in another bug. > > (In reply to comment #82) > > (From update of attachment 182293 [details] [details]) > > Thank you for the review. > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=182293&action=review > > > > >> Source/WebCore/css/StyleResolver.cpp:262 > > >> +void ShadowDistributedRules::addShadowDistributedRule(StyleRule* rule, size_t selectorIndex, ContainerNode* scope, AddRuleFlags addRuleFlags) > > > > > > addRule > > > > Done. > > > > >> Source/WebCore/css/StyleResolver.cpp:479 > > >> +} > > > > > > This is not needed anymore. RuleSet can add directly: styleResolver.shadowDistributedRules().addRule(...) > > > > Okay. It's like a WebKit style. Done. > > > > >> Source/WebCore/css/StyleResolver.cpp:850 > > >> + collectMatchingRules(matchRequests[i], result.ranges.firstAuthorRule, result.ranges.lastAuthorRule); > > > > > > This is where stuff falls apart somewhat. If there was a RuleCollector class factored out of StyleResolver, you could just give it to ShadowDistributedRules, so that it could use it directly, instead of queueing up requests. > > > > Actually I've tried that before sending the patch and decided not to include that in the patch :). It requires non-trivial refactoring since collectMatchingXXX families depend on a lot of member variables in StyleResolver and these variables seems to be used in other function in the StyleResolver. > > > > Let me retry that. I am aware that it is worth trying. > > > > >> Source/WebCore/css/StyleResolver.h:145 > > >> + MatchRequest updated(RuleSet* newRuleSet) const { return MatchRequest(newRuleSet, scope, includeEmptyRules, crossBoundary); } > > > > > > This sounds like a copy constructor-style thing. "updated" is awkward. > > > > Okay. I've renamed it to 'cloneWith'.
Comment on attachment 182702 [details] iter removing review flag, since this is just an iteration.
Created attachment 182936 [details] Rebased on a factored patch.
Comment on attachment 182936 [details] Rebased on a factored patch. View in context: https://bugs.webkit.org/attachment.cgi?id=182936&action=review > Source/WebCore/ChangeLog:11 > + # FIXME: Update URL after the spec is updated. Assuming this will be fixed when the spec is updated? > Source/WebCore/ChangeLog:35 > + pseudo element which takes parameter. It's also the first CSS parameter => a parameter > Source/WebCore/ChangeLog:37 > + tree of its shadow host. That means a '::distributed()' pseudo What about @host? It also crosses the boundary. > Source/WebCore/ChangeLog:61 > + Excellent change description. Very clear and easy to understand. > Source/WebCore/css/SelectorChecker.cpp:286 > + if (context.crossBoundary == CrossesBoundary) { Both crossBoundary and CrossesBoundary sound like booleans; maybe the field should be behaviorAtBoundary, or even behaviorAtParentScopeBoundary to be really specific. I guess this naming was set in an earlier patch. What was the review feedback? Would you consider revisiting the naming? > Source/WebCore/css/StyleResolver.h:226 > + Two blank lines? > Source/WebCore/html/shadow/InsertionPoint.h:164 > +inline void collectAllInsertionPoints(const Node* projectedNode, Vector<InsertionPoint*, 8>& results) Thank you for working on renaming this. Now I’m concerned that the meaning of this method is only clear when you see the parameter name projectedNode. If you just see a callsite: collectAllInsertionPoints(element, &result) I would guess it means something like collect all of the insertion points that are children or descendants of this node. What about collectInsertionPointsWhichProject(node, &result)? Not great. Do you have any ideas? Is there a precise difference in meaning between distributed (ie the child of a host) and projected (anything presented via an insertion point)? > LayoutTests/fast/dom/shadow/distributed-pseudo-element-expected.html:1 > +<!DOCTYPE html> Excellent use of reftests. > LayoutTests/fast/dom/shadow/distributed-pseudo-element-match-all.html:20 > + createDOM('div', {'class': 'hello'}, So A::distributed(B) actually has a kind of implicit descendant selector, A::distributed(* B)? > LayoutTests/fast/dom/shadow/distributed-pseudo-element.html:1 > +<!DOCTYPE html> Could we have tests with chained and nested ::distributed? eg A::distributed(B)::distributed(C) (what does it mean? What does it do?) A::distributed(B::distributed(C))
Comment on attachment 182936 [details] Rebased on a factored patch. Thank you for the review. Let me upload new patch soon, which also updates the ChangeLog. View in context: https://bugs.webkit.org/attachment.cgi?id=182936&action=review >> Source/WebCore/css/SelectorChecker.cpp:286 >> + if (context.crossBoundary == CrossesBoundary) { > > Both crossBoundary and CrossesBoundary sound like booleans; maybe the field should be behaviorAtBoundary, or even behaviorAtParentScopeBoundary to be really specific. > > I guess this naming was set in an earlier patch. What was the review feedback? Would you consider revisiting the naming? No review for this in an earlier patch. 'behaviorAtBoundary' sounds good. 'behaviorAtParentScopeBoundary' sounds unnatural in this context for me. I've renamed the field to 'behaviorAtBoundary' and enum itself to BehaviorAtBoundary. >> Source/WebCore/css/StyleResolver.h:226 >> + > > Two blank lines? Done. >> Source/WebCore/html/shadow/InsertionPoint.h:164 >> +inline void collectAllInsertionPoints(const Node* projectedNode, Vector<InsertionPoint*, 8>& results) > > Thank you for working on renaming this. > > Now I’m concerned that the meaning of this method is only clear when you see the parameter name projectedNode. If you just see a callsite: > > collectAllInsertionPoints(element, &result) > > I would guess it means something like collect all of the insertion points that are children or descendants of this node. > > What about collectInsertionPointsWhichProject(node, &result)? Not great. Do you have any ideas? > > Is there a precise difference in meaning between distributed (ie the child of a host) and projected (anything presented via an insertion point)? Okay. Let's rename it. The spec says: https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#reprojection > The effect of a node being distributed into more than one insertion point is called reprojection. So I've renamed it to the 'collectInsertionPointsWhereNodeIsDistributed(node, &results)'. We can say 'NodeIsDistributed' into more than one insertion point in this context. >> LayoutTests/fast/dom/shadow/distributed-pseudo-element-match-all.html:20 >> + createDOM('div', {'class': 'hello'}, > > So A::distributed(B) actually has a kind of implicit descendant selector, A::distributed(* B)? Yes. That's an intentional behavior. A distributed-pseudo-element-match-descendant.html tests that. >> LayoutTests/fast/dom/shadow/distributed-pseudo-element.html:1 >> +<!DOCTYPE html> > > Could we have tests with chained and nested ::distributed? eg > > A::distributed(B)::distributed(C) (what does it mean? What does it do?) > > A::distributed(B::distributed(C)) A nested case is already covered by a distributed-pseudo-element-nested.html. A *chained* case, A::distributed(B)::distributed(C), is not allowed according to the selector spec. http://dev.w3.org/csswg/selectors4/#pseudo-elements > A future version of this specification may allow multiple pseudo-elements per selector. So I'd like to leave the behavior in this case 'undefined' for a while.
Created attachment 183111 [details] Addressed. We need to update the URL in the ChangeLog once the spec is updated.
*** Bug 107654 has been marked as a duplicate of this bug. ***
I wonder how much smaller this patch has gotten in light of all the recent refactorings?
I don't expect this patch get any benefits from the recent refactorings. I guess there isn't any meaningful difference between landing this patch before and after refactoring. (In reply to comment #95) > I wonder how much smaller this patch has gotten in light of all the recent refactorings?
(In reply to comment #96) > I don't expect this patch get any benefits from the recent refactorings. > > I guess there isn't any meaningful difference between landing this patch before and after refactoring. Ok. Can you rebase and let me look at it?
Sure. Let me rebase. (In reply to comment #97) > (In reply to comment #96) > > I don't expect this patch get any benefits from the recent refactorings. > > > > I guess there isn't any meaningful difference between landing this patch before and after refactoring. > > Ok. Can you rebase and let me look at it?
Created attachment 187789 [details] Rebased.
Comment on attachment 187789 [details] Rebased. The rebase didn't work? All EWSes are purple.
Created attachment 188023 [details] Rabased again.
I rebased again. This rebase was not simple one since DocumentRuleSets had been introduced. ShadowDistributedRules have moved from StyleResolver to DocumentRuleSets. (In reply to comment #101) > Created an attachment (id=188023) [details] > Rabased again.
Comment on attachment 188023 [details] Rabased again. View in context: https://bugs.webkit.org/attachment.cgi?id=188023&action=review I've made peace with this patch. > Source/WebCore/ChangeLog:50 > + scope is 'D'. The scope is used to check whether the left-most Wait... isn't the scope [A's ShadowRoot] here? This is not a scoped style. > Source/WebCore/ChangeLog:75 > + * css/CSSGrammar.y.in: If you're not enumerating changes in each file, probably don't need this generated list. > Source/WebCore/css/SelectorChecker.cpp:288 > + return context.scope->contains(context.element) ? SelectorMatches : SelectorFailsLocally; Linear ancestor search, ugh. We can get away with TreeScope comparisons, except for cases where the scope is created by <style scoped>. We should consider optimizing this somehow. > Source/WebCore/css/StyleResolver.cpp:597 > + m_ruleSets.shadowDistributedRules().collectMatchRequests(includeEmptyRules, matchRequests); > + for (size_t i = 0; i < matchRequests.size(); ++i) > + collectMatchingRules(matchRequests[i], ruleRange); This is about the only place in the patch where I wish we could do better. In my crazy imagination, the collector of rules is decoupled from StyleResolver and could be passed to various actors, but I understand this is probably not feasible. > Source/WebCore/css/StyleResolver.h:139 > +class MatchRequest { This didn't need to move up in the file anymore. Probably would make the diff look better.
Comment on attachment 188023 [details] Rabased again. Thank you for the review. Let me land this patch after addressing the comments. Since StyleResolver has still been hot recently, it might be good timing to land this. View in context: https://bugs.webkit.org/attachment.cgi?id=188023&action=review > Source/WebCore/ChangeLog:12 > + http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#selecting-nodes-distributed-to-insertion-points Let me update the URL to the W3C's bug's one: https://www.w3.org/Bugs/Public/show_bug.cgi?id=19684 >> Source/WebCore/ChangeLog:50 >> + scope is 'D'. The scope is used to check whether the left-most > > Wait... isn't the scope [A's ShadowRoot] here? This is not a scoped style. Right. The example is not a scoped style. Fixed. >> Source/WebCore/ChangeLog:75 >> + * css/CSSGrammar.y.in: > > If you're not enumerating changes in each file, probably don't need this generated list. Since this patch is becoming stable, let me enumerate changes in each file briefly. >> Source/WebCore/css/SelectorChecker.cpp:288 >> + return context.scope->contains(context.element) ? SelectorMatches : SelectorFailsLocally; > > Linear ancestor search, ugh. We can get away with TreeScope comparisons, except for cases where the scope is created by <style scoped>. We should consider optimizing this somehow. Good point. I am sure we can improve this. Let me improve this in another bug since this code path is hit only when '::distributed' is used. >> Source/WebCore/css/StyleResolver.cpp:597 >> + collectMatchingRules(matchRequests[i], ruleRange); > > This is about the only place in the patch where I wish we could do better. In my crazy imagination, the collector of rules is decoupled from StyleResolver and could be passed to various actors, but I understand this is probably not feasible. Agreed. I felt the same thing. We can do better if we factor StyleResolver further. Let me think further how we can factor StyleResolver. >> Source/WebCore/css/StyleResolver.h:139 >> +class MatchRequest { > > This didn't need to move up in the file anymore. Probably would make the diff look better. Unfortunately, ShadowDistributedRules::collectMatchRequests() in DocumentRuleSets.h needs MatchRequest. Therefore, MatchRequest needs to be in a top level class in WebCore namespace. I don't think this is an ideal situation, though.
Created attachment 188267 [details] Patch for landing
Comment on attachment 188267 [details] Patch for landing Clearing flags on attachment: 188267 Committed r142855: <http://trac.webkit.org/changeset/142855>
All reviewed patches have been landed. Closing bug.