Bug 113362

Summary: [Custom Elements]: document.register doesn't upgrade nodes in document above the registering script tag
Product: WebKit Reporter: Scott Miles <sjmiles>
Component: DOMAssignee: Hajime Morrita <morrita>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, cmarcelo, dglazkov, ericbidelman, esprehn+autocc, haraken, japhet, morrita, ojan.autocc, rniwa, webcomponents-bugzilla, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 99688    
Attachments:
Description Flags
Test document for above
none
Test showing document.register upgrade issue
none
Patch
none
Patch
none
Addressed Adam's points dglazkov: review-

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
DetailsFormatted DiffDiff
Patch (21.45 KB, patch)
2013-03-27 03:44 PDT, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Addressed Adam's points (25.16 KB, patch)
2013-04-01 00:28 PDT, Hajime Morrita 		dglazkov: review-
DetailsFormatted DiffDiff
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> ><!-- >Copyright 2013 The Toolkitchen Authors. All rights reserved. >Use of this source code is governed by a BSD-style >license that can be found in the LICENSE file. >--> ><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.