Fix custom elements setup and teardown#3841
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3841 +/- ##
=======================================
Coverage 98.06% 98.06%
=======================================
Files 322 322
Lines 8530 8530
=======================================
Hits 8365 8365
Misses 165 165 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tvdeyen
force-pushed
the
fix-custom-elements-cloneable
branch
5 times, most recently
from
afc7ad0 to
0c01f20
Compare
tvdeyen
changed the title
tvdeyen
force-pushed
the
fix-custom-elements-cloneable
branch
6 times, most recently
from
3208ff8 to
ffc575c
Compare
Member
Author
|
I think it is worth splitting this PR up into smaller pieces as this gets larger and larger |
tvdeyen
force-pushed
the
fix-custom-elements-cloneable
branch
from
2f42736 to
ffc575c
Compare
The `build:admin` command is the one to run when modifying admin JavaScript source files, not `build:js` (which bundles vendored dependencies like sortablejs, shoelace, and tinymce).
The constructor read the src attribute and called start() which mutated the element's subtree. This breaks the Web Components spec requirement that constructors must not inspect attributes or touch children, and caused cloneNode(true) to produce a different subtree than the source. When SortableJS uses jQuery's clone(true) to build a drag ghost, the node count mismatch makes jQuery crash while copying events. The bootstrap now lives in connectedCallback, which fires after the element is inserted into the document and attributes are available.
Web Components spec requires constructors to avoid inspecting attributes or children, because custom element constructors run during cloneNode(true). Reading innerHTML and attributes at construction time leaves clones in an inconsistent state and trips up libraries like jQuery's clone(true), which SortableJS uses for drag clones. Instead of capturing the original innerHTML and rewriting it, prepend the icon via insertAdjacentHTML inside connectedCallback. A guard skips the insertion when an icon is already present, keeping the operation idempotent across reconnections without needing to store the original content.
Web Components spec requires constructors to avoid mutating the DOM, because custom element constructors run during cloneNode(true). Writing innerHTML in the constructor leaves clones in an inconsistent state and can trip up libraries like jQuery's clone(true), which SortableJS uses for drag clones. Prepend the icon via insertAdjacentHTML inside connectedCallback with a guard that makes the insertion idempotent. Initialize the Clipboard instance on connect so it is paired with the disconnectedCallback teardown and survives reconnection.
Web Components spec requires constructors to avoid mutating the DOM, because custom element constructors run during cloneNode(true). Writing innerHTML and setting attributes in the constructor leaves clones in an inconsistent state and can trip up libraries like jQuery's clone(true), which SortableJS uses for drag clones. Move the setup into connectedCallback. The operations are either idempotent (classList.add, setAttribute, addEventListener with the same listener reference) or always overwrite a known state (innerHTML is always the icon since this button's content is never rendered server-side), so no explicit guard is needed.
Web Components spec requires constructors to avoid inspecting attributes or mutating the DOM, because custom element constructors run during cloneNode(true). Reading the linked state from classList, setting attributes, and writing innerHTML at construction time leaves clones in an inconsistent state and can trip up libraries like jQuery's clone(true), which SortableJS uses for drag clones. Move the setup into connectedCallback. The operations are idempotent on reconnection (classList.add, setAttribute, addEventListener with the same listener reference) and innerHTML always resolves to the icon since this button's content is never rendered server-side.
Web Components spec requires constructors to avoid inspecting children, because custom element constructors run during cloneNode(true). Running querySelector and attaching listeners to children at construction time can trip up libraries like jQuery's clone(true), which SortableJS uses for drag clones, when the subtree has not yet been populated on the clone. Move the child lookups and the delete link listener registration into connectedCallback, where the element's subtree is guaranteed to be in its final shape.
Web Components spec requires constructors to avoid inspecting children, because custom element constructors run during cloneNode(true). Running querySelector, reading dataset values, and attaching listeners to children at construction time can trip up libraries like jQuery's clone(true), which SortableJS uses for drag clones, when the subtree has not yet been populated on the clone. Move the setup into connectedCallback. Convert removeImage and mutationCallback to arrow function class fields so each yields a stable reference, which keeps addEventListener's same-listener deduplication working if the element is ever reconnected, and avoids the need for an explicit init guard.
Web Components spec requires constructors to avoid inspecting children or attaching listeners, because custom element constructors run during cloneNode(true). Attaching listeners to window, document.body, and child elements at construction time can leave clones in an inconsistent state and also leaks window/document listeners when the element is garbage collected. Move all listener registration into connectedCallback and add the matching removeEventListener calls in disconnectedCallback. Convert the inline arrow handlers to stable arrow function class fields so the same reference can be used for both add and remove.
The constructor added three self-listeners, a jQuery change handler on the form, and dblclick/click listeners on child elements. Reading the subtree and binding to it at construction time violates the Web Components spec and left cloned drag ghosts inconsistent with their own listeners, contributing to the SortableJS clone crash. All wiring now happens in connectedCallback and is torn down in disconnectedCallback. The form, header and toggle button references are cached in private fields so removeEventListener sees the same node that addEventListener did. The inline header dblclick and toggle button click handlers are promoted to stable arrow-function fields for the same reason. onChange is now an arrow-function field, which lets it reach the editor through this directly instead of walking up from the form element with closest().
The constructor attached two window-level listeners. A disconnected handle would keep those listeners on window, which holds a reference back to the instance through handleEvent and prevents garbage collection. Wiring now happens in connectedCallback and is torn down in disconnectedCallback so disconnected instances can be collected.
The constructor reached into the subtree to attach keyup, focus and click listeners on the filter field and clear button, and registered two global hotkey handlers through the key() library. Binding to children at construction time violates the Web Components spec and leaves the global hotkeys alive even before the element is attached. Wiring now happens in connectedCallback. The filter field and clear button are cached in private fields so removeEventListener targets the same nodes in disconnectedCallback. The anonymous inline handlers are promoted to stable arrow-function fields to make removal possible. The debounce timer is cleared on disconnect so a pending filter does not fire against a detached element.
The constructor attached a toggle listener and mutated the open attribute based on localStorage. Setting attributes in the constructor violates the Web Components spec and can leave the element in a state that does not match its rendered DOM. Wiring and the initial open state are now applied in connectedCallback, and the listener is torn down in disconnectedCallback so a reconnected element does not double up its handler.
The constructor queried the button child and attached a click listener. Reaching into the subtree at construction time violates the Web Components spec. Wiring now happens in connectedCallback and is torn down in disconnectedCallback so a reconnected element does not double up its listener.
The constructor attached the alchemy:link and alchemy:unlink listeners. Per the Web Components spec, listener wiring belongs in connectedCallback. Moving it there and adding the matching disconnectedCallback keeps the element safe to reconnect without stacking handlers.
The submit listener was attached in the constructor, which violates the Web Components spec. It now lives alongside the page-dirty listener in connectedCallback and is torn down in disconnectedCallback.
The constructor attached the iframe load listener and bound the message handler. Per the Web Components spec, listener wiring belongs in connectedCallback. The load listener now sits alongside the window message listener in connectedCallback and is torn down in disconnectedCallback. The previously bound #handlePreviewReadyMessage is replaced by an arrow-function field, which removes the need for a constructor and for the separate handler field.
The click listener was attached in the constructor, which violates the Web Components spec. Wiring now happens in connectedCallback and is torn down in disconnectedCallback.
…Callback The change listener was attached in the constructor, which violates the Web Components spec. Wiring now happens in connectedCallback and is torn down in disconnectedCallback.
When an element editor containing a tags field was drag-n-dropped, SortableJS moved the DOM node, triggering disconnect and reconnect. connectedCallback re-initialized select2 on an already-wrapped input, producing nested select2 wrappers that accumulated on every drag. The select2 instance is now stashed on the element and destroyed in disconnectedCallback, so the next connect sees a clean input. A new isConnected guard after the async locale import bails out if the element was disconnected during the await, matching the pattern used for Datepicker, to prevent initializing select2 on a detached input.
The change handler was attached with an anonymous function, so there was no stable reference to remove. jQuery stores handlers in its internal cache keyed by element, so every reconnect stacked another handler. A single change event then dispatched multiple submit events, which made Turbo submit the form more than once. The handler is now a stable arrow-function field and is removed in disconnectedCallback.
…nect The change handler was an inline arrow function without a stable reference, and select2 was never destroyed. On reconnect the change handler double-bound and select2 re-initialized on an already-wrapped select, stacking nested wrappers. The select2 instance is now stashed, the change handler is a stable arrow-function field, and disconnectedCallback removes the handler with .off before calling select2's destroy. select2's destroy only cleans up its own .select2-namespaced bindings, so user handlers must be removed explicitly.
The custom element never called select2("destroy") when detached, so
cloning or removing the element left orphaned select2 DOM and jQuery
bindings behind. Add a disconnectedCallback that tears down the
select2 instance.
The component never tore down its select2 instance, so detaching or cloning the element left the dropdown and its jQuery bindings attached to stale DOM. Stash the select2 instance and destroy it in disconnectedCallback. Also drop the empty constructor.
The click handler was an anonymous function closed over local consts and was never removed, so reconnecting the element stacked duplicate handlers. Convert it to a stable arrow-function field, resolve the referenced children through getters, and remove the listener in disconnectedCallback.
The view component adds the correct url so that the component actually works.
The custom element spec forbids inspecting children or attaching a shadow root in the constructor, which prevents the element from being cloned. Moving the template lookup and shadow root attachment to connectedCallback aligns Menubar with the other custom elements on this branch.
tvdeyen
force-pushed
the
fix-custom-elements-cloneable
branch
from
ffc575c to
e62fe70
Compare
sascha-karnatz
approved these changes
Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this pull request for?
Reworks every admin custom element so setup happens in
connectedCallbackand teardown happens indisconnectedCallback, instead of in the constructor. Several components were reading attributes, mutating their own subtree, or wiring jQuery/select2 handlers at construction time, which violates the Web Components spec: constructors must not inspect attributes or touch children.The visible symptom was that
cloneNode(true)produced a subtree that did not match the source element. When SortableJS uses jQuery'sclone(true)to build a drag ghost for elements and ingredients, that node-count mismatch made jQuery crash mid-copy and drag-and-drop broke. Moving the bootstrap intoconnectedCallback— which fires after insertion, when attributes and children are available — restores spec compliance and fixes the clone.The same pass also gave every component that attaches global handlers (jQuery
change/click, select2 instances,windowload listeners, page-level click listeners) a matchingdisconnectedCallbackthat removes the handler or callsselect2("destroy"). Without this, elements re-rendered by Turbo or removed by the editor leaked listeners against the old DOM.Notable changes
The
AlchemyHTMLElementbase class has been removed. Nothing extended it anymore after the migration, and the base class itself encouraged the constructor-time DOM work.Checklist