Bug 93755

(In reply to comment #0) > Created an attachment (id=157837) [details] > Reduction showing unexpected result and expected result when altering dom not using an insertion point. > > If an insertion point select attribute in a shadowRoot is changed synchronously with a css change that should provoke a transition, the transition does not occur. > > It is possible to address this problem by using webkitRequestAnimationFrame to provoke the transition. > > If this is the intended behavior it should be noted that it is unlike other similar changes to dom (e.g. appending a node to the document and provoking a transition on it can be done synchronously.) I investigated the bug and I'm now thinking of how to fix this issue. I think, this is caused by reattach in Element::recalcStyle. (1) Changing insertion point's select causes the shadow host's reattach. (c.f. ElementShadow::invalidateDistribution(). if (needsReattach && host->attached()) { host->detach(); ...) (2) Element::recalcStyle for the shadow host invokes reattach() because the host was detached. (3) During the reattach, the shadow host's renderer is re-created, and RenderObject::setAnimatableStyle is invoked. So animation()->updateAnimations() is also invoked. (animation is an instance of class AnimationController). (4) AnimationController::updateAnimations doesn't create ImplicitAnimation because currentStyle == null. (transitions() requires currentStyle != 0, but as renderer was detached, currentStyle was set to be null. i.e. the condition: (renderer->parent() || newStyle->animations() || (oldStyle && oldStyle->animations())) is false. (5) All styles are resolved, but no transitions are started. So, r.querySelector("content").setAttribute("select", "*:last-child"); // expected to provoke a css transition but does not. + document.body.offsetLeft; ts.classList.add("fade"); adding "document.body.offsetLeft;" before ts.classList.add("fade"), css transition for distributed nodes will work. Best regards, Takashi Sakamoto

Created attachment 158241 [details] WIP

Comment on attachment 158241 [details] WIP Attachment 158241 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13500128 New failing tests: fast/html/details-marker-style.html fast/dom/shadow/content-child-whitespace-between-span.html fast/html/details-add-child-2.html fast/html/details-open4.html fast/dom/shadow/shadow-contents-fallback-dynamic.html fast/dom/shadow/shadow-contents-select.html fast/html/details-no-summary4.html fast/html/details-open2.html fast/html/details-nested-1.html fast/html/details-nested-2.html fast/html/details-writing-mode.html fast/html/details-add-details-child-2.html fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

Created attachment 158250 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 159399 [details] WIP

Comment on attachment 159399 [details] WIP Attachment 159399 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13537707 New failing tests: fast/dom/shadow/content-child-whitespace-between-span.html fast/html/details-open4.html fast/html/details-no-summary4.html fast/html/details-nested-2.html fast/html/details-add-details-child-2.html fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

Created attachment 159405 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 159601 [details] WIP

Comment on attachment 159601 [details] WIP Attachment 159601 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13548283 New failing tests: fast/html/details-nested-2.html fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

Created attachment 159615 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 160063 [details] WIP

Comment on attachment 160063 [details] WIP Attachment 160063 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13569487 New failing tests: fast/html/details-nested-2.html fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html

Created attachment 160073 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 160103 [details] WIP

Comment on attachment 160103 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=160103&action=review > Source/WebCore/ChangeLog:10 > + shadow host's style clear. So when distribution is changed, no css The meaning of "clear" here is unclear :) Do you mean it clears the style of host's style? > Source/WebCore/dom/ElementShadow.cpp:92 > + // FIXME: Suppose that some css transition is applied to a shadow host and > + // new shadow roots are attached during the transition. In this case, > + // detaching the shadow host reset its style and breaks the css transition. > + // However, for replace elements, e.g. IMG, it is required to re-create > + // shadow host's renderer at the first time when an author shadow root is > + // added. So it might be better to set some flag here and to recreate > + // a renderer in Element::recalcStyle. This comment probably belongs in a bug. Reference bug here and enjoy using a much shorter explanation :) > Source/WebCore/dom/ElementShadow.cpp:95 > + shadowHost->detach(); > + shadowHost->lazyAttach(); Didn't we have something called re-attach on Node? > Source/WebCore/dom/ElementShadow.cpp:220 > + n->detach(); > + n->lazyAttach(); Are you sure we need something as drastic as reattach? Given that you're calling lazyAttach, you might be ok with just setNeedsStyleRecalc?

Comment on attachment 160103 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=160103&action=review >> Source/WebCore/dom/ElementShadow.cpp:95 >> + shadowHost->lazyAttach(); > > Didn't we have something called re-attach on Node? That isn't lazy. But I agree that it might be nice to have lazyReattach(). >> Source/WebCore/dom/ElementShadow.cpp:220 >> + n->lazyAttach(); > > Are you sure we need something as drastic as reattach? Given that you're calling lazyAttach, you might be ok with just setNeedsStyleRecalc? - We need deatch() here because the shape of the render tree is going to change. - lazyAttach() is basically setting the "attached" flag + setNeedsStyleRecalc().

Created attachment 160341 [details] Patch

Comment on attachment 160103 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=160103&action=review Thank you for reviewing. >> Source/WebCore/ChangeLog:10 >> + shadow host's style clear. So when distribution is changed, no css > > The meaning of "clear" here is unclear :) > > Do you mean it clears the style of host's style? Yes. I would like to say so. I fixed the description. >> Source/WebCore/dom/ElementShadow.cpp:92 >> + // a renderer in Element::recalcStyle. > > This comment probably belongs in a bug. Reference bug here and enjoy using a much shorter explanation :) Sure. I created a new bug 94905. >>> Source/WebCore/dom/ElementShadow.cpp:95 >>> + shadowHost->lazyAttach(); >> >> Didn't we have something called re-attach on Node? > > That isn't lazy. But I agree that it might be nice to have lazyReattach(). I have already tried shadowHost->reattach() here and I found that the reattach breaks the following layout tests: fast/dom/shadow/input-with-validation.html - crash fast/dom/shadow/shadowdom-for-progress-dynamic.html - image diff fast/dom/shadow/shadowdom-for-progress-multiple.html - image diff fast/dom/shadow/shadowdom-for-progress-with-style.html - image diff fast/dom/shadow/shadowdom-for-progress.html - image diff At least, the oldest shadow root will be reattached too often.

Comment on attachment 160341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160341&action=review Looks much better now. > Source/WebCore/ChangeLog:8 > + ElementShadow always detach shadow hosts when distribution is s/detach/detaches/ > Source/WebCore/ChangeLog:13 > + distribution, should not make shadow host reattach. Instead, should s/should/it should/ > Source/WebCore/ChangeLog:14 > + make distribute nodes reattach and set needsRecalcStyle flag of shadow s/reattach/reattached/ > Source/WebCore/ChangeLog:21 > + To support replace elements, i.e. IMG, and so on, shadow host's s/replace/replaced/

Created attachment 160624 [details] Patch

Comment on attachment 160624 [details] Patch Rejecting attachment 160624 [details] from commit-queue. New failing tests: http/tests/cache/post-with-cached-subresources.php Full output: http://queues.webkit.org/results/13614001

Created attachment 160631 [details] Archive of layout-test-results from gce-cq-02 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: gce-cq-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 160624 [details] Patch Clearing flags on attachment: 160624 Committed r126789: <http://trac.webkit.org/changeset/126789>

All reviewed patches have been landed. Closing bug.