[BUGFIX] {{#in-element}} with DocumentFragment targets, continued - #21550
Closed
NullVoxPopuli-ai-agent wants to merge 4 commits into
Closed
[BUGFIX] {{#in-element}} with DocumentFragment targets, continued#21550NullVoxPopuli-ai-agent wants to merge 4 commits into
{{#in-element}} with DocumentFragment targets, continued#21550NullVoxPopuli-ai-agent wants to merge 4 commits into
Conversation
Combined state of emberjs#21253 rebased onto latest main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per review on emberjs#21253: instead of widening SimpleElement to SimpleElement | SimpleDocumentFragment at every use site, move the over-specific types to SimpleNode. DOM's parentElement is defined to always be an Element or null, while parentNode may be an Element, DocumentFragment, or Document — so the method is renamed to match what it can actually return. Also drop the casts in clear()/resume() (SimpleNodeBase already carries parentNode/insertBefore/removeChild), extract the live-parent resolution into liveParent(), and use it in move() and the list-update paths in update.ts so {{#each}} insertions, reorders, and the sync marker also follow content that migrated out of a DocumentFragment. The serialized CapturedRenderNode['bounds'].parentElement key and Renderer#getBounds shape are intentionally unchanged so ember-inspector keeps working. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gets Covers the behavior discussed in emberjs#21253: - After container.appendChild(fragment), toggling an {{#if}} renders the new element into the container next to the migrated content (the empty-block comment marker migrates with the fragment's children and anchors the live location), and removes it from the container again. - Destroying the {{#in-element}} after attachment clears the content from the container, and re-creating it renders into the (again detached) fragment. - {{#each}} append/reorder/remove all mutate the container, never the stale fragment. - {{#in-element}} accepts a ShadowRoot (a DocumentFragment subtype). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Contributor
|
Continued over here: #21551 |
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
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.
Continues #21253 (rebased onto latest
main, squashed to resolve conflicts with the import-style changes onmain), and addresses the review discussion there.What this adds
{{#in-element}}accepts aDocumentFragment(includingtemplate.contentandShadowRoot) as its destination, and rendered content keeps working after the fragment's children are moved into the DOM viacontainer.appendChild(fragment).Review feedback addressed
Types go to
SimpleNode, notSimpleElement | SimpleDocumentFragment(@ef4)Instead of widening
SimpleElementat each use site, the over-specificSimpleElementtypes moved toSimpleNode, andBounds#parentElement()is renamed toBounds#parentNode(). DOM'sparentElementis always anElementornull, whileparentNodemay be anElement,DocumentFragment, orDocument, so the name now matches what the method can return. The casts inclear()/resume()are gone;SimpleNodeBasealready carriesparentNode/insertBefore/removeChild, so everything checks without assertions. The one remaining runtime check is theCheckElement | CheckDocumentFragmentdebug check on thein-elementdestination itself.Two serialized shapes intentionally keep the
parentElementkey with a widened type rather than being renamed:CapturedRenderNode['bounds']andRenderer#getBounds, since ember-inspector consumes them.Updates follow the content, not the fragment (@lifeart, @ef4)
The "smeared across two locations" scenario is now handled and tested: bounds resolve their mutation parent from
firstNode().parentNode(the live location) with the stored parent as fallback. The marker mechanism @lifeart proposed effectively already exists: every block, including an empty{{#if}}, leaves at least a comment node in the DOM, and those markers migrate with the fragment's children, anchoring the live location.This applies uniformly to:
clear()andmove()inbounds.ts(block teardown and{{#each}}reordering)NewTreeBuilder.resume()(re-rendering a resettable block, e.g. toggling{{#if}}){{#each}}sync-marker and item-insertion paths inupdate.ts, which anchor on the live reference node's parent (the list's own bounds are mid-initialization duringsync(), so they can't supply it)RemoteBlockdestructor (teardown after the content migrated)New tests cover: toggling an
{{#if}}on/off after attachment (renders next to the migrated content in the container, never into the stale fragment),{{#each}}append/reorder/remove after attachment, destroying the whole{{#in-element}}after attachment (clears the container; re-creating renders into the again-detached fragment), and aShadowRootdestination (the shadow-DOM prerequisite from the discussion).Out of scope
{{this.fragment}}) — separate PR, per discussion.{{#in-element}}), which is tested.insertBeforevalues other thannull/undefined(unchanged fromElementdestinations).