Skip to content

[BUGFIX] DocumentFragment support — {{#in-element}} with DocumentFragment targets #21253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Copilot wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

[BUGFIX] DocumentFragment support — {{#in-element}} with DocumentFragment targets #21253

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e1a05ff
Extract document-fragment support from shadow DOM PR
Copilot Mar 26, 2026
5c6b385
Fix misleading comment about querySelector on DocumentFragment
Copilot Mar 26, 2026
3a93b79
Add test: verify text updates and conditional elements after fragment…
Copilot Mar 27, 2026
3e1e1c1
Refactor test: use step helper called from template instead of DOM as…
Copilot Mar 27, 2026
8eb9b7d
Fix test: use per-test assert parameter and reactive step helper to f…
Copilot Mar 27, 2026
9b96a2c
Fix test: only rerender while fragment is detached, not after appendi…
Copilot Mar 27, 2026
122e989
Fix: make rerenders work after DocumentFragment is appended to DOM
Copilot Mar 27, 2026
ef0a958
Fix: prettier formatting in in-element-document-fragment.ts
Copilot Mar 27, 2026
0e0c6d5
The true cost (in types) of | SimpleDocumentFragment
NullVoxPopuli Apr 6, 2026
d6fc612
Move to SimpleNode
NullVoxPopuli Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/@glimmer-workspace/integration-tests/lib/suites.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export * from './suites/entry-point';
export * from './suites/has-block';
export * from './suites/has-block-params';
export * from './suites/in-element';
export * from './suites/in-element-document-fragment';
export * from './suites/initial-render';
export * from './suites/scope';
export * from './suites/shadowing';
Expand Down
164 changes: 164 additions & 0 deletions packages/@glimmer-workspace/integration-tests/lib/suites/in-element-document-fragment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
import { RenderTest } from '../render-test';
import { test } from '../test-decorator';

export class InElementDocumentFragmentSuite extends RenderTest {
static suiteName = '#in-element (DocumentFragment)';

@test
'Renders curlies into a detached DocumentFragment'() {
const fragment = document.createDocumentFragment();

this.render('{{#in-element this.fragment}}[{{this.foo}}]{{/in-element}}', {
fragment,
foo: 'Hello Fragment!',
});

this.assert.strictEqual(
fragment.textContent,
'[Hello Fragment!]',
'content rendered in document fragment'
);
this.assertHTML('<!---->');
this.assertStableRerender();

this.rerender({ foo: 'Updated!' });
this.assert.strictEqual(
fragment.textContent,
'[Updated!]',
'content updated in document fragment'
);
this.assertHTML('<!---->');

this.rerender({ foo: 'Hello Fragment!' });
this.assert.strictEqual(
fragment.textContent,
'[Hello Fragment!]',
'content reverted in document fragment'
);
this.assertHTML('<!---->');
}

@test
'Renders curlies into a template.content fragment'() {
const templateEl = document.createElement('template');
const fragment = templateEl.content;

this.render('{{#in-element this.fragment}}[{{this.foo}}]{{/in-element}}', {
fragment,
foo: 'Hello Template Content!',
});

this.assert.strictEqual(
fragment.textContent,
'[Hello Template Content!]',
'content rendered in template.content fragment'
);
this.assertHTML('<!---->');
this.assertStableRerender();

this.rerender({ foo: 'Updated!' });
this.assert.strictEqual(
fragment.textContent,
'[Updated!]',
'content updated in template.content fragment'
);
this.assertHTML('<!---->');

this.rerender({ foo: 'Hello Template Content!' });
this.assert.strictEqual(
fragment.textContent,
'[Hello Template Content!]',
'content reverted in template.content fragment'
);
this.assertHTML('<!---->');
}

@test
'Renders elements into a fragment that is later attached to the DOM'() {
const fragment = document.createDocumentFragment();
const container = document.createElement('div');

this.render('{{#in-element this.fragment}}<p id="frag-p">{{this.message}}</p>{{/in-element}}', {
fragment,
message: 'in fragment',
});

this.assert.strictEqual(
fragment.querySelector('#frag-p')?.textContent,
'in fragment',
'content rendered in detached fragment'
);
this.assertHTML('<!---->');

// Attach fragment's children to the DOM
container.appendChild(fragment);
Comment thread
NullVoxPopuli marked this conversation as resolved.
this.assert.strictEqual(
container.querySelector('#frag-p')?.textContent,
'in fragment',
'content is in the DOM after fragment is appended'
);
// Fragment itself is now empty (children moved to container)
this.assert.strictEqual(fragment.childNodes.length, 0, 'fragment is empty after append');
}

@test
'Multiple in-element calls to the same DocumentFragment'() {
const fragment = document.createDocumentFragment();

this.render(
'{{#in-element this.fragment}}[{{this.foo}}]{{/in-element}}' +
'{{#in-element this.fragment insertBefore=null}}[{{this.bar}}]{{/in-element}}',
{
fragment,
foo: 'first',
bar: 'second',
}
);

this.assert.ok(fragment.textContent?.includes('[first]'), 'first block present in fragment');
this.assert.ok(fragment.textContent?.includes('[second]'), 'second block present in fragment');
this.assertHTML('<!----><!---->');
this.assertStableRerender();

this.rerender({ foo: 'updated-first', bar: 'updated-second' });
this.assert.ok(
fragment.textContent?.includes('[updated-first]'),
'first block updated in fragment'
);
this.assert.ok(
fragment.textContent?.includes('[updated-second]'),
'second block updated in fragment'
);
this.assertHTML('<!----><!---->');
}

@test
'Multiple in-element calls to the same DocumentFragment with insertBefore=null'() {
const fragment = document.createDocumentFragment();

this.render(
'{{#in-element this.fragment insertBefore=null}}<p id="a">{{this.foo}}</p>{{/in-element}}' +
'{{#in-element this.fragment insertBefore=null}}<p id="b">{{this.bar}}</p>{{/in-element}}',
{
fragment,
foo: 'first',
bar: 'second',
}
);

// Use childNodes to query into the fragment since querySelector doesn't work on detached fragment nodes in all browsers
Comment thread
NullVoxPopuli marked this conversation as resolved.
Outdated
const nodes = Array.from(fragment.childNodes);
const pA = nodes.find((n) => (n as Element).id === 'a') as HTMLElement | undefined;
const pB = nodes.find((n) => (n as Element).id === 'b') as HTMLElement | undefined;

this.assert.strictEqual(pA?.textContent, 'first', 'first block appended to fragment');
this.assert.strictEqual(pB?.textContent, 'second', 'second block appended to fragment');
this.assertHTML('<!----><!---->');
this.assertStableRerender();

this.rerender({ foo: 'updated-first', bar: 'updated-second' });
this.assert.strictEqual(pA?.textContent, 'updated-first', 'first block updated in fragment');
this.assert.strictEqual(pB?.textContent, 'updated-second', 'second block updated in fragment');
this.assertHTML('<!----><!---->');
}
}
2 changes: 2 additions & 0 deletions packages/@glimmer-workspace/integration-tests/test/jit-suites-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
GlimmerishComponents,
HasBlockParamsHelperSuite,
HasBlockSuite,
InElementDocumentFragmentSuite,
InElementSuite,
jitComponentSuite,
jitSuite,
Expand All @@ -18,6 +19,7 @@ import {
jitComponentSuite(DebuggerSuite);
jitSuite(EachSuite);
jitSuite(InElementSuite);
jitSuite(InElementDocumentFragmentSuite);

jitComponentSuite(GlimmerishComponents);
jitComponentSuite(TemplateOnlyComponents);
Expand Down
8 changes: 7 additions & 1 deletion packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type {
ModifierInstance,
Nullable,
Owner,
SimpleElement,
UpdatingOpcode,
UpdatingVM,
} from '@glimmer/interfaces';
Expand All @@ -29,10 +30,12 @@ import {
} from '@glimmer/constants';
import {
check,
CheckDocumentFragment,
CheckElement,
CheckMaybe,
CheckNode,
CheckNullable,
CheckOr,
CheckString,
} from '@glimmer/debug';
import { debugToString, expect } from '@glimmer/debug-util';
Expand Down Expand Up @@ -74,7 +77,10 @@ APPEND_OPCODES.add(VM_PUSH_REMOTE_ELEMENT_OP, (vm) => {
let insertBeforeRef = check(vm.stack.pop(), CheckReference);
let guidRef = check(vm.stack.pop(), CheckReference);

let element = check(valueForRef(elementRef), CheckElement);
let element = check(
valueForRef(elementRef),
CheckOr(CheckElement, CheckDocumentFragment)
) as SimpleElement;
Copy link
Copy Markdown
Contributor

@ef4 ef4 Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not cast. Push the type change to the implementation so that we can really show it's always safe to pass document fragments.

Copy link
Copy Markdown
Contributor

@NullVoxPopuli NullVoxPopuli Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type changed pushed here: 0e0c6d5

Copy link
Copy Markdown
Contributor

@ef4 ef4 Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing the type changes makes me think we're missing something here, and the preexisting uses of SimpleElement that this PR is forced to expand to SimpleElement | SimpleDocumentFragment should possibly go the other direction to SimpleNode. For example, this:

export interface Bounds {
  // ...
  parentElement(): SimpleElement;
  // ...
}

Should probably really be:

export interface Bounds {
  // ...
  parentNode(): SimpleNode;
  // ...
}

And then you would get support for DocumentFragment (and critically also Document!).

HTML Node's parentElement is defined to always be Element or null. It's never a document fragment or document. Whereas parentNode is allowed to be a DocumentFragment, etc.

I think the existing types were unnecessarily specific when they used SimpleElement instead of SimpleNode in most o the places this PR had to touch. Most of them a clearly not relying on any Element-specific API, since they happily tolerate DocumentFragment instead with very few changes.

let insertBefore = check(valueForRef(insertBeforeRef), CheckMaybe(CheckNullable(CheckNode)));
let guid = valueForRef(guidRef) as string;

Expand Down
Loading