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-

Description Scott Miles 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.
Comment 1 Scott Miles 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.
Comment 2 Scott Miles 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>
Comment 3 Hajime Morrita 2013-03-26 23:14:37 PDT
Lifecycle callbacks should be also called
https://www.w3.org/Bugs/Public/show_bug.cgi?id=21409
Comment 4 Hajime Morrita 2013-03-27 02:54:04 PDT
Created attachment 195254 [details]
Patch
Comment 5 WebKit Review Bot 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
Comment 6 WebKit Review Bot 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
Comment 7 Hajime Morrita 2013-03-27 03:44:50 PDT
Created attachment 195265 [details]
Patch
Comment 8 Dimitri Glazkov (Google) 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.
Comment 9 Hajime Morrita 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.
Comment 10 Adam Barth 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.
Comment 11 Hajime Morrita 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.
Comment 12 Hajime Morrita 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.
Comment 13 Hajime Morrita 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.
Comment 14 Hajime Morrita 2013-04-01 00:28:18 PDT
Created attachment 195934 [details]
Addressed Adam's points
Comment 15 Dimitri Glazkov (Google) 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?
Comment 16 Ryosuke Niwa 2015-10-16 00:48:23 PDT
The old implementation has been removed.