87815 – [Shadow DOM] ShadowRoot.getElementById() should work outside document

RESOLVED FIXED 87815

[Shadow DOM] ShadowRoot.getElementById() should work outside document

https://bugs.webkit.org/show_bug.cgi?id=87815

Summary [Shadow DOM] ShadowRoot.getElementById() should work outside document

Hajime Morrita

Reported 2012-05-29 21:04:08 PDT

Currently getElementById() doesn't work with ShadowRoot which is attached to orphan host. But it should work.

Attachments
Modify insertedInto (5.85 KB, patch) 2012-12-05 22:10 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (7.23 KB, patch) 2012-12-05 23:02 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (7.76 KB, patch) 2012-12-06 01:34 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (8.11 KB, patch) 2012-12-06 02:51 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (8.09 KB, patch) 2012-12-06 16:22 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
WIP (24.11 KB, patch) 2012-12-12 21:04 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
WIP (24.95 KB, patch) 2012-12-12 22:59 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
WIP (24.26 KB, patch) 2012-12-13 02:14 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch (12.03 KB, patch) 2012-12-13 23:10 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Performance Results (first 2 is without this patch, last 2 is with this patch) (24.06 KB, text/html) 2012-12-13 23:13 PST, Shinya Kawanaka	no flags	Details
Patch (12.15 KB, patch) 2012-12-13 23:17 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch for landing (12.21 KB, patch) 2012-12-13 23:35 PST, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Shinya Kawanaka

Comment 1 2012-12-05 18:47:58 PST

At the first point, it might be OK to search everything, I think. We could enhance the performance later.

Shinya Kawanaka

Comment 2 2012-12-05 22:10:07 PST

Created attachment 177932 [details] Modify insertedInto

Shinya Kawanaka

Comment 3 2012-12-05 22:12:04 PST

This version has performance regression by 2~3% on Dromaeo/dom-modify.html 35.98 runs/s -> 35.10 runs/s I don't think this is acceptable. Let me consider another approach.

Shinya Kawanaka

Comment 4 2012-12-05 23:02:18 PST

Created attachment 177946 [details] Patch

Hajime Morrita

Comment 5 2012-12-05 23:27:57 PST

Comment on attachment 177946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177946&action=review > Source/WebCore/dom/TreeScope.cpp:103 > + if (Element* element = m_elementsById ? m_elementsById->getElementById(elementId.impl(), this) : 0) Can we have this in ShadowRoot.cpp ? It would be cleaner.

Shinya Kawanaka

Comment 6 2012-12-06 01:05:48 PST

Comment on attachment 177946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177946&action=review >> Source/WebCore/dom/TreeScope.cpp:103 >> + if (Element* element = m_elementsById ? m_elementsById->getElementById(elementId.impl(), this) : 0) > > Can we have this in ShadowRoot.cpp ? > It would be cleaner. I think m_elementsById should not be exposed to ShadowRoot. The other things can be moved to ShadowRoot.cpp.

Hajime Morrita

Comment 7 2012-12-06 01:12:31 PST

> > It would be cleaner. > > I think m_elementsById should not be exposed to ShadowRoot. The other things can be moved to ShadowRoot.cpp. Cannot we just call TreeScope::getElementById() for non-orphan case?

Shinya Kawanaka

Comment 8 2012-12-06 01:23:50 PST

(In reply to comment #7) > > > It would be cleaner. > > > > I think m_elementsById should not be exposed to ShadowRoot. The other things can be moved to ShadowRoot.cpp. > > Cannot we just call TreeScope::getElementById() for non-orphan case? Ah, you mean we should create ShadowRoot::getElementById(), right? That makes sense.

Shinya Kawanaka

Comment 9 2012-12-06 01:34:51 PST

Created attachment 177970 [details] Patch

Build Bot

Comment 10 2012-12-06 01:42:25 PST

Comment on attachment 177970 [details] Patch Attachment 177970 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15181031

Shinya Kawanaka

Comment 11 2012-12-06 01:50:33 PST

Hmm... instead of exposing the symbol, I would like to kill Internals.getElementByIdShadowRoot. It's nonsense now.

Hajime Morrita

Comment 12 2012-12-06 01:53:00 PST

Comment on attachment 177970 [details] Patch Could you add test like this? - Append child during the shadow is in the document, - then remove the host from document, - and finallly exercise against that now-orphan shadow root.

Shinya Kawanaka

Comment 13 2012-12-06 02:01:40 PST

(In reply to comment #12) > (From update of attachment 177970 [details]) > Could you add test like this? > > - Append child during the shadow is in the document, > - then remove the host from document, > - and finallly exercise against that now-orphan shadow root. OK.

Build Bot

Comment 14 2012-12-06 02:43:15 PST

Comment on attachment 177970 [details] Patch Attachment 177970 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15175180

Shinya Kawanaka

Comment 15 2012-12-06 02:51:25 PST

Created attachment 177982 [details] Patch

Shinya Kawanaka

Comment 16 2012-12-06 02:51:58 PST

This patch can be built after https://bugs.webkit.org/show_bug.cgi?id=104241 is resolved.

Build Bot

Comment 17 2012-12-06 03:02:04 PST

Comment on attachment 177982 [details] Patch Attachment 177982 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15154820

Build Bot

Comment 18 2012-12-06 03:35:52 PST

Comment on attachment 177982 [details] Patch Attachment 177982 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15144929

Shinya Kawanaka

Comment 19 2012-12-06 16:22:44 PST

Created attachment 178102 [details] Patch

Kent Tamura

Comment 20 2012-12-06 19:52:58 PST

(In reply to comment #3) > This version has performance regression by 2~3% on Dromaeo/dom-modify.html > > 35.98 runs/s -> 35.10 runs/s > > I don't think this is acceptable. Was this caused by Bug 87034?

Shinya Kawanaka

Comment 21 2012-12-06 20:03:17 PST

(In reply to comment #20) > (In reply to comment #3) > > This version has performance regression by 2~3% on Dromaeo/dom-modify.html > > > > 35.98 runs/s -> 35.10 runs/s > > > > I don't think this is acceptable. > > Was this caused by Bug 87034? I'm not 100% sure, but since insertedInto() is very host function, I'm thinking that adding if-condition can regress dom-modify performance. At least, I tested the first version of the patch after Bug 87034 has been resolved. So I don't think such regression is mainly caused by the slowness of treeScope(). # If treeScope() is still too slow, I'll change my mind.

Dimitri Glazkov (Google)

Comment 22 2012-12-07 10:52:14 PST

This seems like a workaround for a bigger issue: the inDocument is not expressive enough to communicate the state of the tree. In terms of shadow trees, an element in that tree should always seem inDocument, even if it's hosted by an orphan element. However, this means that we may have a situation that the root is inDocument, but the host is not. That's bad. Customizing getElementById for ShadowRoot is just masking this issue. Let's fix the real problem :)

Hajime Morrita

Comment 23 2012-12-09 16:01:52 PST

(In reply to comment #22) > This seems like a workaround for a bigger issue: the inDocument is not expressive enough to communicate the state of the tree. > > In terms of shadow trees, an element in that tree should always seem inDocument, even if it's hosted by an orphan element. However, this means that we may have a situation that the root is inDocument, but the host is not. That's bad. > > Customizing getElementById for ShadowRoot is just masking this issue. Let's fix the real problem :) I agree this, but the correct fix will take longer. My suggestion here is that we could have workaround now and fix it since making Shadow DOM available without making this completely broken will be a disaster. We could possibly change title of Bug 104223 for let the problem visible.

Shinya Kawanaka

Comment 24 2012-12-12 17:57:19 PST

My current plan is to introduce InShadowTreeFlag and make have a method isInTreeScope() (name should be reconsidered) which returns "inDocument() || isInShadowTree()" (actually we use bit-and to return it). Also, I'll move setInDocument()/clearInDocument() from insertedInto()/removedFrom() to ContainerNodeAlgorithm. I'm thinking this doesn't impact performance

Shinya Kawanaka

Comment 25 2012-12-12 21:04:45 PST

Created attachment 179193 [details] WIP

Hajime Morrita

Comment 26 2012-12-12 21:21:59 PST

Comment on attachment 179193 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=179193&action=review > Source/WebCore/dom/ContainerNodeAlgorithms.h:63 > + void notifyDescendantRemovedFromDocument(ContainerNode*, Node::TreeScopeTypes); You could define another enum here for clarity. It's really confusing to see the flag name to be cleared here. Let's give more intention revealing name.

Shinya Kawanaka

Comment 27 2012-12-12 22:22:51 PST

The last WIP patch regresses performance by 0.5%. (37.71runs/s -> 37.45 runs/s, Dromaeo/dom-modify.html) It would be acceptable, but let me try to improve more.

WebKit Review Bot

Comment 28 2012-12-12 22:40:33 PST

Comment on attachment 179193 [details] WIP Attachment 179193 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15321098 New failing tests: fast/forms/radio/radio-group.html

Shinya Kawanaka

Comment 29 2012-12-12 22:59:27 PST

Created attachment 179204 [details] WIP

Shinya Kawanaka

Comment 30 2012-12-12 23:00:39 PST

This does not regress performance anymore. I would like make code cleaner.

Shinya Kawanaka

Comment 31 2012-12-13 02:12:50 PST

It seems this still regress Parser/html5-full-render.html performance. Ohya

Shinya Kawanaka

Comment 32 2012-12-13 02:14:38 PST

Created attachment 179234 [details] WIP

WebKit Review Bot

Comment 33 2012-12-13 03:52:42 PST

Comment on attachment 179234 [details] WIP Attachment 179234 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15322171 New failing tests: fast/forms/radio/radio-group.html

Build Bot

Comment 34 2012-12-13 04:43:34 PST

Comment on attachment 179234 [details] WIP Attachment 179234 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15311216

WebKit Review Bot

Comment 35 2012-12-13 04:55:26 PST

Comment on attachment 179234 [details] WIP Attachment 179234 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15323212 New failing tests: fast/forms/radio/radio-group.html

Shinya Kawanaka

Comment 36 2012-12-13 23:10:41 PST

Created attachment 179423 [details] Patch

Shinya Kawanaka

Comment 37 2012-12-13 23:13:29 PST

Created attachment 179424 [details] Performance Results (first 2 is without this patch, last 2 is with this patch)

Shinya Kawanaka

Comment 38 2012-12-13 23:17:50 PST

Created attachment 179425 [details] Patch

Hajime Morrita

Comment 39 2012-12-13 23:24:50 PST

Comment on attachment 179425 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179425&action=review It's surprising to see this doesn't hurt perf. But well, that happens. Wait for bots beging green. > Source/WebCore/dom/Node.cpp:2098 > + if (parentNode() && parentNode()->isInShadowTree()) You can use parentOrHostNode() here. It will be a bit faster.

Shinya Kawanaka

Comment 40 2012-12-13 23:32:38 PST

(In reply to comment #39) > (From update of attachment 179425 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179425&action=review > > It's surprising to see this doesn't hurt perf. But well, that happens. Wait for bots beging green. > > > Source/WebCore/dom/Node.cpp:2098 > > + if (parentNode() && parentNode()->isInShadowTree()) > > You can use parentOrHostNode() here. It will be a bit faster. Good point. Thanks!

Shinya Kawanaka

Comment 41 2012-12-13 23:35:49 PST

Created attachment 179428 [details] Patch for landing

WebKit Review Bot

Comment 42 2012-12-14 01:24:25 PST

Comment on attachment 179428 [details] Patch for landing Clearing flags on attachment: 179428 Committed r137731: <http://trac.webkit.org/changeset/137731>

WebKit Review Bot

Comment 43 2012-12-14 01:24:32 PST

All reviewed patches have been landed. Closing bug.

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component DOM

Assignee

Shinya Kawanaka

Reported

2012-05-29 21:04 PDT

Modified

2012-12-14 01:24 PST History

CC List

11 users Show

URL

Keywords

Depends on

104241

Blocks

72352 104223

Dependencies

tree graph