Bug 160331

Summary:

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

Product:

WebKit

Reporter:

Trey Shugart <treshugart>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, commit-queue, dbates, esprehn+autocc, joepeck, kangil.han, koivisto, rniwa, simon.fraser, treshugart, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari Technology Preview

Hardware:

Mac

OS:

Other

Bug Depends on:

Bug Blocks:

148695

Attachments:

Description	Flags
Safari 10 left, Chrome 54 right	none
patch	dbates: review+

Trey Shugart

Reported 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!

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
View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

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

Radar WebKit Bug Importer

Comment 2 2016-07-29 08:43:09 PDT

<rdar://problem/27609715>

Ryosuke Niwa

Comment 3 2016-08-01 17:53:53 PDT

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

Antti Koivisto

Comment 4 2016-08-02 00:14:13 PDT

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

Ryosuke Niwa

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

Trey Shugart

Comment 6 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!

Ryosuke Niwa

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

Antti Koivisto

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

Antti Koivisto

Comment 9 2016-09-25 12:43:36 PDT

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

Antti Koivisto

Comment 10 2016-09-26 04:17:44 PDT

Created attachment 289818 [details] patch

Ryosuke Niwa

Comment 11 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?

Daniel Bates

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

Ryosuke Niwa

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

Ryosuke Niwa

Comment 14 2016-09-26 10:11:57 PDT

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

Antti Koivisto

Comment 15 2016-09-26 10:54:17 PDT

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

Ryosuke Niwa

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

Antti Koivisto

Comment 17 2016-09-26 16:54:31 PDT

https://trac.webkit.org/r206404

Antti Koivisto

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

Trey Shugart

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

Note You need to log in before you can comment on or make changes to this bug.