Bug 160331 – Setter on style element's textContent or cssText doesn't trigger style recalc

• Component: CSS
• Status: RESOLVED FIXED

[Trey Shugart]

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!

[Ryosuke Niwa]

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);

[Radar WebKit Bug Importer]

<rdar://problem/27609715>

[Ryosuke Niwa]

The problem here appears to be that Document::styleResolverChanged doesn't update style resolvers in shadow trees.

[Antti Koivisto]

Or that we don't have ShadowRoot equivalent of styleResolverChanged.

[Ryosuke Niwa]

(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.

[Trey Shugart]

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!

[Ryosuke Niwa]

(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.

[Antti Koivisto]

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.

[Antti Koivisto]

...or at least before pinning them to deep architectural issues. :)

[Antti Koivisto]

Created attachment 289818 [details]
patch

[Ryosuke Niwa]

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?

[Daniel Bates]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Also FrameView::setViewportSizeForCSSViewportUnits and possibly FrameView::setPagination.

[Antti Koivisto]

Sure, there are some known and likely many unknown shadow DOM related bugs and inefficiencies.

[Ryosuke Niwa]

(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.

[Antti Koivisto]

https://trac.webkit.org/r206404

[Antti Koivisto]

> 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.

[Trey Shugart]

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.