Bug 92899 – Dynamically styling ShadowDom content on a node distributed to another shadow insertion point fails.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Steve Orvell]

Created attachment 155859 [details]
Test case for bug, including expected rendering.

Setup:
1. A node is given a shadow dom with an insertion point.
2. A child of the node is distributed to this insertion point.
3. The child is given a shadow dom that contains unapplied styling and another node.
4. The child shadow's node is styled with one of the css selectors in the child shadow dynamically (after initial rendering).

Result:
The node is not rendered with the expected styling even though the selector is properly added to the node.

[Takashi Sakamoto]

Created attachment 156653 [details]
WIP

[Takashi Sakamoto]

I will explain this bug by using the attached test case example. The DOM tree of the test case looks like:

#box ------------ sr1
  |                             |
  |                            +--<content>
  |
  +---#item ------ sr2
                                |
                                +---<style> 
                                |
                                +---<div> 

To render the test case, ElementShadow::invalidateDistribution is invoked because of <content>.
In ElementShadow::invalidateDistribution for sr1's <content>, #box->detach() and #box->lazyAttach(...) are invoked. So, all children of the host have childNeedsStyleRecalc, i.e. #item has childNeedsStyleRecalc true.
As #box was detached, #box's Element::recalcStyle invokes reattach() and only clears #box's childNeedsStyleRecalc. 

When the class attribute of <div> in sr2 is changed to be "selected", Node::setNeedsStyleRecalc is invoked and Node::markAncestorsWithChildNeedsStyleRecalc is invoked.

In Node::markAncestorsWithChildNeedsStyleRecalc, walk up from <div> and set childNeedsStyleRecalc until childNeedsStyleRecalc has been already set or no parent is found. i.e.

   <sr2> --> #item --> #box --> ... 

As #item's childNeedsStyleRecalc is not cleared, so document()'s childNeedsStyleRecalc is not updated.
So,

    if (document()->childNeedsStyleRecalc())
        document()->scheduleStyleRecalc();

Document::scheduleStyleRecalc is not invoked.

No re-layout will be executed. 

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 159102 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159102 [details]
Patch

Attachment 159102 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13521449

New failing tests:
editing/shadow/delete-characters-in-distributed-node-crash.html

[WebKit Review Bot]

Created attachment 159144 [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

[Dimitri Glazkov (Google)]

Comment on attachment 159102 [details]
Patch

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

I think Morrita-san should look over this. You're removing the logic that he's added :)

> Source/WebCore/dom/Element.cpp:-1001
> -        detachAsNode();

Is this the only callsite of this method? Perhaps we should remove the method too?

> Source/WebCore/dom/ElementShadow.cpp:-209
> -        host->lazyAttach(Node::DoNotSetAttached);

Ditto with DoNotSetAttached.

[WebKit Review Bot]

Comment on attachment 159102 [details]
Patch

Attachment 159102 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13531078

New failing tests:
editing/shadow/delete-characters-in-distributed-node-crash.html

[WebKit Review Bot]

Created attachment 159161 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Takashi Sakamoto]

Created attachment 159323 [details]
Patch

[Hajime Morrita]

Comment on attachment 159323 [details]
Patch

Looks sane. And code becomes more simpler.

[WebKit Review Bot]

Comment on attachment 159323 [details]
Patch

Rejecting attachment 159323 [details] from commit-queue.

New failing tests:
editing/shadow/delete-characters-in-distributed-node-crash.html
Full output: http://queues.webkit.org/results/13543188

[WebKit Review Bot]

Created attachment 159334 [details]
Archive of layout-test-results from gce-cq-04

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: gce-cq-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 159323 [details]
Patch

Attachment 159323 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13529831

New failing tests:
editing/shadow/delete-characters-in-distributed-node-crash.html

[WebKit Review Bot]

Created attachment 159337 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 159323 [details]
Patch

Attachment 159323 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13533716

New failing tests:
editing/shadow/delete-characters-in-distributed-node-crash.html

[WebKit Review Bot]

Created attachment 159346 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Takashi Sakamoto]

Comment on attachment 159102 [details]
Patch

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

>> Source/WebCore/dom/Element.cpp:-1001
>> -        detachAsNode();
> 
> Is this the only callsite of this method? Perhaps we should remove the method too?

I see. I removed.

>> Source/WebCore/dom/ElementShadow.cpp:-209
>> -        host->lazyAttach(Node::DoNotSetAttached);
> 
> Ditto with DoNotSetAttached.

Done.

[Takashi Sakamoto]

I looked at cq- log and found that the test, delete-characters-in-distributed-node-crash passes. However one more newline character is added to the actual text:

--- /tmp/layout-test-results/editing/shadow/delete-characters-in-distributed-node-crash-expected.txt 
+++ /tmp/layout-test-results/editing/shadow/delete-characters-in-distributed-node-crash-actual.txt 
@@ -1,4 +1,3 @@
 This tests the deletion of text in distributed node does not crash. To run it outside of DRT, you must delete text, 'foo', manually.
 
-
 PASS

I'm now thinking of how to fix this issue.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 159643 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159643 [details]
Patch

Attachment 159643 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13547571

New failing tests:
editing/shadow/delete-characters-in-distributed-node-crash.html

[WebKit Review Bot]

Created attachment 159712 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Takashi Sakamoto]

Created attachment 159823 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #22)
> Created an attachment (id=159823) [details]
> Patch

Since I heard that cq was restarted, I uploaded the same patch (locally the failed test passes) again.
However, I'm sorry. I forgot to update reviewer line.

If I obtain cq- again, I will modify test expectation and will fix the ChangeLog. If not, I will fix the ChangeLog.

Best regards,
Takashi Sakamoto

[WebKit Review Bot]

Comment on attachment 159823 [details]
Patch

Attachment 159823 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13566093

New failing tests:
editing/shadow/delete-characters-in-distributed-node-crash.html

[WebKit Review Bot]

Created attachment 159847 [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

[Takashi Sakamoto]

Created attachment 159853 [details]
Patch

[Shinya Kawanaka]

Comment on attachment 159853 [details]
Patch

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

> LayoutTests/platform/chromium/TestExpectations:3515
> +BUGWK92899 LINUX : editing/shadow/delete-characters-in-distributed-node-crash.html = PASS TEXT

I think it would be better to make the test more robust instead of writing test expectations here.

[Takashi Sakamoto]

Created attachment 159856 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #27)
> (From update of attachment 159853 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159853&action=review
> 
> > LayoutTests/platform/chromium/TestExpectations:3515
> > +BUGWK92899 LINUX : editing/shadow/delete-characters-in-distributed-node-crash.html = PASS TEXT
> 
> I think it would be better to make the test more robust instead of writing test expectations here.

Thank you for advice.
Done.

Best regards,
Takashi Sakamoto

[Shinya Kawanaka]

Comment on attachment 159856 [details]
Patch

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

> LayoutTests/editing/shadow/delete-characters-in-distributed-node-crash.html:16
> +document.getElementById("wrapper").innerHTML = "PASS";

It seems more robust than before. Good.

[Hajime Morrita]

Comment on attachment 159856 [details]
Patch

let's see how the bot takes this.

[WebKit Review Bot]

Comment on attachment 159856 [details]
Patch

Rejecting attachment 159856 [details] from commit-queue.

New failing tests:
editing/shadow/delete-characters-in-distributed-node-crash.html
Full output: http://queues.webkit.org/results/13557198

[WebKit Review Bot]

Created attachment 159867 [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

[WebKit Review Bot]

Comment on attachment 159856 [details]
Patch

Attachment 159856 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13561172

New failing tests:
editing/shadow/delete-characters-in-distributed-node-crash.html

[WebKit Review Bot]

Created attachment 159868 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Shinya Kawanaka]

Comment on attachment 159856 [details]
Patch

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

>> LayoutTests/editing/shadow/delete-characters-in-distributed-node-crash.html:16
>> +document.getElementById("wrapper").innerHTML = "PASS";
> 
> It seems more robust than before. Good.

Oh, you have to update the expected result, too.

[Takashi Sakamoto]

Created attachment 159873 [details]
Patch for landing

[Takashi Sakamoto]

(In reply to comment #36)
> (From update of attachment 159856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159856&action=review
> 
> >> LayoutTests/editing/shadow/delete-characters-in-distributed-node-crash.html:16
> >> +document.getElementById("wrapper").innerHTML = "PASS";
> > 
> > It seems more robust than before. Good.
> 
> Oh, you have to update the expected result, too.

Done.

Best regards,
Takashi Sakamoto

[WebKit Review Bot]

Comment on attachment 159873 [details]
Patch for landing

Clearing flags on attachment: 159873

Committed r126275: <http://trac.webkit.org/changeset/126275>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Strangely, the test added in this patch (fast/dom/shadow/shadowdom-dynamic-styling.html) fails on EFL port (we have SHADOW_DOM enabled):
-PASS window.getComputedStyle(target).backgroundColor is "rgb(255, 0, 0)"
+FAIL window.getComputedStyle(target).backgroundColor should be rgb(255, 0, 0). Was rgba(0, 0, 0, 0).

Bug 94690 was filed. If you have any idea of what the reason could be, help would be much appreciated.