Bug 102919

Created attachment 175406 [details] repro

Created attachment 179464 [details] WIP

My patch is still "WIP", however I would like to ask feedbacks. I'm afraid that I'm on the wrong way to fix this issue. What I'm trying to do is: - custom pseudo in UA stylesheets --> should check whether elements in any treescope match or not. e.g. html.css: progress:-webkit-progress-value { ... } #shadow-root #shadow-root div::-webkit-progress-value <--- always apply above rule - custom pseudo in an author stylesheet --> should check whether elements in any child treescopes of document and in some treescopes whose parent treescopes have apply-author-styles true. If so, try to apply. Otherwise, ignore. e.g. document: <style> div::x-foo { ... } </style> #shadow-root div::x-foo <--- apply #shadow-root #shadow-root div::x-foo <--- not apply - custom pseudo in some stylesheet in a shadow dom tree --> for the given element, looking at enclosing shadow dom trees from the element's parent treescope: - an enclosing shadow dom tree has apply-author-styles false, stop. - check all custom pseudo styles in the current shadow dom tree, - walk up to an shadow dom tree which encloses the current one. e.g. #shadow-root <style> div::x-foo { ... } #shadow-root (apply-author-styles: true) #shadow-root div::x-foo <--- apply Best regards, Takashi Sakamoto

Comment on attachment 179464 [details] WIP Attachment 179464 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15353001 New failing tests: fast/dom/shadow/shadow-nested-pseudo-id.html

Comment on attachment 179464 [details] WIP Attachment 179464 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15343003 New failing tests: fast/dom/shadow/shadow-nested-pseudo-id.html

(In reply to comment #3) > My patch is still "WIP", however I would like to ask feedbacks. I'm afraid that I'm on the wrong way to fix this issue. > > What I'm trying to do is: > - custom pseudo in UA stylesheets > --> should check whether elements in any treescope match or not. > > e.g. > html.css: > progress:-webkit-progress-value { ... } > > #shadow-root > #shadow-root > div::-webkit-progress-value <--- always apply above rule I am assuming that by "div::-webkit-progress-value" you mean "div[pseudo=-webkit-progress-value]". Right? In this case, yes. (naturally, only if the host of the tree in which the div is located has tag name "progress") I apologize, I didn't have a chance to look at this over the weekend, but I'll attack this Monday! :) > > > - custom pseudo in an author stylesheet > --> should check whether elements in any child treescopes of document and in some treescopes whose parent treescopes have apply-author-styles true. If so, try to apply. Otherwise, ignore. > > e.g. > document: > <style> > div::x-foo { ... } > </style> > > #shadow-root > div::x-foo <--- apply > #shadow-root > #shadow-root > div::x-foo <--- not apply > > - custom pseudo in some stylesheet in a shadow dom tree > --> for the given element, looking at enclosing shadow dom trees from the element's parent treescope: > - an enclosing shadow dom tree has apply-author-styles false, stop. > - check all custom pseudo styles in the current shadow dom tree, > - walk up to an shadow dom tree which encloses the current one. > > e.g. > #shadow-root > <style> div::x-foo { ... } > #shadow-root (apply-author-styles: true) > #shadow-root > div::x-foo <--- apply > > Best regards, > Takashi Sakamoto

(In reply to comment #6) > (In reply to comment #3) > > My patch is still "WIP", however I would like to ask feedbacks. I'm afraid that I'm on the wrong way to fix this issue. > > > > What I'm trying to do is: > > - custom pseudo in UA stylesheets > > --> should check whether elements in any treescope match or not. > > > > e.g. > > html.css: > > progress:-webkit-progress-value { ... } > > > > #shadow-root > > #shadow-root > > div::-webkit-progress-value <--- always apply above rule > > I am assuming that by "div::-webkit-progress-value" you mean "div[pseudo=-webkit-progress-value]". Right? Yes, right. Sorry for writing ambiguous things. > In this case, yes. (naturally, only if the host of the tree in which the div is located has tag name "progress") > > I apologize, I didn't have a chance to look at this over the weekend, but I'll attack this Monday! :) Thank you! So I updated my ambiguous diagrams... > > - custom pseudo in an author stylesheet > > --> should check whether elements in any child treescopes of document and in some treescopes whose parent treescopes have apply-author-styles true. If so, try to apply. Otherwise, ignore. > > > > e.g. > > document: > > <style> > > div::x-foo { ... } > > </style> > > > > #shadow-root > > div::x-foo <--- apply > > #shadow-root > > #shadow-root > > div::x-foo <--- not apply #document: <style> div::x-foo { ... } </style> div #shadow-root div[pseudo=x-foo] <--- apply ... div #shadow-root div #shadow-root div[pseudo=x-foo] <--- not apply With apply-author-styles, #document: div #shadow-root <style> div::x-foo { ... } </style> ... div #shadow-root (apply-author-styles: true) div #shadow-root (apply-author-styles: false) div[pseudo=x-foo] <--- apply Best regards, Takashi Sakamoto

I missed the following case, reported by layout test: shadow-netsted-pseudo-id.html: p::x-shadow-child::x-nested-shadow-child { background-color: green; } So I think, we need scopeOfLastElement in SelectorChecker, introduced by hayato@ in https://bugs.webkit.org/show_bug.cgi?id=82169, #if ENABLE(SHADOW_DOM) if (context.scopeOfLastElement) { // FIXME: Check the scope more strictly. // Obtained from https://bugs.webkit.org/attachment.cgi?id=176892 // Should wait till the original bug is fixed. return context.scopeOfLastElement->treeScope() == context.element->\ treeScope() ? SelectorMatches : SelectorFailsLocally; } #endif I'm thinking of: - for author stylesheets (not scoped), set scopeOfLastElement document. So I can fix: New failing tests: fast/dom/shadow/shadow-nested-pseudo-id.html I will update the patch which uses hayato@'s code. Best regards, Takashi Sakamoto

Created attachment 179709 [details] Patch

Comment on attachment 179709 [details] Patch Attachment 179709 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15378623

Comment on attachment 179709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179709&action=review > Source/WebCore/css/StyleResolver.cpp:641 > +void StyleResolver::collectMatchingRulesForPseudo(RuleSet* rules, int& firstRuleIndex, int& lastRuleIndex, const MatchOptions& options) collectMatchingPseudoElementRules

Comment on attachment 179709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179709&action=review > Source/WebCore/css/StyleScopeResolver.cpp:161 > + applyAuthorStyles = false; > + // m_stackParentBoundsIndex + 1 doesn't match any items in m_stack. > + if (element->shadowPseudoId().isEmpty()) > + return m_stackParentBoundsIndex + 1; This seems like it could just be moved outside of this function. > Source/WebCore/css/StyleScopeResolver.cpp:167 > + for (; parent && !parent->isShadowRoot(); parent = parent->parentOrHostNode()) { } Why can't we just go parentTreeScope = m_stackParent->treeScope()? > Source/WebCore/css/StyleScopeResolver.cpp:174 > + return toShadowRoot(parent)->applyAuthorStyles() ? m_stackParentBoundsIndex : m_stackParentBoundsIndex - 1; So it's always either at the end of the stack or just preceding it?

Comment on attachment 179709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179709&action=review > Source/WebCore/css/StyleResolver.h:375 > + bool checkSelector(const RuleData&, const ContainerNode* scope, const ContainerNode* scopeOfLastElement); This is nasty :-\

Comment on attachment 179709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179709&action=review > Source/WebCore/css/SelectorChecker.cpp:481 > + return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally; Why are we doing this in checkSelector? I should know this way above (at the level of a rule) whether the tree scope will match. This is wrong.

Comment on attachment 179709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179709&action=review Thank you for reviewing. >> Source/WebCore/css/SelectorChecker.cpp:481 >> + return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally; > > Why are we doing this in checkSelector? I should know this way above (at the level of a rule) whether the tree scope will match. This is wrong. I wrote the document about this: https://docs.google.com/document/d/1yu-zVrNAO79JFEbnfjI20yrfZ428l3hqKf82k5CU69Q/edit >> Source/WebCore/css/StyleResolver.cpp:641 >> +void StyleResolver::collectMatchingRulesForPseudo(RuleSet* rules, int& firstRuleIndex, int& lastRuleIndex, const MatchOptions& options) > > collectMatchingPseudoElementRules Done. >> Source/WebCore/css/StyleResolver.h:375 >> + bool checkSelector(const RuleData&, const ContainerNode* scope, const ContainerNode* scopeOfLastElement); > > This is nasty :-\ I see. I checked to use "const MatchOptions&". >> Source/WebCore/css/StyleScopeResolver.cpp:161 >> + return m_stackParentBoundsIndex + 1; > > This seems like it could just be moved outside of this function. Done. >> Source/WebCore/css/StyleScopeResolver.cpp:167 >> + for (; parent && !parent->isShadowRoot(); parent = parent->parentOrHostNode()) { } > > Why can't we just go parentTreeScope = m_stackParent->treeScope()? Done. >> Source/WebCore/css/StyleScopeResolver.cpp:174 >> + return toShadowRoot(parent)->applyAuthorStyles() ? m_stackParentBoundsIndex : m_stackParentBoundsIndex - 1; > > So it's always either at the end of the stack or just preceding it? Yeah. m_stackParentBoundsIndex always have m_stackParent's shadow root's bounds index. So, - If m_stackParent's shadow root has apply-author-styles true, the parent tree scope of m_stackParent has the same bounds index. - Otherwise, since ++m_stackParentBoundsIndex when we find a shadow root whose apply-author-styles is false, m_stackParentBoundsIndex-1 is the same as the parent tree scope's bounds index. https://docs.google.com/document/d/1HRlJdq6kIFWKHzXL4uDSspsCmnHyiGxwwVbROpySrOg/edit

Created attachment 179881 [details] Patch

Comment on attachment 179881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179881&action=review > Source/WebCore/css/StyleScopeResolver.cpp:154 > +int StyleScopeResolver::findStyleBoundsOfParentTreeScope(const Element* element, bool& applyAuthorStyles) const You don't need element arg anymore.

(In reply to comment #15) > I wrote the document about this: > https://docs.google.com/document/d/1yu-zVrNAO79JFEbnfjI20yrfZ428l3hqKf82k5CU69Q/edit I read the doc. It seems that you've arrived at the same conclusion as I have (in solution 1) -- the number of pseudo-elements should be stored with StyleRule (since it's known at parsing and does not change for each style recalc), and certainly not re-determined for each checkSelector. That stored number of pseudo-elements is exactly the increment to determine the scope on which to match elements. Or am I still missing something? > > I see. I checked to use "const MatchOptions&". The reason why I thought it was nasty is because this does not belong in checking selector. This information is known well before we get to individual selector checking. We can discard an entire rule simply based on the number of pseudo-elements it contains.

Created attachment 180078 [details] Patch

(In reply to comment #18) > (In reply to comment #15) > > > I wrote the document about this: > > https://docs.google.com/document/d/1yu-zVrNAO79JFEbnfjI20yrfZ428l3hqKf82k5CU69Q/edit > > I read the doc. It seems that you've arrived at the same conclusion as I have (in solution 1) -- the number of pseudo-elements should be stored with StyleRule (since it's known at parsing and does not change for each style recalc), and certainly not re-determined for each checkSelector. > > That stored number of pseudo-elements is exactly the increment to determine the scope on which to match elements. > > Or am I still missing something? Yeah. For fixing only this issue, I agree that we don't need solution2 (=lastScopeOfElement). I forgot to add the reason why we need solution2, not solution1 to the document. The idea, lastScopeOfElement comes from https://bugs.webkit.org/show_bug.cgi?id=82169, "implement /select/ reference combinator". To implement the function, we will need solution2, because number of /select/ doesn't provide any information about how many treescopes we have to check. So I think, it would be better to re-use lastScopeOfElement for fixing this issue. If this reason is weak, I will add 82169 to "Depends on" and will wait till hayato-san's patch is landed. > > I see. I checked to use "const MatchOptions&". > > The reason why I thought it was nasty is because this does not belong in checking selector. This information is known well before we get to individual selector checking. We can discard an entire rule simply based on the number of pseudo-elements it contains. The changes about checkSelector(...) also comes from https://bugs.webkit.org/show_bug.cgi?id=82169. Best regards, Takashi Sakamoto

Comment on attachment 179881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179881&action=review >> Source/WebCore/css/StyleScopeResolver.cpp:154 >> +int StyleScopeResolver::findStyleBoundsOfParentTreeScope(const Element* element, bool& applyAuthorStyles) const > > You don't need element arg anymore. Thank you. I missed this.

(In reply to comment #20) > (In reply to comment #18) > > (In reply to comment #15) > > > > > I wrote the document about this: > > > https://docs.google.com/document/d/1yu-zVrNAO79JFEbnfjI20yrfZ428l3hqKf82k5CU69Q/edit > > > > I read the doc. It seems that you've arrived at the same conclusion as I have (in solution 1) -- the number of pseudo-elements should be stored with StyleRule (since it's known at parsing and does not change for each style recalc), and certainly not re-determined for each checkSelector. > > > > That stored number of pseudo-elements is exactly the increment to determine the scope on which to match elements. > > > > Or am I still missing something? > > Yeah. For fixing only this issue, I agree that we don't need solution2 (=lastScopeOfElement). I forgot to add the reason why we need solution2, not solution1 to the document. > > The idea, lastScopeOfElement comes from https://bugs.webkit.org/show_bug.cgi?id=82169, "implement /select/ reference combinator". To implement the function, we will need solution2, because number of /select/ doesn't provide any information about how many treescopes we have to check. > > So I think, it would be better to re-use lastScopeOfElement for fixing this issue. If this reason is weak, I will add 82169 to "Depends on" and will wait till hayato-san's patch is landed. That patch won't ever land, since the syntax is changing. Also, I don't think this is the right approach anyway. Let's not do this. Please.

(In reply to comment #22) > (In reply to comment #20) > > (In reply to comment #18) > > > (In reply to comment #15) > > > > > > > I wrote the document about this: > > > > https://docs.google.com/document/d/1yu-zVrNAO79JFEbnfjI20yrfZ428l3hqKf82k5CU69Q/edit > > > > > > I read the doc. It seems that you've arrived at the same conclusion as I have (in solution 1) -- the number of pseudo-elements should be stored with StyleRule (since it's known at parsing and does not change for each style recalc), and certainly not re-determined for each checkSelector. > > > > > > That stored number of pseudo-elements is exactly the increment to determine the scope on which to match elements. > > > > > > Or am I still missing something? > > > > Yeah. For fixing only this issue, I agree that we don't need solution2 (=lastScopeOfElement). I forgot to add the reason why we need solution2, not solution1 to the document. > > > > The idea, lastScopeOfElement comes from https://bugs.webkit.org/show_bug.cgi?id=82169, "implement /select/ reference combinator". To implement the function, we will need solution2, because number of /select/ doesn't provide any information about how many treescopes we have to check. > > > > So I think, it would be better to re-use lastScopeOfElement for fixing this issue. If this reason is weak, I will add 82169 to "Depends on" and will wait till hayato-san's patch is landed. > > That patch won't ever land, since the syntax is changing. Also, I don't think this is the right approach anyway. Let's not do this. Please. I know. I'm not sure whether hayato-san will re-use the existing bug or file a new bug for the new syntax. However his new patch for the new syntax still requires lastScopeOfElement as far as I know. Best regards, Takashi Sakamoto

Because dglazkov prefers the counting solution, and because bug 82169 is blocked on the CSS4 selectors spec, let’s use the counting approach to unblock this work. Let’s abandon "solution 2".

(In reply to comment #24) > Because dglazkov prefers the counting solution, and because bug 82169 is blocked on the CSS4 selectors spec, let’s use the counting approach to unblock this work. Let’s abandon "solution 2". However if we have some css selector which has both :distributed and x-???, "solution 1"(i.e. couting) doesn't work. After :distributed patch is landed, we will have to remove "solution 1" code and to re-add "solution 2". Is this good approach? If so, I will implement "solution 1" and wait for ":distributed". Best regards, Takashi Sakamoto

Comment on attachment 180078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180078&action=review Some nits inline. I am having a hard time following the code change because I am not very familiar with selector checking. > LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:11 > +var host = document.getElementById("host"); I think it is neater to use ' for JavaScript string literals. That way you use the same quotes (") for attributes both in markup, as on line 4, and markup in JavaScript string literals, as on line 13. Line 31, etc. will also be much cleaner if you use '. > LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:30 > +debug("Case1: A pseudo style is in the same treescope as div with x-foo pseudo."); It would be typical to put a space after "Case". > LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:67 > +</html> What about successfullyParsed and including js-test-post.js?

Comment on attachment 180078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180078&action=review > LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:1 > +<!doctype html> You should add tests with multiple shadow roots and reprojection through <shadow> and <content> as well as nesting.

(In reply to comment #25) > … if we have some css selector which has both :distributed and x-???, "solution 1"(i.e. couting) doesn't work. My intuition is that CSS selectors should work compositionally, and anywhere they don’t is a bad sign! The reason you think counting does not work is because given a rule R: A::distributed(::x-foo) { … } The scope associated with R (A’s scope) has no countable relationship to the scope of ::x-foo because of reprojection. However can’t you simply treat the subselector ::distributed(…) is matching as a separate selector? Set the count to zero, and interpret relative to the scope of the current element (ie wherever ::distributed tried to start matching?)

(In reply to comment #28) > (In reply to comment #25) > > … if we have some css selector which has both :distributed and x-???, "solution 1"(i.e. couting) doesn't work. > > My intuition is that CSS selectors should work compositionally, and anywhere they don’t is a bad sign! > > The reason you think counting does not work is because given a rule R: > > A::distributed(::x-foo) { … } > > The scope associated with R (A’s scope) has no countable relationship to the scope of ::x-foo because of reprojection. > > However can’t you simply treat the subselector ::distributed(…) is matching as a separate selector? Set the count to zero, and interpret relative to the scope of the current element (ie wherever ::distributed tried to start matching?) I found that we can ignore all custom pseudo in ::distributed. So talking about the above example, we don't need to check "count". However, if we have C::x-foobar > A::x-bar::distributed(B::x-foo) we have to count all custom pseudo outside of ::distributed, e.g. 2 (x-foobar and x-bar), in CSSParser. ... however after ::distributed patch is landed, we will have lastScopeOfElement. I don't know any good reason why we have to use both "count" and "lastScopeOfElement" at the same time or why we have to select "count" solution or "lastScopeOfElement" solution in StyleResolver (or somewhere). Just we can set lastScopeOfElement as the scope of matched rules. Best regards, Takashi Sakamoto Since all selector matches from right to left order, firstly we have to check whether the given element matches ::x-foo or not. Next, we will see ::distributed.

(In reply to comment #29) I'm sorry. I forgot to delete the following sentence. Please ignore this. > Since all selector matches from right to left order, firstly we have to check whether the given element matches ::x-foo or not. Next, we will see ::distributed.

Created attachment 181890 [details] Patch

Comment on attachment 180078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180078&action=review Thank you for reviewing. >> LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:1 >> +<!doctype html> > > You should add tests with multiple shadow roots and reprojection through <shadow> and <content> as well as nesting. I see. Done. >> LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:11 >> +var host = document.getElementById("host"); > > I think it is neater to use ' for JavaScript string literals. That way you use the same quotes (") for attributes both in markup, as on line 4, and markup in JavaScript string literals, as on line 13. > > Line 31, etc. will also be much cleaner if you use '. Done. >> LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:30 >> +debug("Case1: A pseudo style is in the same treescope as div with x-foo pseudo."); > > It would be typical to put a space after "Case". Done. >> LayoutTests/fast/dom/shadow/pseudo-style-in-shadow.html:67 >> +</html> > > What about successfullyParsed and including js-test-post.js? Done.

Comment on attachment 181890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181890&action=review The vision of the actual architecture eludes me, but all this new code makes me wonder why we make a distinction between scoped styles and document styles. After all, they are simply styles scoped at the document. I think this is where the path to the right architecture lies. > Source/WebCore/css/StyleResolver.cpp:752 > + bool elementHasShadowPseudo = !m_element->shadowPseudoId().isEmpty(); This is a bad smell. We do this earlier in collectMatchingPseudoElementRules, and this logic looks like duplication, but not quite.

Comment on attachment 181890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181890&action=review >> Source/WebCore/css/StyleResolver.cpp:752 >> + bool elementHasShadowPseudo = !m_element->shadowPseudoId().isEmpty(); > > This is a bad smell. We do this earlier in collectMatchingPseudoElementRules, and this logic looks like duplication, but not quite. So... do you mean we have to always invoke collectMatchingPseudoElementRules even if a given element has no pseudo id?

Comment on attachment 181890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181890&action=review >>> Source/WebCore/css/StyleResolver.cpp:752 >>> + bool elementHasShadowPseudo = !m_element->shadowPseudoId().isEmpty(); >> >> This is a bad smell. We do this earlier in collectMatchingPseudoElementRules, and this logic looks like duplication, but not quite. > > So... do you mean we have to always invoke collectMatchingPseudoElementRules even if a given element has no pseudo id? I mean, if possible, I would like to do in the following way: if (hasPseudoId) { // loop with collectMatchingPseudoElementRules for ( ... ) { // ... collectMatchingPseudoElementRuels(...); ... if (documnetScope && ... ) { collectMatchingRules(...); ... } } } else { // loop without collectMatchingPseudoElementRules for ( ... ) { // This is the same as the code before applying this patch. } }

Created attachment 182106 [details] WIP

I tried to make bad layout tests, which look like fuzzer, better. Best regards, Takashi Sakamoto

This bug is no longer relevant since the old shadow DOM implementation has been replaced by a new one.