Closed Bug 1624657 Opened 4 months ago Closed 4 months ago

Reduce overhead from Custom Element constructors for elements inside HTML template nodes

Categories

(Toolkit :: XUL Widgets, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox76 --- wontfix
firefox77 --- fixed

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:

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...

Blocks: 1515799
Summary: Investigate reducing overhead from Custom Element constructors for elements inside HTML template nodes → Reduce overhead from Custom Element constructors for elements inside HTML template nodes

Oh, and of course, if we decide to act here, we should investigate adding eslint rules to ensure we don't accidentally regress things.

Whiteboard: [fxperf]
Type: defect → task
Priority: -- → P3

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.

Flags: needinfo?(emilio)

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.

Flags: needinfo?(emilio) → needinfo?(bgrinstead)

Or are you talking about template as in MozXULElement.parseXULToFragment?

(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.

Flags: needinfo?(bgrinstead) → needinfo?(gijskruitbosch+bugs)

Sorry for the delay. STR:

  1. open toolkit/content/widgets/panel.js, and add console.error(this) into the constructor (alt: apply this patch)
  2. ./mach build faster && ./mach run
  3. open the browser console
  4. note the entry for editBookmarkPanel - I can't figure out how to make it conditional on just this entry because this.id is undefined, and there are no attributes for this, 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 .

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)
Attached file Stack

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).

Flags: needinfo?(emilio)

Ah, I guess this is supposed to deal with this.

This matches closer the behavior of the HTML parser, which doesn't run custom
element constructors, among other things.

Assignee: nobody → emilio
Status: NEW → ASSIGNED
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

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:

https://searchfox.org/mozilla-central/rev/3f9e822318e8ec18ce673a9cb983d3608a3e2ed2/browser/components/places/content/editBookmarkPanel.inc.xhtml#81

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:

https://searchfox.org/mozilla-central/rev/3f9e822318e8ec18ce673a9cb983d3608a3e2ed2/toolkit/content/widgets/autocomplete-input.js#38

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.

Flags: needinfo?(emilio)
Blocks: 1627520
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cb3871ec3aa0
Eagerly load autocomplete-input custom element. r=Gijs
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
Whiteboard: [fxperf] → [fxperf:p2]
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
You need to log in before you can comment on or make changes to this bug.