Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add customElements.upgrade #710

Closed
rniwa opened this issue Nov 13, 2017 · 11 comments
Closed

Add customElements.upgrade #710

rniwa opened this issue Nov 13, 2017 · 11 comments
Assignees
@domenic
Labels
custom-elements needs concrete proposal

Comments

@rniwa
Copy link
Collaborator

rniwa commented Nov 13, 2017

We need a mechanism to trigger an upgrade on a disconnected tree of nodes per in-persion discussions with @domenic and @justinfagnani
@rniwa
Copy link
Collaborator Author

rniwa commented Nov 13, 2017

Also see #419 (comment)

@justinfagnani
Copy link
Contributor

Additional thought:

If we get something like CustomElementRegistry#upgrade(rootNode), would it be interesting to think about a way to block automatic upgrade of a tree due to registrations? The motivation is when you know you're about to define a large number of elements and want to do upgrading in a single batch for performance.

We've had various requests for controlling upgrades come in from customers, but @sorvell would have more detail.

@domenic
Copy link
Collaborator

domenic commented Nov 14, 2017

Argh, let's not scope-creep this, or it won't happen :(. We already have to figure out deep vs. not...

@domenic domenic self-assigned this Nov 14, 2017
@rniwa
Copy link
Collaborator Author

rniwa commented Nov 14, 2017

I think deep makes more sense than shallow. It's almost never useful to upgrade just the root and not its descendent.

@bkardell
Copy link

Is there actually a use case for shallow even?

@snuggs
Copy link

snuggs commented Nov 14, 2017

@bkardell @rniwa is there any way you can provide a code sample of shallow vs deep?
In our implementation to "upgrade" is merely prototype swizzling and dispatching connectedCallback(). A subset of the entire spec but suits our minimalistic needs.

https://github.com/devpunks/snuggsi/blob/master/custom-element-registry/index.es#L30-L45

Just trying to understand if you mean the following:

<foo-bar>
  <baz-bat>Who is responsible for my upgrade?</baz-bat>
</foo-bar>

@annevk
Copy link
Collaborator

annevk commented Nov 14, 2017

@snuggs the method is given a node. The question is then whether you just upgrade that node, or that node and also all its descendants. So with your example, if you pass in foo-bar, is baz-bat upgraded or not?

@snuggs
Copy link

snuggs commented Nov 14, 2017 •
edited
Loading

@annevk Thanks for the clarity! Ran into this use case with a client. The use case was foo-bar definition blocking head. while baz-bat was defered in definition. Wound up causing strange race conditions with the upgrade process. We settled on covering all mutually exclusive positions:

  1. parent defined before child.
  2. child defined before parent.

What worked was wherever the script is declared (pre/post body) the algo is this:

  1. Collect all custom elements up to point and upgrade.
  2. (Always) upgrade upon DOM insertion of a custom element.
  3. Whether upgrade is successful or not fails silently and doesn't throw to the call site.

3 is where the specs would usually 💥 but we were more lackadaisical.

To be clear this isn't an encouragement towards the spec. More trying to abide with implementation aligned as close to what we are doing here while underlying vendor implementation evolves.

Super looking forward to movement in this space. Many of these problems have been solving for the past year or two. Love seeing this be revisited.

@TakayoshiKochi TakayoshiKochi mentioned this issue Feb 22, 2018
Open
4 tasks
@annevk annevk mentioned this issue Mar 5, 2018
Closed
domenic added a commit to whatwg/html that referenced this issue Mar 6, 2018
@domenic
Add customElements.upgrade()
5e3d064 
Closes WICG/webcomponents#710.
@domenic domenic mentioned this issue Mar 6, 2018
Merged
domenic added a commit to whatwg/html that referenced this issue Mar 6, 2018
@domenic
Add customElements.upgrade()
17eec98 
Closes WICG/webcomponents#710.
domenic added a commit to whatwg/html that referenced this issue Mar 7, 2018
@domenic
Add customElements.upgrade()
6df4863 
Closes WICG/webcomponents#710.
@snuggs snuggs mentioned this issue Mar 15, 2018
Merged
29 tasks
hubot pushed a commit to WebKit/WebKit-http that referenced this issue Aug 2, 2018
Implement customElements.upgrade()
2ee15a4 
https://bugs.webkit.org/show_bug.cgi?id=183397

Reviewed by Frédéric Wang.

LayoutTests/imported/w3c:

Rebaseline the test now that we're passing.

* web-platform-tests/custom-elements/custom-element-registry/upgrade-expected.txt:

Source/WebCore:

Added the support to upgrade custom elements directly. Ordinarily, custom elements get upgraded as they are
inserted / connected into a document but some script libraries and authors want to be able to upgrade them before that.
Also see WICG/webcomponents#710

Implemented the method as specified at:
https://html.spec.whatwg.org/multipage/custom-elements.html#dom-customelementregistry-upgrade

    When invoked, the upgrade(root) method must run these steps:
    1. Let candidates be a list of all of root's shadow-including inclusive descendant elements,
       in shadow-including tree order.
    2. For each candidate of candidates, try to upgrade candidate.

Tests: imported/w3c/web-platform-tests/custom-elements/custom-element-registry/upgrade.html

* dom/CustomElementReactionQueue.cpp:
(WebCore::CustomElementReactionQueue::enqueueElementUpgradeIfDefined): Removed the assertion that the upgraded element
is connected since the whole point of this API is to upgrade a disconnected element.
* dom/CustomElementRegistry.cpp:
(WebCore::upgradeElementsInShadowIncludingdescendants): Added.
(WebCore::CustomElementRegistry::upgrade): Added.
* dom/CustomElementRegistry.h: Forward declare DeferredPromise instead of unnecessarily including JSDOMPromiseDeferred.h.
* dom/CustomElementRegistry.idl:
* dom/Element.cpp:
(WebCore::Element::insertedIntoAncestor): Moved the assertion here.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@234507 268f45cc-cd09-0410-ab3c-d52691b4dbfc
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
@domenic
Add customElements.upgrade()
ba4ce4c 
Closes WICG/webcomponents#710.
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
@rniwa
Implement customElements.upgrade()
b0c176d 
https://bugs.webkit.org/show_bug.cgi?id=183397

Reviewed by Frédéric Wang.

LayoutTests/imported/w3c:

Rebaseline the test now that we're passing.

* web-platform-tests/custom-elements/custom-element-registry/upgrade-expected.txt:

Source/WebCore:

Added the support to upgrade custom elements directly. Ordinarily, custom elements get upgraded as they are
inserted / connected into a document but some script libraries and authors want to be able to upgrade them before that.
Also see WICG/webcomponents#710

Implemented the method as specified at:
https://html.spec.whatwg.org/multipage/custom-elements.html#dom-customelementregistry-upgrade

    When invoked, the upgrade(root) method must run these steps:
    1. Let candidates be a list of all of root's shadow-including inclusive descendant elements,
       in shadow-including tree order.
    2. For each candidate of candidates, try to upgrade candidate.

Tests: imported/w3c/web-platform-tests/custom-elements/custom-element-registry/upgrade.html

* dom/CustomElementReactionQueue.cpp:
(WebCore::CustomElementReactionQueue::enqueueElementUpgradeIfDefined): Removed the assertion that the upgraded element
is connected since the whole point of this API is to upgrade a disconnected element.
* dom/CustomElementRegistry.cpp:
(WebCore::upgradeElementsInShadowIncludingdescendants): Added.
(WebCore::CustomElementRegistry::upgrade): Added.
* dom/CustomElementRegistry.h: Forward declare DeferredPromise instead of unnecessarily including JSDOMPromiseDeferred.h.
* dom/CustomElementRegistry.idl:
* dom/Element.cpp:
(WebCore::Element::insertedIntoAncestor): Moved the assertion here.


Canonical link: https://commits.webkit.org/203374@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@234507 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@GeorgeTailor
Copy link

Hello @justinfagnani,
is there a tracking issue somewhere for your proposition? Such behavior would greatly increase performance when using a lot of the same custom element in a page.

@justinfagnani
Copy link
Contributor

@GeorgeTailor We don't have a specific separate issue that I know of, though the idea of batched definitions came up briefly in the scoped custom element registry discussions.

Note that batching won't have any impact on multiple instances of the same definition - it might have an impact on many separate definitions though by avoiding multiple tree walks.

@rniwa rniwa mentioned this issue Apr 16, 2021
Open
@rniwa
Copy link
Collaborator Author

rniwa commented Apr 16, 2021

@GeorgeTailor We don't have a specific separate issue that I know of, though the idea of batched definitions came up briefly in the scoped custom element registry discussions.

Filed #922. Please continue the discussion there of using this closed issue for adding customElements.upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@domenic domenic

Labels
custom-elements needs concrete proposal
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@snuggs @rniwa @justinfagnani @domenic @bkardell @annevk @GeorgeTailor