Skip to content

Commit b65dcdf

Browse files
wmertenswmertens
authored
Merge pull request #7538 from QwikDev/v2-insert-before-shared-text
fix: inflating text nodes from single shared text node
2 parents c3b4975 + f5732f5 commit b65dcdf

File tree

3 files changed

+160
-24
lines changed

3 files changed

+160
-24
lines changed

‎.changeset/wet-bobcats-decide.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@qwik.dev/core': patch
3+
---
4+
5+
fix: inflating text nodes from single shared text node

‎packages/qwik/src/core/client/vnode.ts

Lines changed: 75 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -991,7 +991,74 @@ export const vnode_insertBefore = (
991991
}
992992
}
993993

994-
// ensure that the previous node is unlinked.
994+
/**
995+
* Find the parent node and the dom children with the correct namespaces before we unlink the
996+
* previous node. If we don't do this, we will end up with situations where we inflate text nodes
997+
* from shared text node not correctly.
998+
*
999+
* Example:
1000+
*
1001+
* ```
1002+
* <Component>
1003+
* <Projection>a</Projection>
1004+
* <Projection>b</Projection>
1005+
* </Component>
1006+
* ```
1007+
*
1008+
* Projection nodes are virtual nodes, so they don't have a dom parent. They will be written to
1009+
* the q:template element if not visible at the start. Inside the q:template element, the
1010+
* projection nodes will be streamed as single text node "ab". We need to split it, but if we
1011+
* unlink the previous or next sibling, we don't know that after "a" node is "b". So we need to
1012+
* find children first (and inflate them).
1013+
*/
1014+
const domParentVNode = vnode_getDomParentVNode(parent);
1015+
const parentNode = domParentVNode && domParentVNode[ElementVNodeProps.element];
1016+
let domChildren: (Element | Text)[] | null = null;
1017+
if (domParentVNode) {
1018+
domChildren = vnode_getDomChildrenWithCorrectNamespacesToInsert(
1019+
journal,
1020+
domParentVNode,
1021+
newChild
1022+
);
1023+
}
1024+
1025+
/**
1026+
* Ensure that the previous node is unlinked.
1027+
*
1028+
* We need to do it before finding the adjustedInsertBefore. The problem is when you try to render
1029+
* the same projection multiple times in the same node but under different conditions. We reuse
1030+
* projection nodes, so when this happens, we can end up with a situation where the node is
1031+
* inserted before node above it.
1032+
*
1033+
* Example:
1034+
*
1035+
* ```
1036+
* <>
1037+
* {props.toggle && <Slot />}
1038+
* {!props.toggle && (
1039+
* <>
1040+
* <Slot />
1041+
* </>
1042+
* )}
1043+
* </>
1044+
* ```
1045+
*
1046+
* Projected content:
1047+
*
1048+
* ```
1049+
* <h1>Test</h1>
1050+
* <p>Test content</p>
1051+
* ```
1052+
*
1053+
* If we don't unlink the previous node, we will end up at some point with the following:
1054+
*
1055+
* ```
1056+
* <h1>Test</h1>
1057+
* <p>Test content</p> // <-- inserted before the first h1
1058+
* <h1>Test</h1> // <-- to remove, but still in the tree
1059+
* <p>Test content</p> // <-- to remove
1060+
* ```
1061+
*/
9951062
if (
9961063
newChildCurrentParent &&
9971064
(newChild[VNodeProps.previousSibling] ||
@@ -1008,7 +1075,6 @@ export const vnode_insertBefore = (
10081075
// Well, not quite. If the parent is a virtual node, our "last node" is not the same
10091076
// as the DOM "last node". So in that case we need to look for the "next node" from
10101077
// our parent.
1011-
10121078
adjustedInsertBefore = vnode_getDomSibling(parent, true, false);
10131079
}
10141080
} else if (vnode_isVirtualVNode(insertBefore)) {
@@ -1018,30 +1084,15 @@ export const vnode_insertBefore = (
10181084
adjustedInsertBefore = insertBefore;
10191085
}
10201086
adjustedInsertBefore && vnode_ensureInflatedIfText(journal, adjustedInsertBefore);
1021-
// If `insertBefore` is null, than we need to insert at the end of the list.
1022-
// Well, not quite. If the parent is a virtual node, our "last node" is not the same
1023-
// as the DOM "last node". So in that case we need to look for the "next node" from
1024-
// our parent.
1025-
// const shouldWeUseParentVirtual = insertBefore == null && vnode_isVirtualVNode(parent);
1026-
// const insertBeforeNode = shouldWeUseParentVirtual
1027-
// ? vnode_getDomSibling(parent, true)
1028-
// : insertBefore;
1029-
const domParentVNode = vnode_getDomParentVNode(parent);
1030-
const parentNode = domParentVNode && domParentVNode[ElementVNodeProps.element];
10311087

1032-
if (parentNode) {
1033-
const domChildren = vnode_getDomChildrenWithCorrectNamespacesToInsert(
1034-
journal,
1035-
domParentVNode,
1036-
newChild
1088+
// Here we know the insertBefore node
1089+
if (domChildren && domChildren.length) {
1090+
journal.push(
1091+
VNodeJournalOpCode.Insert,
1092+
parentNode,
1093+
vnode_getNode(adjustedInsertBefore),
1094+
...domChildren
10371095
);
1038-
domChildren.length &&
1039-
journal.push(
1040-
VNodeJournalOpCode.Insert,
1041-
parentNode,
1042-
vnode_getNode(adjustedInsertBefore),
1043-
...domChildren
1044-
);
10451096
}
10461097

10471098
// link newChild into the previous/next list

‎packages/qwik/src/core/tests/projection.spec.tsx

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1808,6 +1808,86 @@ describe.each([
18081808
</Component>
18091809
);
18101810
});
1811+
1812+
it('should correctly inflate text nodes from q:template', async () => {
1813+
const Cmp = component$((props: { show: boolean }) => {
1814+
return <span>{props.show && <Slot />}</span>;
1815+
});
1816+
const Parent = component$(() => {
1817+
const show = useSignal(false);
1818+
return (
1819+
<>
1820+
<button onClick$={() => (show.value = !show.value)}></button>
1821+
<Cmp show={show.value}>a</Cmp>
1822+
<Cmp show={show.value}>b</Cmp>
1823+
</>
1824+
);
1825+
});
1826+
const { vNode, document } = await render(<Parent />, { debug: DEBUG });
1827+
expect(vNode).toMatchVDOM(
1828+
<Component ssr-required>
1829+
<Fragment ssr-required>
1830+
<button></button>
1831+
<Component ssr-required>
1832+
<span></span>
1833+
</Component>
1834+
<Component ssr-required>
1835+
<span></span>
1836+
</Component>
1837+
</Fragment>
1838+
</Component>
1839+
);
1840+
await trigger(document.body, 'button', 'click');
1841+
expect(vNode).toMatchVDOM(
1842+
<Component ssr-required>
1843+
<Fragment ssr-required>
1844+
<button></button>
1845+
<Component ssr-required>
1846+
<span>
1847+
<Projection ssr-required>a</Projection>
1848+
</span>
1849+
</Component>
1850+
<Component ssr-required>
1851+
<span>
1852+
<Projection ssr-required>b</Projection>
1853+
</span>
1854+
</Component>
1855+
</Fragment>
1856+
</Component>
1857+
);
1858+
await trigger(document.body, 'button', 'click');
1859+
expect(vNode).toMatchVDOM(
1860+
<Component ssr-required>
1861+
<Fragment ssr-required>
1862+
<button></button>
1863+
<Component ssr-required>
1864+
<span></span>
1865+
</Component>
1866+
<Component ssr-required>
1867+
<span></span>
1868+
</Component>
1869+
</Fragment>
1870+
</Component>
1871+
);
1872+
await trigger(document.body, 'button', 'click');
1873+
expect(vNode).toMatchVDOM(
1874+
<Component ssr-required>
1875+
<Fragment ssr-required>
1876+
<button></button>
1877+
<Component ssr-required>
1878+
<span>
1879+
<Projection ssr-required>a</Projection>
1880+
</span>
1881+
</Component>
1882+
<Component ssr-required>
1883+
<span>
1884+
<Projection ssr-required>b</Projection>
1885+
</span>
1886+
</Component>
1887+
</Fragment>
1888+
</Component>
1889+
);
1890+
});
18111891
});
18121892

18131893
describe('svg', () => {

0 commit comments

Comments
 (0)