Reduce overhead from Custom Element constructors for elements inside HTML template nodes
Categories
(Toolkit :: UI Widgets, task, P3)
Tracking
()
People
(Reporter: Gijs, Assigned: emilio)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fxperf:p2])
Attachments
(3 files)
When I did some initial investigation in custom element overhead, I noticed that even when wrapping things in template tags to reduce cost/overhead, constructors are still run and so elements still show up in the instrumentation built-in to customElements.js .
We seem to do a few different things in the constructors:
- call
super() - initialize some field values to null / 0
- add event listeners (by and large, inline in the constructor)
- calling
this.attachShadow() - calling
this.initializeAttributeInheritance() - sometimes, loads of stuff (cf. https://searchfox.org/mozilla-central/source/toolkit/content/widgets/wizard.js#22 ; https://searchfox.org/mozilla-central/rev/c80fa7258c935223fe319c5345b58eae85d4c6ae/toolkit/content/widgets/arrowscrollbox.js#36 )
It seems like most of this work could happen in connectedCallback - certainly the event listeners for "normal" DOM events attached to the node itself will not fire before then. It's less clear to me off-hand how necessary some of the other work is, and how easily we could move it. Plenty of things in the tree call this.attachShadow() in connectedCallback - does that mean all bindings can? And, in principle, the default values could be required if consumers want to call methods on the node before appending it to the DOM - but in practice I wonder how often those fields are really needed before the node is in the DOM.
Of course, we'll need to work out how this interacts with delayConnectedCallback.
Unfortunately, we can't instrument constructors so it's unclear what the impact of all this work is...
|
Reporter | |
Updated•4 years ago
|
|
|
Reporter | |
Comment 1•4 years ago
|
||
Oh, and of course, if we decide to act here, we should investigate adding eslint rules to ensure we don't accidentally regress things.
|
||
Updated•4 years ago
|
|
|
||
Comment 2•4 years ago
|
||
Is this expected behavior? I was surprised to find out that constructors on CEs run when the CEs are in a <template> but before they are appended elsewhere. We've been preferring to insert Shadow DOM in the constructor to simplify things (so we don't have to handle the state where the DOM isn't inserted if we wait for connectedCallback to insert). But this might be enough reason to go back to connectedCallback for perf.
|
Assignee | |
Comment 3•4 years ago
|
||
can you post a test-case that shows the issue? I have tried something simple like:
<!doctype html>
<script>
customElements.define("x-foo", class extends HTMLElement {
constructor() {
super();
console.log("woo");
}
});
onload = function() {
document.body.innerHTML = `
<template>
<x-foo></x-foo>
</template>
`;
}
</script>
<template>
<x-foo></x-foo>
</template>
And I don't see any constructor logged.
|
|
Assignee | |
Comment 4•4 years ago
|
||
Or are you talking about template as in MozXULElement.parseXULToFragment?
|
|
||
Comment 5•4 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
Or are you talking about template as in
MozXULElement.parseXULToFragment?
I'm pretty sure this was a <template> tag in the DOM in browser.xhtml. Gijs had it pulled up locally so I'll redirect to him to see if he can still reproduce and/or provide a patch to demo it.
|
|
Reporter | |
Comment 6•4 years ago
•
|
||
Sorry for the delay. STR:
- open toolkit/content/widgets/panel.js, and add
console.error(this)into the constructor (alt: apply this patch) ./mach build faster && ./mach run- open the browser console
- note the entry for editBookmarkPanel - I can't figure out how to make it conditional on just this entry because
this.idis undefined, and there are no attributes forthis, when the constructor fires; it's the third one logged for me, YMMV
JS stack:
MozPanel chrome://global/content/elements/panel.js:13
(Async: CustomElementConstructor)
<anonymous> chrome://global/content/elements/panel.js:319
<anonymous> chrome://global/content/customElements.js:797
<anonymous> chrome://global/content/customElements.js:820
observe resource://gre/modules/CustomElementsListener.jsm:24
Note that this element is inside a template tag .
|
|
Assignee | |
Comment 7•4 years ago
|
||
So this comes from the XUL prototype document stuff. This is a bit of an issue: The prototype document constructs a template > popup, instead of a template having a fragment belonging to another document, which is how template usually works (and think is why constructors don't run in a <template> in regular HTML documents).
|
|
Assignee | |
Comment 9•4 years ago
|
||
This matches closer the behavior of the HTML parser, which doesn't run custom
element constructors, among other things.
|
||
Updated•4 years ago
|
|
||
Comment 10•4 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/14eb5adc6cd9 Use the right document to create elements from the prototype document. r=smaug
|
||
Comment 11•4 years ago
|
||
Backed out changeset 14eb5adc6cd9 (bug 1624657) for browser-chrome failures at browser/modules/test/browser/browser_UsageTelemetry_searchbar.js
Backout: https://hg.mozilla.org/integration/autoland/rev/9951ef319e127b148f0b9a71e4894536e6335dad
Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=14eb5adc6cd9917b3c90d8994d5518b17fa0a7a1
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296298565&repo=autoland&lineNumber=26909
|
|
Assignee | |
Comment 12•4 years ago
|
||
My patch causes an interesting regression. We stop constructing custom elements
for the stuff inside the edit-bookmark-panel as expected. This means that the
autocomplete-input here:
Doesn't cause autocomplete-input.js to load. That means that it'll load by the
time the searchbar-textbox autocomplete is created, which means that it'll get
upgraded asynchronously. So far so good. However there's an interesting race due
to the way keypress event listeners are setup in autocomplete-input.js. In
particular, it uses the capture phase and the system group:
This means it'll race with the other native / system group event listeners that
get added by the editor component. This caused arrow-up / arrow-down to not work
on the searchbar because the custom element was upgraded async.
This kinda papers over the issue the same way editBookmarkPanel was papering
over it, but ideally we should figure out a way to not use the system group or
keypress there so that we don't race in this awful way. Separate bug, probably.
|
|
Assignee | |
Updated•4 years ago
|
|
|
||
Comment 13•4 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cb3871ec3aa0 Eagerly load autocomplete-input custom element. r=Gijs
|
|
||
Comment 14•4 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cb2f5027a9e6 Use the right document to create elements from the prototype document. r=smaug
|
||
Updated•4 years ago
|
|
||
Comment 15•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/cb3871ec3aa0
https://hg.mozilla.org/mozilla-central/rev/cb2f5027a9e6
|
||
Updated•4 years ago
|
Description
•