Bug 92899 - Dynamically styling ShadowDom content on a node distributed to another shadow insertion point fails.
Summary: Dynamically styling ShadowDom content on a node distributed to another shadow...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.7
: P2 Normal
Assignee: Takashi Sakamoto
URL:
Keywords: HasReduction
Depends on:
Blocks: 72352
  Show dependency treegraph
 
Reported: 2012-08-01 12:18 PDT by Steve Orvell
Modified: 2012-09-10 06:23 PDT (History)
9 users (show)

See Also:


Attachments
Test case for bug, including expected rendering. (1.36 KB, text/html)
2012-08-01 12:18 PDT, Steve Orvell
no flags Details
WIP (1.56 KB, patch)
2012-08-06 04:11 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (5.50 KB, patch)
2012-08-17 05:33 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (404.82 KB, application/zip)
2012-08-17 09:52 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-07 (437.11 KB, application/zip)
2012-08-17 10:52 PDT, WebKit Review Bot
no flags Details
Patch (6.51 KB, patch)
2012-08-19 20:49 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cq-04 (378.37 KB, application/zip)
2012-08-19 22:32 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-07 (566.91 KB, application/zip)
2012-08-19 23:00 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-06 (555.58 KB, application/zip)
2012-08-19 23:56 PDT, WebKit Review Bot
no flags Details
Patch (6.50 KB, patch)
2012-08-21 03:30 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (581.68 KB, application/zip)
2012-08-21 10:06 PDT, WebKit Review Bot
no flags Details
Patch (6.51 KB, patch)
2012-08-21 17:53 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (367.01 KB, application/zip)
2012-08-21 19:51 PDT, WebKit Review Bot
no flags Details
Patch (7.53 KB, patch)
2012-08-21 20:49 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (8.08 KB, patch)
2012-08-21 21:22 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cq-02 (621.56 KB, application/zip)
2012-08-21 23:26 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-02 (495.09 KB, application/zip)
2012-08-21 23:29 PDT, WebKit Review Bot
no flags Details
Patch for landing (8.66 KB, patch)
2012-08-21 23:54 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Orvell 2012-08-01 12:18:57 PDT
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.
Comment 1 Takashi Sakamoto 2012-08-06 04:11:42 PDT
Created attachment 156653 [details]
WIP
Comment 2 Takashi Sakamoto 2012-08-06 04:28:23 PDT
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
Comment 3 Takashi Sakamoto 2012-08-17 05:33:29 PDT
Created attachment 159102 [details]
Patch
Comment 4 WebKit Review Bot 2012-08-17 09:52:55 PDT
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
Comment 5 WebKit Review Bot 2012-08-17 09:52:58 PDT
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
Comment 6 Dimitri Glazkov (Google) 2012-08-17 09:53:20 PDT
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.
Comment 7 WebKit Review Bot 2012-08-17 10:52:03 PDT
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
Comment 8 WebKit Review Bot 2012-08-17 10:52:23 PDT
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
Comment 9 Takashi Sakamoto 2012-08-19 20:49:40 PDT
Created attachment 159323 [details]
Patch
Comment 10 Hajime Morrita 2012-08-19 21:00:55 PDT
Comment on attachment 159323 [details]
Patch

Looks sane. And code becomes more simpler.
Comment 11 WebKit Review Bot 2012-08-19 22:32:43 PDT
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
Comment 12 WebKit Review Bot 2012-08-19 22:32:47 PDT
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
Comment 13 WebKit Review Bot 2012-08-19 23:00:51 PDT
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
Comment 14 WebKit Review Bot 2012-08-19 23:00:54 PDT
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
Comment 15 WebKit Review Bot 2012-08-19 23:56:46 PDT
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
Comment 16 WebKit Review Bot 2012-08-19 23:56:50 PDT
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
Comment 17 Takashi Sakamoto 2012-08-20 02:27:33 PDT
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.
Comment 18 Takashi Sakamoto 2012-08-20 02:37:14 PDT
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
Comment 19 Takashi Sakamoto 2012-08-21 03:30:48 PDT
Created attachment 159643 [details]
Patch
Comment 20 WebKit Review Bot 2012-08-21 10:06:53 PDT
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
Comment 21 WebKit Review Bot 2012-08-21 10:06:58 PDT
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
Comment 22 Takashi Sakamoto 2012-08-21 17:53:07 PDT
Created attachment 159823 [details]
Patch
Comment 23 Takashi Sakamoto 2012-08-21 18:56:06 PDT
(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
Comment 24 WebKit Review Bot 2012-08-21 19:51:45 PDT
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
Comment 25 WebKit Review Bot 2012-08-21 19:51:48 PDT
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
Comment 26 Takashi Sakamoto 2012-08-21 20:49:31 PDT
Created attachment 159853 [details]
Patch
Comment 27 Shinya Kawanaka 2012-08-21 21:03:38 PDT
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.
Comment 28 Takashi Sakamoto 2012-08-21 21:22:07 PDT
Created attachment 159856 [details]
Patch
Comment 29 Takashi Sakamoto 2012-08-21 21:23:07 PDT
(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
Comment 30 Shinya Kawanaka 2012-08-21 21:24:30 PDT
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.
Comment 31 Hajime Morrita 2012-08-21 21:34:25 PDT
Comment on attachment 159856 [details]
Patch

let's see how the bot takes this.
Comment 32 WebKit Review Bot 2012-08-21 23:26:00 PDT
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
Comment 33 WebKit Review Bot 2012-08-21 23:26:04 PDT
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
Comment 34 WebKit Review Bot 2012-08-21 23:29:23 PDT
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
Comment 35 WebKit Review Bot 2012-08-21 23:29:27 PDT
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
Comment 36 Shinya Kawanaka 2012-08-21 23:39:53 PDT
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.
Comment 37 Takashi Sakamoto 2012-08-21 23:54:34 PDT
Created attachment 159873 [details]
Patch for landing
Comment 38 Takashi Sakamoto 2012-08-21 23:55:45 PDT
(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
Comment 39 WebKit Review Bot 2012-08-22 01:20:17 PDT
Comment on attachment 159873 [details]
Patch for landing

Clearing flags on attachment: 159873

Committed r126275: <http://trac.webkit.org/changeset/126275>
Comment 40 WebKit Review Bot 2012-08-22 01:20:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 41 Chris Dumez 2012-09-10 06:23:30 PDT
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.