-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[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.
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
e1a05ff
5c6b385
3a93b79
3e1e1c1
8eb9b7d
9b96a2c
122e989
ef0a958
0e0c6d5
d6fc612
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,229 @@ | ||
| 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); | ||
| 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 | ||
| 'After fragment is attached to DOM, text updates and new conditional elements appear in the container'() { | ||
| const fragment = document.createDocumentFragment(); | ||
| const container = document.createElement('div'); | ||
|
|
||
| this.render( | ||
| '{{#in-element this.fragment}}' + | ||
| '<p id="msg">{{this.message}}</p>' + | ||
|
NullVoxPopuli marked this conversation as resolved.
Outdated
|
||
| '{{#if this.show}}<span id="extra">extra</span>{{/if}}' + | ||
| '{{/in-element}}', | ||
| { | ||
| fragment, | ||
| message: 'initial', | ||
| show: false, | ||
| } | ||
| ); | ||
|
|
||
| this.assert.step('initial render into fragment'); | ||
| const p = fragment.querySelector('#msg') as HTMLElement; | ||
| this.assert.strictEqual(p?.textContent, 'initial', 'p rendered in fragment'); | ||
| this.assert.notOk(fragment.querySelector('#extra'), 'no extra span in fragment yet'); | ||
|
|
||
| // Move fragment's children (including Glimmer's comment bounds) into the container | ||
| container.appendChild(fragment); | ||
| this.assert.step('fragment attached to DOM'); | ||
| this.assert.strictEqual(fragment.childNodes.length, 0, 'fragment is empty after append'); | ||
| this.assert.strictEqual( | ||
| container.querySelector('#msg')?.textContent, | ||
| 'initial', | ||
| 'p is in the container' | ||
| ); | ||
|
|
||
| // Text-node update: Glimmer holds a direct reference to the text node, so the | ||
| // update is visible in the container even though the fragment is now empty. | ||
| this.rerender({ message: 'updated' }); | ||
| this.assert.step('text updated'); | ||
| this.assert.strictEqual( | ||
| container.querySelector('#msg')?.textContent, | ||
| 'updated', | ||
| 'text update is reflected in the container' | ||
| ); | ||
| this.assert.strictEqual(fragment.childNodes.length, 0, 'fragment remains empty after text update'); | ||
|
|
||
| // New-element update: Glimmer inserts the span relative to the comment bounds, | ||
| // which also moved to the container, so the new element appears in the container. | ||
| this.rerender({ show: true }); | ||
| this.assert.step('conditional element shown'); | ||
| this.assert.ok( | ||
| container.querySelector('#extra'), | ||
| 'new conditional element appears in the container (comment bounds moved with the fragment)' | ||
| ); | ||
| this.assert.notOk( | ||
| fragment.querySelector('#extra'), | ||
| 'new conditional element is not in the (now-empty) fragment' | ||
| ); | ||
|
|
||
| this.assert.verifySteps([ | ||
| 'initial render into fragment', | ||
| 'fragment attached to DOM', | ||
| 'text updated', | ||
| 'conditional element shown', | ||
| ]); | ||
| } | ||
|
|
||
| @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 traverse the fragment's direct children since glimmer also | ||
| // inserts comment marker nodes alongside the rendered elements. | ||
| 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('<!----><!---->'); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import type { | |
| ModifierInstance, | ||
| Nullable, | ||
| Owner, | ||
| SimpleElement, | ||
| UpdatingOpcode, | ||
| UpdatingVM, | ||
| } from '@glimmer/interfaces'; | ||
|
|
@@ -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'; | ||
|
|
@@ -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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. type changed pushed here: 0e0c6d5
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 I think the existing types were unnecessarily specific when they used |
||
| let insertBefore = check(valueForRef(insertBeforeRef), CheckMaybe(CheckNullable(CheckNode))); | ||
| let guid = valueForRef(guidRef) as string; | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.