Bug 160331 - Setter on style element's textContent or cssText doesn't trigger style recalc
Summary: Setter on style element's textContent or cssText doesn't trigger style recalc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari Technology Preview
Hardware: Macintosh Other
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2016-07-28 21:06 PDT by Trey Shugart
Modified: 2016-10-04 21:50 PDT (History)
11 users (show)

See Also:


Attachments
Safari 10 left, Chrome 54 right (1.21 MB, image/png)
2016-07-28 21:06 PDT, Trey Shugart
no flags Details
patch (8.85 KB, patch)
2016-09-26 04:17 PDT, Antti Koivisto
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Trey Shugart 2016-07-28 21:06:04 PDT
Created attachment 284846 [details]
Safari 10 left, Chrome 54 right

I'm running macOS Sierra and using Safari 10. I'm using a custom element polyfill for v1 along with native Shadow DOM v1 and no styles are getting picked up at all. I've added style tags to the shadow roots. Things work fine in Chrome Canary (native v1). See http://skate.js.org and inspect the shadow roots to debug it. I've attached a side-by-side screenshot of Safari 10 and Chrome 54 running the same page.

I've raised this as a blocker because if it is in fact an issue, Shadow DOM is unusable. I also raised this under CSS because there is no component for Shadow DOM or Web Component it seems.

Thanks!
Comment 1 Ryosuke Niwa 2016-07-28 23:13:36 PDT
Wow, this is a terrible bug. It looks like something is style recalc is completely broken.  e.g. if you re-insert the style element again, it renders correctly.

The bug here is that setting style element's content doesn't update the element.


You can work around it by re-inserting the element in your `applyStyle` function:
https://github.com/skatejs/kickflip/blob/8dd73b87c7027acb7b3da4906c9a290569a4c42e/dist/index.js#L525

as in

el.parentNode.insertBefore(el, el.nextSibling);
Comment 2 Radar WebKit Bug Importer 2016-07-29 08:43:09 PDT
<rdar://problem/27609715>
Comment 3 Ryosuke Niwa 2016-08-01 17:53:53 PDT
The problem here appears to be that Document::styleResolverChanged doesn't update style resolvers in shadow trees.
Comment 4 Antti Koivisto 2016-08-02 00:14:13 PDT
Or that we don't have ShadowRoot equivalent of styleResolverChanged.
Comment 5 Ryosuke Niwa 2016-08-02 00:16:46 PDT
(In reply to comment #4)
> Or that we don't have ShadowRoot equivalent of styleResolverChanged.

Adding that would solve this problem. It looks like we're missing a bunch of style recalcs due to media type changes, etc... right now though. I think we need some mechanism to notify all style resolvers in a given document to be updated for those.
Comment 6 Trey Shugart 2016-09-20 18:16:23 PDT
Just wondering if there's been any movement on this. We're still seeing this in Safari 10 stable, which renders native Shadow DOM V1 unusable for us. Thanks!
Comment 7 Ryosuke Niwa 2016-09-23 16:25:02 PDT
(In reply to comment #6)
> Just wondering if there's been any movement on this. We're still seeing this
> in Safari 10 stable, which renders native Shadow DOM V1 unusable for us.

Fixing this bug requires solving some architectural problem in WebKit but we're hoping to fix in the coming months.
Comment 8 Antti Koivisto 2016-09-25 12:34:19 PDT
The main reason this fails is a simple bug. We don't signal text content mutations to parent in shadow trees:

void CharacterData::dispatchModifiedEvent(const String& oldData)
{
    ...
    if (!isInShadowTree()) {
        if (parentNode()) {
            ...
            parentNode()->childrenChanged(change);
        }

Fixing this makes the page render mostly correctly. Generally we should have a reduced test case available before trying to analyze the cause.
Comment 9 Antti Koivisto 2016-09-25 12:43:36 PDT
...or at least before pinning them to deep architectural issues. :)
Comment 10 Antti Koivisto 2016-09-26 04:17:44 PDT
Created attachment 289818 [details]
patch
Comment 11 Ryosuke Niwa 2016-09-26 09:56:14 PDT
Comment on attachment 289818 [details]
patch

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

> Source/WebCore/dom/CharacterData.h:62
> +    void notifyParentAfterChange(ContainerNode::ChildChangeSource);

Does this really need to be a protected member?  Can't it be a private member instead?
Comment 12 Daniel Bates 2016-09-26 10:05:37 PDT
Comment on attachment 289818 [details]
patch

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

> Source/WebCore/dom/AuthorStyleSheets.cpp:343
> -        if (m_shadowRoot)
> -            m_shadowRoot->setNeedsStyleRecalc();
> -        else
> +        if (m_shadowRoot) {
> +            for (auto& shadowChild : childrenOfType<Element>(*m_shadowRoot))
> +                shadowChild.setNeedsStyleRecalc();
> +        } else

It seems easy to make this kind of mistake and call setNeedsStyleRecalc() on a ShadowRoot. Would it make sense to either override ShadowRoot::setNeedsStyleRecalc() and have it call Element::setNeedsStyleRecalc() on its children or mark ShadowRoot::setNeedsStyleRecalc() as "delete"d to cause a compile-time error when ShadowRoot::setNeedsStyleRecalc() is called.
Comment 13 Ryosuke Niwa 2016-09-26 10:10:01 PDT
(In reply to comment #9)
> ...or at least before pinning them to deep architectural issues. :)

I think we still have an issue where we don't trigger style recalc in every shadow tree in Document::setContentLanguage, Page::setViewMode, Page::setNeedsRecalcStyleInAllFrames, SVGFontFaceElement::rebuildFontFace, and SVGFontFaceElement::removedFrom to name a few.
Comment 14 Ryosuke Niwa 2016-09-26 10:11:57 PDT
Also FrameView::setViewportSizeForCSSViewportUnits and possibly FrameView::setPagination.
Comment 15 Antti Koivisto 2016-09-26 10:54:17 PDT
Sure, there are some known and likely many unknown shadow DOM related bugs and inefficiencies.
Comment 16 Ryosuke Niwa 2016-09-26 12:08:33 PDT
(In reply to comment #15)
> Sure, there are some known and likely many unknown shadow DOM related bugs
> and inefficiencies.

Well, my point about the architectural problem is that basically everywhere we call Document::styleResolverChanged is a place where we might need to find a StyleResolver in every shadow tree and trigger a similar style recalc logic.
Comment 17 Antti Koivisto 2016-09-26 16:54:31 PDT
https://trac.webkit.org/r206404
Comment 18 Antti Koivisto 2016-09-26 16:55:18 PDT
> It seems easy to make this kind of mistake and call setNeedsStyleRecalc() on
> a ShadowRoot. Would it make sense to either override
> ShadowRoot::setNeedsStyleRecalc() and have it call
> Element::setNeedsStyleRecalc() on its children or mark
> ShadowRoot::setNeedsStyleRecalc() as "delete"d to cause a compile-time error
> when ShadowRoot::setNeedsStyleRecalc() is called.

Good idea, will do something like that later.
Comment 19 Trey Shugart 2016-10-04 21:47:17 PDT
For posterity, el.parentNode.insertBefore(el, el.nextSibling) doesn't seem to work. I had to first remove the style node and then insert before the next sibling.

Our solution in its entirety was to override attachShadow() and observe the shadow root that was created so that we can observe any style elements in it and re-insert them if they were mutated.

Code for fix: https://bitbucket.org/atlassian/atlaskit/src/c4fe7f74539ca5e76449c1c72b960ad0c0809fd0/packages/akutil-polyfills/src/index.js?fileviewer=file-view-default

We're going to go ahead with the override for a short-term fix. Thanks again for looking into this and fixing it so quickly.