Bug 89061 – applyAuthorStyles makes rules declared in all enclosing shadow dom subtrees applicable.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Takashi Sakamoto]

Shadow DOM spec says:
If TREE has apply-author-styles flag set and RULE is declared in an enclosing shadow DOM subtree
    Let TOP be this tree
    If each shadow DOM subtree that both encloses TREE and is enclosed by TOP has apply-author-styles set, the RULE is applicable in TREE.

(c.f. http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#styles )

However, currently apply-author-styles changes whether rules declared in document is applicable in a shadow dom subtree or not.

[Takashi Sakamoto]

(In reply to comment #0)

> However, currently apply-author-styles changes whether rules declared in document is applicable in a shadow dom subtree or not.

This is not correct. Current implementation applies rules declared in enclosing shadow dom subtrees independent of apply-author-styles' chain. I mean:

- There are 3 shadow dom subtrees, A, B and C: shadow dom tree A ----> shadow dom tree B ---> shadow dom tree C. 

- A encloses B.
- B encloses C.
- C's apply-author-styles is set.
- A and B's apply-author-styles is not set.

All rules declared in A and B are applicable in C. 

But, in this case, rules declared in B should be applicable in C. Rules declared in A should not be applicable.

[Takashi Sakamoto]

Created attachment 147505 [details]
Patch

[Takashi Sakamoto]

Created attachment 147506 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 147506 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:945
> +        for (const ContainerNode* scope = m_element; scopedIndex > authorRuleIndex && scope; scope = scope->parentOrHostNode()) {

in this context, authorRuleIndex means first shadow DOM scope. It sounds like moving this code to a static non-class function would serve well to better explain this.

Why are we going up using parentOrHostNode?

> Source/WebCore/css/StyleResolver.cpp:948
> +            for (; scopedIndex > authorRuleIndex; --scopedIndex)
> +                if (m_scopeStack[scopedIndex - 1].m_scope != scope)
> +                    break;

Can you explain what this code is doing? It seems to me that just going up the scope stack should be enough?

> LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html:32
> +    document.body.offsetLeft;

I am worried that you keep using this to trigger style recalc. Shouldn't setting applyAuthorStyles trigger the recalc?

[Takashi Sakamoto]

(In reply to comment #4)
> (From update of attachment 147506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147506&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:945
> > +        for (const ContainerNode* scope = m_element; scopedIndex > authorRuleIndex && scope; scope = scope->parentOrHostNode()) {
> 
> in this context, authorRuleIndex means first shadow DOM scope. It sounds like moving this code to a static non-class function would serve well to better explain this.
> 
> Why are we going up using parentOrHostNode?
> 
> > Source/WebCore/css/StyleResolver.cpp:948
> > +            for (; scopedIndex > authorRuleIndex; --scopedIndex)
> > +                if (m_scopeStack[scopedIndex - 1].m_scope != scope)
> > +                    break;
> 
> Can you explain what this code is doing? It seems to me that just going up the scope stack should be enough?

Because m_scopeStack doesn't have all ancestor nodes of m_element. m_scopeStack has information like (node, ruleset) only if the node has some rule. For examples,

Document
  A
  |
  B ---- ShadowRoot1
                 |
                 C---------ShadowRoot2 (applyAuthorStyles=true)
                                           |
                                          D---------- ShadowRoot3 (applyAuthorStyles=true)
                                                                      |
                                                                     E

and suppose that ShadowRoot1 has some <style> and that  C, D, E, ShadowRoot2, and ShadowRoot3 have no <style>.

When matchScopedAuthorRules is invoked and m_element is E, m_scopeStack looks like:
[(ShadowRoot1, some RuleSet)]
ShadowRoot2, D, ShadowRoot3, E are not stored in m_scopeStack.

So, by going up using parentOrHostNode(), generate the following sequence:
[E, ShadowRoot3, D, ShadowRoot2, C, ShadowRoot1]
and compare with m_scopeStack:

E: not in m_scopeStack, skip.
ShadowRoot3: not in m_scopeStack, skip. Since ShadowRoot3 has applyAuthorStyles set, continue.
D: not in m_scopeStack, skip.
ShadowRoot2: not in m_scopeStack, skip. Since ShadowRoot2 has applyAuthorStyles set, continue.
C: not in m_scopeStack, skip.
ShadowRoot1: in m_scopeStack. Try to apply.

It is difficult to find whether ShadowRoot2 and ShadowRoot3 have applyAuthorStyle set or not by just scanning m_scopeStack. 
This is also the reason why going up using parentOrHostNode().

However, I got an advice from Morita-san. I will change m_scopeStack to hold some information about applyAuthorStyles and update the patch.

> > LayoutTests/fast/css/style-scoped/style-scoped-apply-author-styles.html:32
> > +    document.body.offsetLeft;
> 
> I am worried that you keep using this to trigger style recalc. Shouldn't setting applyAuthorStyles trigger the recalc?

Yes, I'm using this to trigger style recalc… 
However, applyAuthorStyles should trigger recalc and the test should work without any "document.body.offsetLeft" for style realc.
I will remove these document.body.offsetLeft.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 147752 [details]
Patch

[Takashi Sakamoto]

> I am worried that you keep using this to trigger style recalc. Shouldn't setting applyAuthorStyles trigger the recalc?

I asked Morita-san about how to use document.body.offsetLeft. So I agree that we don't need any document.body.offsetLeft in this layout test. 
This test should pass without any "document.body.offsetLeft".

Best regards,
Takashi Sakamoto

[Dimitri Glazkov (Google)]

(In reply to comment #5)

> However, I got an advice from Morita-san. I will change m_scopeStack to hold some information about applyAuthorStyles and update the patch.

Right -- it seems like this is a better solution.

[Dimitri Glazkov (Google)]

Comment on attachment 147752 [details]
Patch

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

getting really close now.

Thank you for the explanation!

> Source/WebCore/css/StyleResolver.cpp:952
> +    if (m_element->isInShadowTree() && !m_scopeStack.isEmpty()) {

Looks like early return would improve readability here.

> Source/WebCore/css/StyleResolver.h:534
> +        ScopeStackFrame() : m_scope(0), m_scopeGroup(0), m_ruleSet(0) { }

It's a scope index, right? Not scope group. What you're doing here is storing the actual index of the stack, creating a virtual sparse array.

[Takashi Sakamoto]

Created attachment 148042 [details]
Patch

[Takashi Sakamoto]

Thank you for reviewing.

(In reply to comment #9)
> (From update of attachment 147752 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147752&action=review
> 
> getting really close now.
> 
> Thank you for the explanation!
> 
> > Source/WebCore/css/StyleResolver.cpp:952
> > +    if (m_element->isInShadowTree() && !m_scopeStack.isEmpty()) {
> 
> Looks like early return would improve readability here.

I see. Done.

> > Source/WebCore/css/StyleResolver.h:534
> > +        ScopeStackFrame() : m_scope(0), m_scopeGroup(0), m_ruleSet(0) { }
> 
> It's a scope index, right? Not scope group. What you're doing here is storing the actual index of the stack, creating a virtual sparse array.

What I would like to do is grouping tree scopes by using applyAuthorStyles.
The following shows how setupScopeStack creates m_scopeGroup:

group    scope        applyAuthorStyles
...
-3
---------------------
-2       ShadowRoot - applyAuthorStyles (false)
...
-2       ShadowRoot - applyAuthorStyles (true)
...
-2       ShadowRoot - applyAuthorStyles (true)
...
-2       ShadowRoot - applyAuthorStyles (true)
---------------------
-1       ShadowRoot - applyAuthorStyles (false)
...
-1       ShadowRoot - applyAuthorStyles (true)
...
---------------------
0        ShadowRoot - applyAuthorStyles (false)
...
0        scope (current, starting scope traversal from this node)

The group number doesn't depend on whether ruleset in a scope or a shadow root.  It just depends on "isShadowRoot and applyAuthorStyles=false".
(As setupScopeStack goes up by using parentOrHostNode, group number decreases by 1 when a shadow root is found and its applyAuthorStyles is false. pushScope increases the group number by 1 when a given scope is a shadow root and its applyAuthorStyles is false.)
m_scopeGroup has such group numbers. If all shadow roots have applyAuthorStyles set, m_scopeGroup in m_scopeStack have the same value. If all shadow roots have applyAuthorStyles unset, m_scopeGroup in m_scopeStack work like treeScope().

So matchScopedAuthorRules can find matched scoped styles in shadow DOM subtrees by  just comparing current group number (=m_scopeParentGroup) with m_scopeGroup in ScopeStackFrame.

Best regards,
Takashi Sakamoto

[Dimitri Glazkov (Google)]

(In reply to comment #11)
> What I would like to do is grouping tree scopes by using applyAuthorStyles.

Oh I see! Then we have a different problem: the purpose of this member variable is very hard to figure out from its name. It's a Good Thing (tm) in WebKit to make sure that things are named appropriately -- so that when a future aspiring WebKit engineer looks at your code, they can understand its purpose quickly.

[Takashi Sakamoto]

Created attachment 148257 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #12)
> (In reply to comment #11)
> > What I would like to do is grouping tree scopes by using applyAuthorStyles.
> 
> Oh I see! Then we have a different problem: the purpose of this member variable is very hard to figure out from its name. It's a Good Thing (tm) in WebKit to make sure that things are named appropriately -- so that when a future aspiring WebKit engineer looks at your code, they can understand its purpose quickly.

I agree that the name, scopeGroup, is not good. So I renamed it to applyAuthorStyleSetGroup in the new patch (2012-06-18's).

I'm also thinking of upper boundary of applicable shadom dom subtrees, but I think, as m_scopeStackParent's upper boundary should be updated when setupScopeStack or popScopeStack is invoked, so the methods will probably be modified in the following way:

void StyleResolver::setupScopeStack(…)
{
...
   const ContainerNode* firstBoundary = 0;
...
   for ( ; scope ; ) {
     const ContainerNode* boundary = 0;
     const ContainerNode* nextLowerBoundary = scope;
     for (nextLowerBoundary = scope; nextLowerBoundary; nextLowerBoundary = nextLowerBoundary->parentOrHostNode()) {
       if (isShadowRoot(nextLowerBoundary) && !toShadowRoot(nextLowerBoundary)->isShadowRoot()) {
          boundary = nextLowerBoundary;
          nextLowerBoundary = nextLowerBoundary->parentOrHostNode();
          break;
        }
     }
     if (!firstBoundary)
       firstBoudary = boundary; // 
     for ( ; scope != nextLowerBoundary; scope = scope->parentOrHostNode()) {
      …
     
     }
   }
   m_scopeStackParentUpperBoundary = firstBoundary;
   m_scopeParent = parent;
}

void StyleResolver::popScope(scope)
{
...
     if (isShadowRoot(scope) && !toShadowRoot(scope)->applyAuthorStyles()) {
         const ContainerNode* boudary = scope;
         // need to go up to find a new upper boundary.
         for (boudary = scope; boudary; boudary = boudary->parentOrHostNode())
           if (isShadowRoot(boudary) && !toShadowRoot(boudary)->isShadowRoot())
               break;
         m_scopeStackParentUpperBoundary = boundary;
     }
     m_scopeStackParent = scope->parentOrHostNode(); 
….


In this case, we can use m_applicableStyleUpperBoundary or something instead of m_scopeGroup. However I feel that the code is not as simple as the current patch.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 148268 [details]
Patch

[Takashi Sakamoto]

As I found that I forgot to use window.testRunner instead of window.layoutTestController, I recreated a patch.

[Dimitri Glazkov (Google)]

Comment on attachment 148268 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:568
> +    int applyAuthorStyleSetGroup = 0;

ok, I'll bikeshed some more: authorStyleBoundsIndex.

[Takashi Sakamoto]

Created attachment 148489 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #17)
> (From update of attachment 148268 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148268&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:568
> > +    int applyAuthorStyleSetGroup = 0;
> 
> ok, I'll bikeshed some more: authorStyleBoundsIndex.

Thank you.
I renamed applyAuthorSetGroup to authorStyleBoundsIndex.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Comment on attachment 148489 [details]
Patch

I'm sorry. I found one mistake. I have to use setupScopeStack(m_parentNode) in matchScopedAuthorRules. The current patch doesn't satisfy the spec:

- the styles of the insertion point node are inherited by those child nodes of the shadow host that are assigned to this insertion point
(http://www.w3.org/TR/shadow-dom/#styles)

I will fix soon.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

(In reply to comment #20)
> (From update of attachment 148489 [details])
> I'm sorry. I found one mistake. I have to use setupScopeStack(m_parentNode) in matchScopedAuthorRules. The current patch doesn't satisfy the spec:
> 
> - the styles of the insertion point node are inherited by those child nodes of the shadow host that are assigned to this insertion point
> (http://www.w3.org/TR/shadow-dom/#styles)
> 
> I will fix soon.
> 
> Best regards,
> Takashi Sakamoto

I'm sorry. As <content> or shadow root styles are not inherited by distributed nodes, I was afraid that the bug was created by this patch. 
However I found that this is another issue and is not caused by this patch. For searching scoped styles, I think, it is correct to looking at nodes according to DOM tree.

I'm not sure whether the inheritance bug has been already filed or not. If not, I will file the bug.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 148518 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 148489 [details] [details])
> > I'm sorry. I found one mistake. I have to use setupScopeStack(m_parentNode) in matchScopedAuthorRules. The current patch doesn't satisfy the spec:
> > 
> > - the styles of the insertion point node are inherited by those child nodes of the shadow host that are assigned to this insertion point
> > (http://www.w3.org/TR/shadow-dom/#styles)
> > 
> > I will fix soon.
> > 
> > Best regards,
> > Takashi Sakamoto
> 
> I'm sorry. As <content> or shadow root styles are not inherited by distributed nodes, I was afraid that the bug was created by this patch. 

I wrote a wrong thing. Shadow root doesn't need (and doesn't have) any styles.

[Dimitri Glazkov (Google)]

Comment on attachment 148518 [details]
Patch

Don't set r+ until you are a reviewer, it will confuse the plumbing.

[WebKit Review Bot]

Comment on attachment 148518 [details]
Patch

Clearing flags on attachment: 148518

Committed r120816: <http://trac.webkit.org/changeset/120816>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.