Default element styles are not always collected for sharing detection

[Dean Jackson]

If an element has a fairly complex shadow tree, the elements do not always resolve style correctly if they are shared. This can be occasionally seen on the Mac port on a page with <video> and multiple captions (e.g. LayoutTests/media/video-controls-captions-trackmenu*). The captions menu is part of the shadow structure, and elements in the menu are styled with a class name to indicate they should be "ticked". Often you will see that all the elements are ticked, even when that is impossible. A recalculation of the style usually fixes it.

As a test I added some code to StyleResolver::locateSharedStyle() so that it will not return a valid style when the element is in the shadow tree. This fixes the problem.

    if (m_styledElement->isInShadowTree())
        return 0;

[Dean Jackson]

<rdar://problem/12913909>

[Dean Jackson]

Created attachment 185631 [details]
Patch

[Dean Jackson]

This fixes the problem, but obviously at the expense of performance. I'd like to know the root cause.

[Takashi Sakamoto]

Would you provide a test case which reproduces this issue?

Because:
- I looked at flakiness dashboard, http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&showAllRuns=true&tests=media%2Fvideo-controls-captions-track.*&showExpectations=true

  However LayoutTests/media/video-controls-captions-trackmenu* seem not flaky.

- I ran Tools/Scripts/new-run-webkit-tests, but media/video-controls-captions-trackmenu* passed.

- I cannot see rdar://problem/12913909.

[Ojan Vafai]

Comment on attachment 185631 [details]
Patch

I'd rather we understand the problem at least before doing this. This seems like something that would stick around for a long time as it's not even clear from this patch now to reproduce it.

[Dean Jackson]

Running these as layout tests won't show the problem, because they don't examine the style of the shadow tree. Also, Chromium hasn't yet implemented the captions menu (afaik).

Anyway, I'll try to work out what's going on so that I can get a reproducible test case, but it might be really difficult because the problem only exposes itself with a complex shadow root, added internally (not a User Shadow Tree), and none of our built-in controls are that complex - with the exception of <video>

[Dean Jackson]

The problem seems to be that StyleResolver::locateSharedStyle does not notice that the elements have a className that would prevent them from sharing style. The resolver's RuleFeatureSet does not include any classNames, and thus the test for classNamesAffectedByRules(m_element->classNames()) fails. This causes elements in the shadow tree to share styles, even when they should not.

[Dean Jackson]

Actually, it may not be related to the shadow root. StyleResolver::collectFeatures runs before ensureDefaultStyleSheetsForElement. The first copies rules from the defaultStyle into its own rules, but misses anything added to the defaultStyle in the latter function.

[Dean Jackson]

Created attachment 185883 [details]
Patch

[Dean Jackson]

Here is a much better patch that addresses the root problem (I hope). Antti should take a look at this.

Unfortunately it's still really hard to get a reproducible test case.

[Dimitri Glazkov (Google)]

Thank you for working on this, Dean!

[Dominic Cooney]

Comment on attachment 185883 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Shadow tree doesn't calculate style correctly if shared

Should this summary be changed? From reading the comments it looks like your conclusion was that being in the shadow tree was a red herring.

[Dean Jackson]

(In reply to comment #12)
> (From update of attachment 185883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185883&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Shadow tree doesn't calculate style correctly if shared
> 
> Should this summary be changed? From reading the comments it looks like your conclusion was that being in the shadow tree was a red herring.

Good point. I'll update the ChangeLog after review, but change the title here now.

[Antti Koivisto]

Comment on attachment 185883 [details]
Patch

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

Good find! A test would be cool though I realize it may be difficult.

> Source/WebCore/ChangeLog:12
> +
> +        This is a fairly obscure case that is difficult to reproduce. It most
> +        often appeared when styling siblings in a complex shadow tree. The problem
> +        was that ensureDefaultStyleSheetsForElement could add rules to the
> +        defaultStyle, but that was not collected into the StyleResolver, which
> +        uses the rules to detect if it should not share styles.

As mentioned, this needs to be updated the describe the actual problem.

> Source/WebCore/css/StyleResolver.cpp:558
> -static void ensureDefaultStyleSheetsForElement(Element* element)
> +static bool ensureDefaultStyleSheetsForElement(Element* element)

I think you should use a bool& argument instead of a return value. Currently it is rather mysterious in the call site what the return value means. Then you can use a sensibly named variable.

[Dean Jackson]

Committed r141844: <http://trac.webkit.org/changeset/141844>