Text nodes in shadow roots don't inherit style properly

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Elliott Sprehn]

Test case:

shadow = new WebKitShadowRoot(document.body)
span = document.createElement('span')
span.textContent = "foo";
shadow.appendChild(span);
shadow.appendChild(document.createTextNode("bar"))
document.body.offsetTop; // cause a layout.
document.body.style.fontSize = '5em';

You should see foo bar in big text, but you only see "foo" in big text.

The bug is here:

http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/Text.cpp&exact_package=chromium&l=259

and SVG has a hack for it:

http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/svg/SVGTRefElement.cpp&exact_package=chromium&q=SVGTRefElement&type=cs&l=156

This means text in a shadow root is broken for zooming and style changes.

[Takashi Sakamoto]

Created attachment 172539 [details]
Patch

[Build Bot]

Comment on attachment 172539 [details]
Patch

Attachment 172539 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14747411

[Dimitri Glazkov (Google)]

Comment on attachment 172539 [details]
Patch

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

> Source/WebCore/dom/Text.cpp:267
> +        Node* parentForStyle = parent->isShadowRoot() ?
> +            toShadowRoot(parent)->shadowHost() :
> +            parent;

This should probably be just one line.

[Elliott Sprehn]

Comment on attachment 172539 [details]
Patch

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

When resetStyleInheritance is true we have no m_parentStyle in StyleResolver and then go down this code path: http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/css/StyleResolver.cpp&exact_package=chromium&q=StyleResolver.cpp&type=cs&l=1545

You need to have this same behavior for text nodes.

> Source/WebCore/dom/Text.cpp:262
>      // is a shadow root.

Remove this comment since it doesn't apply now.

> Source/WebCore/dom/Text.cpp:268
> +        renderer->setStyle(parentForStyle->renderStyle());

This is wrong if you have resetStyleInheritance set because you unconditionally inherit from the shadow host. You need to expose the default style from inside the StyleResolver, perhaps with some abstraction like document()->styleResolver()->styleForText(this) that models the existing styleForElement.

[Dimitri Glazkov (Google)]

Comment on attachment 172539 [details]
Patch

Thanks for looking this over, Elliot.

[Takashi Sakamoto]

Thank you for reviewing.

I missed reset-style-inheritance. I agree that I have to think about this.

So.. however I would like to confirm what style we should apply to resetted text nodes. The reason why I'm thinking about this is:
(1) we cannot directly apply any styles to text nodes.
(2) if we have to check reset-style-inheritance flag, we have to modify NodeRendererFactory::createRendererIfNeeded() too, i.e.

---- before -----
    Element* element = node->isElementNode() ? toElement(node) : 0;
    if (element)
        m_context.setStyle(element->styleForRenderer());
    else if (RenderObject* parentRenderer = m_context.parentRenderer())
        m_context.setStyle(parentRenderer->style());
---- after ----
    Element* element = node->isElementNode() ? toElement(node) : 0;
    if (element)
        m_context.setStyle(element->styleForRenderer());
    else if (node->isTextNode())
        m_context.setStyle(document()->styleResolver()->styleForText(node); // added
    else if (RenderObject* parentRenderer = m_context.parentRenderer())
        m_context.setStyle(parentRenderer->style());

Since for each text node we have to use NodeRenderingContext to check reset-style-inheritance, I'm afraid of performance regression. Is this allowable regression?

Best regards,
Takashi Sakamoto

[Hajime Morrita]

(In reply to comment #6)
> Thank you for reviewing.
> 
> I missed reset-style-inheritance. I agree that I have to think about this.
> 
> So.. however I would like to confirm what style we should apply to resetted text nodes. The reason why I'm thinking about this is:
> (1) we cannot directly apply any styles to text nodes.
> (2) if we have to check reset-style-inheritance flag, we have to modify NodeRendererFactory::createRendererIfNeeded() too, i.e.
> 
> ---- before -----
>     Element* element = node->isElementNode() ? toElement(node) : 0;
>     if (element)
>         m_context.setStyle(element->styleForRenderer());
>     else if (RenderObject* parentRenderer = m_context.parentRenderer())
>         m_context.setStyle(parentRenderer->style());
> ---- after ----
>     Element* element = node->isElementNode() ? toElement(node) : 0;
>     if (element)
>         m_context.setStyle(element->styleForRenderer());
>     else if (node->isTextNode())
>         m_context.setStyle(document()->styleResolver()->styleForText(node); // added
>     else if (RenderObject* parentRenderer = m_context.parentRenderer())
>         m_context.setStyle(parentRenderer->style());
> 
> Since for each text node we have to use NodeRenderingContext to check reset-style-inheritance, I'm afraid of performance regression. Is this allowable regression?

First of all, let's measure it. Our PerformanceTests has some parser-related tests which exercise
not only the parser but also the style computation. It would be a good starting point.

[Elliott Sprehn]

(In reply to comment #6)
> ...
> Since for each text node we have to use NodeRenderingContext to check reset-style-inheritance, I'm afraid of performance regression. Is this allowable regression?

isShadowRoot and isTextNode are bitfield checks and we only need this special case for text nodes that are direct descendants of shadow roots.

Element* element = node->isElementNode() ? toElement(node) : 0;
if (element)
  m_context.setStyle(element->styleForRenderer());
else if (node->isTextNode() && node->parentOrHostNode()->isShadowRoot())
  m_context.setStyle(document()->styleResolver()->styleForText(node);
else
  m_context.setStyle(m_context.parentRenderer()->style());

The last check is simplified as in Bug 102765 and since the conditional is on a bitfield it seems very unlikely there's a perf regression.

[Takashi Sakamoto]

Created attachment 175145 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #8)
> (In reply to comment #6)
> > ...
> > Since for each text node we have to use NodeRenderingContext to check reset-style-inheritance, I'm afraid of performance regression. Is this allowable regression?
> 
> isShadowRoot and isTextNode are bitfield checks and we only need this special case for text nodes that are direct descendants of shadow roots.
> 
> Element* element = node->isElementNode() ? toElement(node) : 0;
> if (element)
>   m_context.setStyle(element->styleForRenderer());
> else if (node->isTextNode() && node->parentOrHostNode()->isShadowRoot())
>   m_context.setStyle(document()->styleResolver()->styleForText(node);
> else
>   m_context.setStyle(m_context.parentRenderer()->style());
> 
> The last check is simplified as in Bug 102765 and since the conditional is on a bitfield it seems very unlikely there's a perf regression.

Thank you.
I will recreate a new patch and upload.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 175150 [details]
Patch

[Takashi Sakamoto]

Created attachment 175151 [details]
Patch

[Takashi Sakamoto]

Created attachment 175152 [details]
Patch

[Elliott Sprehn]

Comment on attachment 175145 [details]
Patch

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

We can make createRendererIfNeeded faster by not creating a whole new NodeRenderingContext and skipping the parent traversal using an overload and passing in the context from NodeRenderFactory:

StyleResolver::styleForText(Node* textNode, NodeRenderingContext& context) { ... }
StyleResolver::styleForText(Node* textNode)
{
  NodeRenderingContext context(textNode);
  return styleForText(textNode, context);
}

I'm not sure this optimization is needed, but what you have right now does traverse the parents repeatedly.

> Source/WebCore/ChangeLog:8
> +        Use NodeRenderingContext to solve styles of text nodes.

resolve

> Source/WebCore/ChangeLog:9
> +        If text nodes are direct chilren of shadow roots, the text nodes

children

> Source/WebCore/css/StyleResolver.cpp:1795
> +    NodeRenderingContext context(textNode);

It seems kind of silly to create a whole new rendering context and do the ComposedShadowTreeWalker::findParent traversal during createRendererIfNeeded() when we already have all that information in createRendererIfNeeded. It might not actually matter for performance though.

[Takashi Sakamoto]

Created attachment 175156 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 175145 [details]
Patch

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

Thank you for quickly reviewing.

>> Source/WebCore/ChangeLog:8
>> +        Use NodeRenderingContext to solve styles of text nodes.
> 
> resolve

Done. Thanks.

>> Source/WebCore/ChangeLog:9
>> +        If text nodes are direct chilren of shadow roots, the text nodes
> 
> children

Done.

>> Source/WebCore/css/StyleResolver.cpp:1795
>> +    NodeRenderingContext context(textNode);
> 
> It seems kind of silly to create a whole new rendering context and do the ComposedShadowTreeWalker::findParent traversal during createRendererIfNeeded() when we already have all that information in createRendererIfNeeded. It might not actually matter for performance though.

I see. I added styleForElement(Node*, NodeRenderingContext&).

[Elliott Sprehn]

Comment on attachment 175156 [details]
Patch

Looks good to me.

[Takashi Sakamoto]

Created attachment 175183 [details]
Patch

[Takashi Sakamoto]

I modified the condition in createRendererIfNeeded, because I think, m_context.insertionPoint() is a little better than isShadowHost(node->parentOrHostNode()).

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 175189 [details]
Patch

[Build Bot]

Comment on attachment 175189 [details]
Patch

Attachment 175189 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14894596

[Takashi Sakamoto]

Created attachment 175347 [details]
Patch

[Hajime Morrita]

What about performance?

[Elliott Sprehn]

Comment on attachment 175347 [details]
Patch

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

This looks good to me, but I'm not a reviewer so I can't r+. Perhaps someone else can do that. :)

> Source/WebCore/dom/NodeRenderingContext.cpp:252
> +        m_context.setStyle(node->document()->styleResolver()->styleForText(m_context));

You can just move the Document* document = node->document(); line back to the top of the function again if you want too.

[Elliott Sprehn]

(In reply to comment #23)
> What about performance?

To test performance of this use the html5-full-render.html perf test. It's a good test of createRendererIfNeeded() and recalcTextStyle().

[Takashi Sakamoto]

(In reply to comment #25)
> (In reply to comment #23)
> > What about performance?
> 
> To test performance of this use the html5-full-render.html perf test. It's a good test of createRendererIfNeeded() and recalcTextStyle().

Thank you. I looked at PerfTest/Parser/*.

So I found that this patch makes Parser/tiny-innerHTML much slower (10%-20% slower).
The test updates innerHTML again and again:

PerfTestRunner.measureRunsPerSecond({run:function() {
    var testDiv = document.createElement("div");
    testDiv.style.display = "none";
    document.body.appendChild(testDiv);
    for (var x = 0; x < 100000; x++) {
        testDiv.innerHTML = "This is a tiny HTML document";
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    }
    document.body.removeChild(testDiv);
}});

So the newly added "if" , i.e.

 + else if (node->isTextNode() && (node->parentOrHostNode()->isShadowRoot() || m_context.insertionPoint()))
 +       m_context.setStyle(node->document()->styleResolver()->styleForText(m_context));

is checked every time testDiv.innerHTML is updated. I couldn't find the way to avoid this performance regression.

Best regards,
Takashi Sakamoto

[Elliott Sprehn]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #23)
> > > What about performance?
> > 
> > To test performance of this use the html5-full-render.html perf test. It's a good test of createRendererIfNeeded() and recalcTextStyle().
> 
> Thank you. I looked at PerfTest/Parser/*.
> 
> So I found that this patch makes Parser/tiny-innerHTML much slower (10%-20% slower).
> The test updates innerHTML again and again:
> 

Can you try this test but with ENABLE(DIALOG_ELEMENT) turned off and ENABLE(FULLSCREEN_API) turned off?  It seems very suspicious that adding this check is such a big perf regression but those two feature flags don't cause similarly massive perf regressions.

[Hajime Morrita]

> 
> Can you try this test but with ENABLE(DIALOG_ELEMENT) turned off and ENABLE(FULLSCREEN_API) turned off?  It seems very suspicious that adding this check is such a big perf regression but those two feature flags don't cause similarly massive perf regressions.

Another thing we can try is to use pprof and see the difference
between before and after the change.

[Takashi Sakamoto]

Created attachment 176199 [details]
Patch

[Takashi Sakamoto]

Thank you for advising, Elliott and Morita-san.

Today I rebased my patch. So I found no big performance regression...

I measured by using the command, "Tools/Scripts/run-perf-tests --release --no-build Parser/\*" and I used the Patch(2012-11-27 01:41 PST).

So I'm now trying to use pperf.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 176674 [details]
Patch

[Takashi Sakamoto]

I created a spreadsheet for Parser/tiny-innerHTML, Parser/html5-full-render, Parser/html-parser:
https://docs.google.com/spreadsheet/ccc?key=0AtdkWCxGaEc2dDhUcTlFanprMUR0cGU0QlB1eWNoMWc

I guess, someone's refactoring would probably hide my change.

Best regards,
Takashi Sakamoto

[Dimitri Glazkov (Google)]

(In reply to comment #32)
> I guess, someone's refactoring would probably hide my change.

Not sure what this means :) Can you elaborate?

[Takashi Sakamoto]

(In reply to comment #33)
> (In reply to comment #32)
> > I guess, someone's refactoring would probably hide my change.
> 
> Not sure what this means :) Can you elaborate?

I found that some one changed createRendererIfNeeded in NodeRenderingContext.cpp. 
My first patch was for updating NodeRendererFactory::createRendererIfNeeded.
But now NodeRenderingContext::createRendererForTextIfNeeded was implemented for just creating a renderer for a text node. I recreated my patch to update the createRendererForTextIfNeeded.
I think, this change affected number of "if" (or ...) when testDiv.innerHTML = "This is a tiny HTML document" is executed.
 
Best regards,
Takashi Sakamoto

[Hajime Morrita]

Comment on attachment 176674 [details]
Patch

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

> Source/WebCore/dom/Text.cpp:292
> +            renderer->setStyle(parent->renderer()->style());

Having these tricky condition in two places is unfortunate and error prone.
Can we put this inside StyleResolver::styleForText() somehow?

[Takashi Sakamoto]

Created attachment 178150 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 176674 [details]
Patch

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

>> Source/WebCore/dom/Text.cpp:292
>> +            renderer->setStyle(parent->renderer()->style());
> 
> Having these tricky condition in two places is unfortunate and error prone.
> Can we put this inside StyleResolver::styleForText() somehow?

I see. Done.

[Hajime Morrita]

Comment on attachment 178150 [details]
Patch

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

> Source/WebCore/dom/NodeRenderingContext.cpp:285
> +    if (m_node->parentOrHostNode()->isShadowRoot() || insertionPoint())

Apparently this check also can be part of styleForText().

[Takashi Sakamoto]

Created attachment 178166 [details]
Patch

[Hajime Morrita]

Comment on attachment 178166 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:1809
> +        m_style->font().update(0);

This empty style creation code looks like being copied from styleForElement(). can we share this between two method?

> Source/WebCore/css/StyleResolver.cpp:1821
> +        return parentNode->renderStyle();

This code is so cryptic and hard to see its correctness. 
Is it possible to just let stleForText(NodeRenderingContext) do this? 
Is it so slow?

[Takashi Sakamoto]

Created attachment 178172 [details]
Patch

[Takashi Sakamoto]

Created attachment 178176 [details]
Patch

[Takashi Sakamoto]

Created attachment 178177 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 178166 [details]
Patch

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

>> Source/WebCore/css/StyleResolver.cpp:1809
>> +        m_style->font().update(0);
> 
> This empty style creation code looks like being copied from styleForElement(). can we share this between two method?

I see. Done.

>> Source/WebCore/css/StyleResolver.cpp:1821
>> +        return parentNode->renderStyle();
> 
> This code is so cryptic and hard to see its correctness. 
> Is it possible to just let stleForText(NodeRenderingContext) do this? 
> Is it so slow?

I agree that this code looks not-good.
I heard that NodeRenderingContext is a little slow. So this code is to creating a fast path, i.e. avoiding creating NodeRenderingContext.
If always using NodeRenderingContext here is ok, I would like to remove this.

[Takashi Sakamoto]

Created attachment 178180 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 178166 [details]
Patch

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

>>> Source/WebCore/css/StyleResolver.cpp:1821
>>> +        return parentNode->renderStyle();
>> 
>> This code is so cryptic and hard to see its correctness. 
>> Is it possible to just let stleForText(NodeRenderingContext) do this? 
>> Is it so slow?
> 
> I agree that this code looks not-good.
> I heard that NodeRenderingContext is a little slow. So this code is to creating a fast path, i.e. avoiding creating NodeRenderingContext.
> If always using NodeRenderingContext here is ok, I would like to remove this.

Done.
I removed the code.

[Elliott Sprehn]

Comment on attachment 178180 [details]
Patch

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

Looks awesome :)

> Source/WebCore/css/StyleResolver.cpp:1781
> +PassRefPtr<RenderStyle> StyleResolver::defaultStyleForElement()

defaultStyleForNode() ? Text are not Elements.

> Source/WebCore/css/StyleResolver.cpp:1794
> +PassRefPtr<RenderStyle> StyleResolver::styleForText(Node* textNode)

This should just take Text* and you can remove the isTextNode() assert below.

> Source/WebCore/dom/NodeRenderingContext.cpp:285
> +    if (UNLIKELY(resetStyleInheritance()))

Does the UNLIKELY actually make a difference? ojan has said it doesn't usually matter (and produces identical assembly).

[Dimitri Glazkov (Google)]

Comment on attachment 178180 [details]
Patch

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

>> Source/WebCore/css/StyleResolver.cpp:1781
>> +PassRefPtr<RenderStyle> StyleResolver::defaultStyleForElement()
> 
> defaultStyleForNode() ? Text are not Elements.

In this context, he's just initializing defaults for element style resolution.

>> Source/WebCore/css/StyleResolver.cpp:1794
>> +PassRefPtr<RenderStyle> StyleResolver::styleForText(Node* textNode)
> 
> This should just take Text* and you can remove the isTextNode() assert below.

Good point!

>> Source/WebCore/dom/NodeRenderingContext.cpp:285
>> +    if (UNLIKELY(resetStyleInheritance()))
> 
> Does the UNLIKELY actually make a difference? ojan has said it doesn't usually matter (and produces identical assembly).

It's called UNLIKELY, because it is unlikely to have any effect on your code.

[Takashi Sakamoto]

Created attachment 178955 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 178180 [details]
Patch

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

>>> Source/WebCore/css/StyleResolver.cpp:1781
>>> +PassRefPtr<RenderStyle> StyleResolver::defaultStyleForElement()
>> 
>> defaultStyleForNode() ? Text are not Elements.
> 
> In this context, he's just initializing defaults for element style resolution.

Yes. Thank you Dimitri. I would like to say so.

>>> Source/WebCore/css/StyleResolver.cpp:1794
>>> +PassRefPtr<RenderStyle> StyleResolver::styleForText(Node* textNode)
>> 
>> This should just take Text* and you can remove the isTextNode() assert below.
> 
> Good point!

Thanks. Done.

>>> Source/WebCore/dom/NodeRenderingContext.cpp:285
>>> +    if (UNLIKELY(resetStyleInheritance()))
>> 
>> Does the UNLIKELY actually make a difference? ojan has said it doesn't usually matter (and produces identical assembly).
> 
> It's called UNLIKELY, because it is unlikely to have any effect on your code.

I removed UNLIKELY. Done.

[Hajime Morrita]

Comment on attachment 178955 [details]
Patch

Looks good now. Thanks for the fix!

[WebKit Review Bot]

Comment on attachment 178955 [details]
Patch

Clearing flags on attachment: 178955

Committed r137418: <http://trac.webkit.org/changeset/137418>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.