113362 – [Custom Elements]: document.register doesn't upgrade nodes in document above the registering script tag

RESOLVED WONTFIX 113362

[Custom Elements]: document.register doesn't upgrade nodes in document above the registering script tag

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

Summary [Custom Elements]: document.register doesn't upgrade nodes in document above ...

Scott Miles

Reported 2013-03-26 19:50:37 PDT

Created attachment 195209 [details] Test document for above Given this construction: <x-foo></x-foo> <script> document.register('x-foo', ...); </script> <x-foo></x-foo> Only the second x-foo gets upgraded. The spec says that after document.register, 9. For DOCUMENT tree and every shadow DOM subtree enclosed by DOCUMENT tree: 1. Let TREE be this tree 2. Run element upgrade algorithm with TREE and DEFINITION as arguments I *think* that means both x-foos above should be upgraded.

Attachments
Test document for above (932 bytes, text/html) 2013-03-26 19:50 PDT, Scott Miles	no flags	Details
Test showing document.register upgrade issue (902 bytes, text/plain) 2013-03-26 19:52 PDT, Scott Miles	no flags	Details
Patch (21.40 KB, patch) 2013-03-27 02:54 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (21.45 KB, patch) 2013-03-27 03:44 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Addressed Adam's points (25.16 KB, patch) 2013-04-01 00:28 PDT, Hajime Morrita	dglazkov: review-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Scott Miles

Comment 1 2013-03-26 19:52:32 PDT

Created attachment 195210 [details] Test showing document.register upgrade issue Previous test said "results in console" when they are not.

Scott Miles

Comment 2 2013-03-26 19:54:02 PDT

Comment on attachment 195210 [details] Test showing document.register upgrade issue ><!DOCTYPE html> > ><html> > <head> > <title>CustomElements Workbench</title> > <meta charset="UTF-8"> > </head> > <body > <x-foo>XFoo One</x-foo> > <script> > XFoo = document.webkitRegister('x-foo', { > prototype: Object.create(HTMLElement.prototype, { > readyCallback: { > value: function() { > this.textContent += ' -- [upgraded]'; > } > }, > foo: { > value: function() { > this.style.cssText = 'border: 1px solid lightblue; padding: 8px;'; > } > } > }) > }); > window.addEventListener('load', function() { > xfoo.foo(); > }); > </script> > <x-foo id="xfoo">XFoo Two</x-foo> > </body> ></html>

Hajime Morrita

Comment 3 2013-03-26 23:14:37 PDT

Lifecycle callbacks should be also called https://www.w3.org/Bugs/Public/show_bug.cgi?id=21409

Hajime Morrita

Comment 4 2013-03-27 02:54:04 PDT

Created attachment 195254 [details] Patch

WebKit Review Bot

Comment 5 2013-03-27 03:19:53 PDT

Comment on attachment 195254 [details] Patch Attachment 195254 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17330173

WebKit Review Bot

Comment 6 2013-03-27 03:24:19 PDT

Comment on attachment 195254 [details] Patch Attachment 195254 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17314335

Hajime Morrita

Comment 7 2013-03-27 03:44:50 PDT

Created attachment 195265 [details] Patch

Dimitri Glazkov (Google)

Comment 8 2013-03-27 09:41:02 PDT

Comment on attachment 195265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195265&action=review Overall, the approach is solid. I have general angst over whether the spec is saying the right thing. > Source/WebCore/ChangeLog:32 > + document-register-upgrade.html would fail without this. This is interesting. I wonder if it would make sense to upgrade the element in such cases? > Source/WebCore/ChangeLog:36 > + - Elements created with valid custom element names (at creation timing), This is where I smell something wrong happening. The developers will get confused why an element is not custom. > Source/WebCore/dom/CustomElementRegistry.cpp:195 > + for (Node* node = root; node; node = NodeTraversal::next(node, root)) { This is going to be slow, but probably okay as the first iteration. In the future, I imagined storing a table of all custom element candidates so that we don't have to walk the tree... or something like that.

Hajime Morrita

Comment 9 2013-03-27 19:14:23 PDT

(In reply to comment #8) > (From update of attachment 195265 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=195265&action=review > > Overall, the approach is solid. I have general angst over whether the spec is saying the right thing. > > > Source/WebCore/ChangeLog:32 > > + document-register-upgrade.html would fail without this. > > This is interesting. I wonder if it would make sense to upgrade the element in such cases? > > > Source/WebCore/ChangeLog:36 > > + - Elements created with valid custom element names (at creation timing), > > This is where I smell something wrong happening. The developers will get confused why an element is not custom. One idea is that yet-to-be custom elements can get "infected" to a registered element definition once it is in the tree. But I believe such approach will introduce yet another complication. Anyway, this topic worth having more visible thread. > > > Source/WebCore/dom/CustomElementRegistry.cpp:195 > > + for (Node* node = root; node; node = NodeTraversal::next(node, root)) { > > This is going to be slow, but probably okay as the first iteration. In the future, I imagined storing a table of all custom element candidates so that we don't have to walk the tree... or something like that. Yup. I:d like to have such an optimization in separate change once things gets more stable.

Adam Barth

Comment 10 2013-03-30 11:17:39 PDT

Comment on attachment 195265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195265&action=review >>> Source/WebCore/dom/CustomElementRegistry.cpp:195 >>> + for (Node* node = root; node; node = NodeTraversal::next(node, root)) { >> >> This is going to be slow, but probably okay as the first iteration. In the future, I imagined storing a table of all custom element candidates so that we don't have to walk the tree... or something like that. > > Yup. I:d like to have such an optimization in separate change once things gets more stable. Don't we need an iteration pattern that avoids raw pointers? If the loop body triggers script execution, we can end up iterating over garbage. > Source/WebCore/dom/CustomElementRegistry.cpp:198 > + Element* element = toElement(node); Do we need to grab a RefPtr to element here? > Source/WebCore/dom/CustomElementRegistry.cpp:201 > + if (ElementShadow* shadow = element->shadow()) { > + for (ShadowRoot* shadowRoot = shadow->youngestShadowRoot(); shadowRoot; shadowRoot = shadowRoot->olderShadowRoot()) Same question about this weak iteration. > Source/WebCore/dom/CustomElementRegistry.cpp:205 > +} IMHO, it would be better to gather all the elements we need to upgrade in one pass and then upgrade them instead of intermixing the two operations.

Hajime Morrita

Comment 11 2013-03-31 21:09:14 PDT

Comment on attachment 195265 [details] Patch Adam, thanks for the attack! I'll come back here after I have clarified the behavior on the spec.

Hajime Morrita

Comment 12 2013-03-31 23:25:03 PDT

I posted my thinking here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=21485 The main point is that I prefer current description than other approach that I came up with. There might be better way though.

Hajime Morrita

Comment 13 2013-03-31 23:29:52 PDT

Comment on attachment 195265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195265&action=review >> Source/WebCore/ChangeLog:36 >> + - Elements created with valid custom element names (at creation timing), > > This is where I smell something wrong happening. The developers will get confused why an element is not custom. I think that this complication comes from the fact that we create wrappers lazily. The flag just emulates of custom element creation. If we create wrappers there immediately, the code will look straightforward.

Hajime Morrita

Comment 14 2013-04-01 00:28:18 PDT

Created attachment 195934 [details] Addressed Adam's points

Dimitri Glazkov (Google)

Comment 15 2013-04-01 10:27:26 PDT

Comment on attachment 195934 [details] Addressed Adam's points View in context: https://bugs.webkit.org/attachment.cgi?id=195934&action=review This is starting to come together. I am still not super-sure about the extra flag, but this discussion is progressing nicely on W3C bug. > Source/WebCore/dom/CustomElementRegistry.h:61 > + CreatedCallback, NeedsReadyCallback? > Source/WebCore/dom/CustomElementRegistry.h:62 > + Upgrade NeedsUpgrade? > Source/WebCore/dom/CustomElementRegistry.h:113 > + void didBecomeCustomElement(Element*, CustomElementConstructor*); didUpgradeElement? > Source/WebCore/dom/Element.h:598 > +#if ENABLE(DIALOG_ELEMENT) CUSTOM_ELEMENTS? > Source/WebCore/dom/ElementRareData.h:205 > +#if ENABLE(DIALOG_ELEMENT) CUSTOM_ELEMENTS?

Ryosuke Niwa

Comment 16 2015-10-16 00:48:23 PDT

The old implementation has been removed.

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

Status RESOLVED

Resolution WONTFIX

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component DOM

Assignee

Hajime Morrita

Reported

2013-03-26 19:50 PDT

Modified

2015-10-16 00:48 PDT History

CC List

12 users Show

URL

Keywords

Depends on

Blocks

99688

Dependencies

tree graph