Skip to content

Commit 122e989

Browse files
Fix: make rerenders work after DocumentFragment is appended to DOM
- bounds.ts clear(): use first.parentNode (live parent) instead of stored parentElement(), so removal targets the real container after fragment append - element-builder.ts resume(): capture live parent from firstNode().parentNode before resetting, so new content renders into the container not the fragment - element-builder.ts RemoteBlock destructor: check firstNode().parentNode !== null (node still attached) instead of === parentElement() (node in original parent), so in-element/DocumentFragment targets get cleaned up correctly on destroy - Update test to attach fragment first, then rerender and verify updates work Agent-Logs-Url: https://github.com/emberjs/ember.js/sessions/6f1bf405-84ab-4bd5-b42d-f061fadcf4df Co-authored-by: NullVoxPopuli <[email protected]>
1 parent 9b96a2c commit 122e989

3 files changed

Lines changed: 40 additions & 19 deletions

File tree

packages/@glimmer-workspace/integration-tests/lib/suites/in-element-document-fragment.ts

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ export class InElementDocumentFragmentSuite extends RenderTest {
133133
}
134134

135135
@test
136-
'Rerenders in a detached DocumentFragment, then content is visible after attaching to DOM'(
136+
'Rerenders work after DocumentFragment is appended to the DOM'(
137137
assert: typeof QUnit.assert
138138
) {
139139
const fragment = document.createDocumentFragment();
@@ -160,27 +160,33 @@ export class InElementDocumentFragmentSuite extends RenderTest {
160160

161161
assert.verifySteps(['initial'], 'initial render fires step from inside fragment');
162162

163-
// Rerender while still detached: text update
163+
// Move the fragment's children into the container. After this the fragment is
164+
// empty, but the rendered nodes (including Glimmer's bounds markers) are live
165+
// children of `container`.
166+
container.appendChild(fragment);
167+
assert.strictEqual(fragment.childNodes.length, 0, 'fragment is empty after append');
168+
assert.ok(container.querySelector('#msg'), 'paragraph is present in container after append');
169+
170+
// Rerenders should continue to work after the fragment is attached — Glimmer
171+
// resolves the live parent from the bounds markers' actual parentNode.
164172
this.rerender({ message: 'updated' });
165-
assert.verifySteps(['updated'], 'text update fires step while fragment is still detached');
173+
assert.verifySteps(['updated'], 'text update fires step after fragment was attached to DOM');
174+
assert.strictEqual(
175+
container.querySelector('#msg')?.textContent,
176+
'updated',
177+
'paragraph text is updated in container'
178+
);
166179

167-
// Rerender while still detached: conditional element appears
180+
// New conditional element should appear in the container.
168181
this.rerender({ show: true });
169182
assert.verifySteps(
170183
['extra rendered'],
171-
'conditional element step fires while fragment is still detached'
184+
'conditional element step fires in container after fragment was attached to DOM'
185+
);
186+
assert.ok(
187+
container.querySelector('#extra'),
188+
'conditional span appears in container after fragment was attached to DOM'
172189
);
173-
174-
// Move the fragment's children into the container; after this the fragment is empty
175-
// but the rendered nodes (including the reactive text and the conditional span) are
176-
// now live children of `container`.
177-
container.appendChild(fragment);
178-
assert.strictEqual(fragment.childNodes.length, 0, 'fragment is empty after append');
179-
180-
const p = container.querySelector('#msg');
181-
const span = container.querySelector('#extra');
182-
assert.ok(p, 'paragraph is present in container');
183-
assert.ok(span, 'conditional span is present in container');
184190
}
185191

186192
@test

packages/@glimmer/runtime/lib/bounds.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,15 @@ export function move(bounds: Bounds, reference: Nullable<SimpleNode>): Nullable<
5353
}
5454

5555
export function clear(bounds: Bounds): Nullable<SimpleNode> {
56-
let parent = bounds.parentElement();
5756
let first = bounds.firstNode();
5857
let last = bounds.lastNode();
5958

59+
// Use the node's actual current parent rather than the stored parentElement.
60+
// When bounds were rendered into a DocumentFragment that was subsequently
61+
// appended to a real DOM container, the nodes' parentNode is the container
62+
// while parentElement() still returns the (now-empty) fragment.
63+
let parent = (first.parentNode as Nullable<SimpleElement>) ?? bounds.parentElement();
64+
6065
let current: SimpleNode = first;
6166

6267
while (true) {

packages/@glimmer/runtime/lib/vm/element-builder.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,12 @@ export class NewTreeBuilder implements TreeBuilder {
9696
}
9797

9898
static resume(env: Environment, block: ResettableBlock): NewTreeBuilder {
99-
let parentNode = block.parentElement();
99+
// Capture the live parent before resetting, because the bounds may have been
100+
// rendered into a DocumentFragment that was subsequently appended to a real
101+
// DOM container. In that case firstNode().parentNode is the container while
102+
// parentElement() still returns the original (now-empty) fragment.
103+
let parentNode =
104+
(block.firstNode().parentNode as SimpleElement | null) ?? block.parentElement();
100105
let nextSibling = block.reset(env);
101106

102107
let stack = new this(env, parentNode, nextSibling).initialize();
@@ -500,7 +505,12 @@ export class RemoteBlock extends AppendingBlockImpl {
500505
// and avoid clearing the node if it was. In most cases this shouldn't happen,
501506
// so this might hide bugs where the code clears nested nodes unnecessarily,
502507
// so we should eventually try to do the correct fix.
503-
if (this.parentElement() === this.firstNode().parentNode) {
508+
//
509+
// Note: we check firstNode().parentNode !== null (node still has a parent)
510+
// rather than === parentElement() (node is in the original parent), so that
511+
// {{#in-element}} into a DocumentFragment still clears correctly after the
512+
// fragment's children are moved to a real DOM container via appendChild().
513+
if (this.firstNode().parentNode !== null) {
504514
clear(this);
505515
}
506516
});

0 commit comments

Comments
 (0)