From 4794f2a5342a64d5b85284f5d14ca7a2740be156 Mon Sep 17 00:00:00 2001 From: Varixo Date: Sat, 4 Oct 2025 16:02:05 +0200 Subject: [PATCH] fix: adding and removing attributes on vnodes --- .changeset/shiny-readers-double.md | 5 + packages/qwik/src/core/client/vnode-diff.ts | 105 +++--- .../qwik/src/core/client/vnode-diff.unit.tsx | 322 ++++++++++++++++++ 3 files changed, 374 insertions(+), 58 deletions(-) create mode 100644 .changeset/shiny-readers-double.md diff --git a/.changeset/shiny-readers-double.md b/.changeset/shiny-readers-double.md new file mode 100644 index 00000000000..c5ca5049052 --- /dev/null +++ b/.changeset/shiny-readers-double.md @@ -0,0 +1,5 @@ +--- +'@qwik.dev/core': patch +--- + +fix: adding and removing attributes on vnodes diff --git a/packages/qwik/src/core/client/vnode-diff.ts b/packages/qwik/src/core/client/vnode-diff.ts index 56c9e177b47..a8a0aa0bb3e 100644 --- a/packages/qwik/src/core/client/vnode-diff.ts +++ b/packages/qwik/src/core/client/vnode-diff.ts @@ -45,7 +45,7 @@ import { hasClassAttr } from '../shared/utils/scoped-styles'; import type { HostElement, QElement, QwikLoaderEventScope, qWindow } from '../shared/types'; import { DEBUG_TYPE, QContainerValue, VirtualType } from '../shared/types'; import type { DomContainer } from './dom-container'; -import { VNodeFlags, type ClientAttrKey, type ClientAttrs, type ClientContainer } from './types'; +import { VNodeFlags, type ClientAttrs, type ClientContainer } from './types'; import { vnode_ensureElementInflated, vnode_getDomParentVNode, @@ -783,11 +783,7 @@ export const vnode_diff = ( vnode_ensureElementInflated(vnode); const dstAttrs = vnode_getProps(vnode) as ClientAttrs; let srcIdx = 0; - const srcLength = srcAttrs.length; let dstIdx = 0; - let dstLength = dstAttrs.length; - let srcKey: ClientAttrKey | null = srcIdx < srcLength ? srcAttrs[srcIdx++] : null; - let dstKey: ClientAttrKey | null = dstIdx < dstLength ? dstAttrs[dstIdx++] : null; let patchEventDispatch = false; const record = (key: string, value: any) => { @@ -820,10 +816,6 @@ export const vnode_diff = ( value !== null ? serializeAttribute(key, value, scopedStyleIdPrefix) : null, journal ); - if (value === null) { - // if we set `null` than attribute was removed and we need to shorten the dstLength - dstLength = dstAttrs.length; - } }; const recordJsxEvent = (key: string, value: any) => { @@ -846,73 +838,70 @@ export const vnode_diff = ( } }; - while (srcKey !== null || dstKey !== null) { + // Two-pointer merge algorithm: both arrays are sorted by key + // Note: dstAttrs mutates during iteration (setAttr uses splice), so we re-read keys each iteration + while (srcIdx < srcAttrs.length || dstIdx < dstAttrs.length) { + const srcKey = srcIdx < srcAttrs.length ? (srcAttrs[srcIdx] as string) : undefined; + const dstKey = dstIdx < dstAttrs.length ? (dstAttrs[dstIdx] as string) : undefined; + + // Skip special keys in destination (HANDLER_PREFIX, Q_PREFIX) if (dstKey?.startsWith(HANDLER_PREFIX) || dstKey?.startsWith(Q_PREFIX)) { - // These are a special keys which we use to mark the event handlers as immutable or - // element key we need to ignore them. - dstIdx++; // skip the destination value, we don't care about it. - dstKey = dstIdx < dstLength ? dstAttrs[dstIdx++] : null; - } else if (srcKey == null) { - // Source has more keys, so we need to remove them from destination - if (dstKey && isHtmlAttributeAnEventName(dstKey)) { - dstIdx++; + dstIdx += 2; // skip key and value + continue; + } + + if (srcKey === undefined) { + // Source exhausted: remove remaining destination keys + if (isHtmlAttributeAnEventName(dstKey!)) { + // HTML event attributes are immutable and not removed from DOM + dstIdx += 2; // skip key and value } else { record(dstKey!, null); - dstIdx--; + // After removal, dstAttrs shrinks by 2, so don't advance dstIdx } - dstKey = dstIdx < dstLength ? dstAttrs[dstIdx++] : null; - } else if (dstKey == null) { - // Destination has more keys, so we need to insert them from source. - const isEvent = isJsxPropertyAnEventName(srcKey); - if (isEvent) { - // Special handling for events + } else if (dstKey === undefined) { + // Destination exhausted: add remaining source keys + const srcValue = srcAttrs[srcIdx + 1]; + if (isJsxPropertyAnEventName(srcKey)) { patchEventDispatch = true; - recordJsxEvent(srcKey, srcAttrs[srcIdx]); + recordJsxEvent(srcKey, srcValue); } else { - record(srcKey!, srcAttrs[srcIdx]); + record(srcKey, srcValue); } - srcIdx++; - srcKey = srcIdx < srcLength ? srcAttrs[srcIdx++] : null; - // we need to increment dstIdx too, because we added destination key and value to the VNode - // and dstAttrs is a reference to the VNode - dstIdx++; - dstKey = dstIdx < dstLength ? dstAttrs[dstIdx++] : null; - } else if (srcKey == dstKey) { - const srcValue = srcAttrs[srcIdx++]; - const dstValue = dstAttrs[dstIdx++]; + srcIdx += 2; // skip key and value + // After addition, dstAttrs grows by 2 at sorted position, advance dstIdx + dstIdx += 2; + } else if (srcKey === dstKey) { + // Keys match: update if values differ + const srcValue = srcAttrs[srcIdx + 1]; + const dstValue = dstAttrs[dstIdx + 1]; if (srcValue !== dstValue) { - record(dstKey, srcValue); + record(srcKey, srcValue); + // Update in place doesn't change array length } - srcKey = srcIdx < srcLength ? srcAttrs[srcIdx++] : null; - dstKey = dstIdx < dstLength ? dstAttrs[dstIdx++] : null; + srcIdx += 2; // skip key and value + dstIdx += 2; // skip key and value } else if (srcKey < dstKey) { - // Destination is missing the key, so we need to insert it. + // Source has a key not in destination: add it + const srcValue = srcAttrs[srcIdx + 1]; if (isJsxPropertyAnEventName(srcKey)) { - // Special handling for events patchEventDispatch = true; - recordJsxEvent(srcKey, srcAttrs[srcIdx]); + recordJsxEvent(srcKey, srcValue); } else { - record(srcKey, srcAttrs[srcIdx]); + record(srcKey, srcValue); } - - srcIdx++; - // advance srcValue - srcKey = srcIdx < srcLength ? srcAttrs[srcIdx++] : null; - // we need to increment dstIdx too, because we added destination key and value to the VNode - // and dstAttrs is a reference to the VNode - dstIdx++; - dstLength = dstAttrs.length; - dstKey = dstIdx < dstLength ? dstAttrs[dstIdx++] : null; + srcIdx += 2; // skip key and value + // After addition, dstAttrs grows at sorted position (before dstIdx), advance dstIdx + dstIdx += 2; } else { - // Source is missing the key, so we need to remove it from destination. + // Destination has a key not in source: remove it (dstKey > srcKey) if (isHtmlAttributeAnEventName(dstKey)) { - patchEventDispatch = true; - dstIdx++; + // HTML event attributes are immutable and not removed from DOM + dstIdx += 2; // skip key and value } else { - record(dstKey!, null); - dstIdx--; + record(dstKey, null); + // After removal, dstAttrs shrinks at dstIdx, so don't advance dstIdx } - dstKey = dstIdx < dstLength ? dstAttrs[dstIdx++] : null; } } return patchEventDispatch; diff --git a/packages/qwik/src/core/client/vnode-diff.unit.tsx b/packages/qwik/src/core/client/vnode-diff.unit.tsx index 6216dc05f53..a549387739a 100644 --- a/packages/qwik/src/core/client/vnode-diff.unit.tsx +++ b/packages/qwik/src/core/client/vnode-diff.unit.tsx @@ -840,6 +840,328 @@ describe('vNode-diff', () => { expect(firstChild).toMatchVDOM(); }); }); + + describe('edge cases and complex scenarios', () => { + it('should handle empty source to multiple attributes', () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('span', {}, null, [], 0, null)); + const test = _jsxSorted( + 'span', + { about: 'a', class: 'test', id: 'b', title: 't' }, + null, + [], + 0, + null + ); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + expect(vnode_getFirstChild(vParent)).toMatchVDOM(test); + }); + + it('should handle multiple attributes to empty', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted( + 'span', + { about: 'a', class: 'test', id: 'b', title: 't' }, + null, + [], + 0, + null + ) + ); + const test = _jsxSorted('span', {}, null, [], 0, null); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + expect(vnode_getFirstChild(vParent)).toMatchVDOM(test); + }); + + it('should handle removing first and last attributes while keeping middle', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted( + 'span', + { about: 'a', class: 'test', id: 'b', title: 't' }, + null, + [], + 0, + null + ) + ); + const test = _jsxSorted('span', { class: 'test', id: 'b' }, null, [], 0, null); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + expect(vnode_getFirstChild(vParent)).toMatchVDOM(test); + }); + + it('should handle adding first and last attributes while keeping middle', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted('span', { class: 'test', id: 'b' }, null, [], 0, null) + ); + const test = _jsxSorted( + 'span', + { about: 'a', class: 'test', id: 'b', title: 't' }, + null, + [], + 0, + null + ); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + expect(vnode_getFirstChild(vParent)).toMatchVDOM(test); + }); + + it('should handle interleaved add/remove/update operations', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted('span', { about: 'old', class: 'old', name: 'remove' }, null, [], 0, null) + ); + const test = _jsxSorted( + 'span', + { about: 'new', id: 'add', title: 'add2' }, + null, + [], + 0, + null + ); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + expect(vnode_getFirstChild(vParent)).toMatchVDOM(test); + }); + + it('should handle multiple consecutive removals at beginning', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted('span', { a: '1', b: '2', c: '3', z: '26' }, null, [], 0, null) + ); + const test = _jsxSorted('span', { z: '26' }, null, [], 0, null); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + expect(vnode_getFirstChild(vParent)).toMatchVDOM(test); + }); + + it('should handle multiple consecutive additions at beginning', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted('span', { z: '26' }, null, [], 0, null) + ); + const test = _jsxSorted('span', { a: '1', b: '2', c: '3', z: '26' }, null, [], 0, null); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + expect(vnode_getFirstChild(vParent)).toMatchVDOM(test); + }); + + it('should handle alternating src and dst keys', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted('span', { b: '2', d: '4', f: '6' }, null, [], 0, null) + ); + const test = _jsxSorted('span', { a: '1', c: '3', e: '5' }, null, [], 0, null); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + expect(vnode_getFirstChild(vParent)).toMatchVDOM(test); + }); + + it('should handle mixed normal attrs and events without special prefixes', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted( + 'span', + { about: 'a', class: 'old', onClick$: () => null }, + null, + [], + 0, + null + ) + ); + const test = _jsxSorted( + 'span', + { class: 'new', id: 'b', onMouseOver$: () => null }, + null, + [], + 0, + null + ); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + const element = vnode_getNode(firstChild) as QElement; + expect(element.qDispatchEvent).toBeDefined(); + }); + + it('should handle multiple HTML event attributes being removed', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted( + 'span', + { 'on:click': 'a', 'on:focus': 'b', 'on:mouseover': 'c', id: 'test' }, + null, + [], + 0, + null + ) + ); + const test = _jsxSorted('span', { id: 'test' }, null, [], 0, null); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(test); + const element = vnode_getNode(firstChild) as QElement; + expect(element.qDispatchEvent).not.toBeDefined(); + }); + + it('should handle replacing HTML events with JSX events', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted('span', { 'on:click': 'a', 'on:mouseover': 'b' }, null, [], 0, null) + ); + const test = _jsxSorted( + 'span', + { onClick$: () => null, onMouseOver$: () => null }, + null, + [], + 0, + null + ); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + const element = vnode_getNode(firstChild) as QElement; + expect(element.qDispatchEvent).toBeDefined(); + }); + + it('should handle all attributes being updated to different values', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted('span', { a: '1', b: '2', c: '3' }, null, [], 0, null) + ); + const test = _jsxSorted('span', { a: '10', b: '20', c: '30' }, null, [], 0, null); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + expect(vnode_getFirstChild(vParent)).toMatchVDOM(test); + }); + + it('should handle attributes with same values (no updates)', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted('span', { a: '1', b: '2', c: '3' }, null, [], 0, null) + ); + const test = _jsxSorted('span', { a: '1', b: '2', c: '3' }, null, [], 0, null); + vnode_diff(container, test, vParent, null); + // Journal should be empty since no changes were made + expect(container.$journal$.length).toEqual(0); + expect(vnode_getFirstChild(vParent)).toMatchVDOM(test); + }); + + it('should handle single attribute changes in various positions', () => { + // Change first + const { vParent: vParent1, container: container1 } = vnode_fromJSX( + _jsxSorted('span', { a: '1', b: '2', c: '3' }, null, [], 0, null) + ); + const test1 = _jsxSorted('span', { a: 'NEW', b: '2', c: '3' }, null, [], 0, null); + vnode_diff(container1, test1, vParent1, null); + vnode_applyJournal(container1.$journal$); + expect(vnode_getFirstChild(vParent1)).toMatchVDOM(test1); + + // Change middle + const { vParent: vParent2, container: container2 } = vnode_fromJSX( + _jsxSorted('span', { a: '1', b: '2', c: '3' }, null, [], 0, null) + ); + const test2 = _jsxSorted('span', { a: '1', b: 'NEW', c: '3' }, null, [], 0, null); + vnode_diff(container2, test2, vParent2, null); + vnode_applyJournal(container2.$journal$); + expect(vnode_getFirstChild(vParent2)).toMatchVDOM(test2); + + // Change last + const { vParent: vParent3, container: container3 } = vnode_fromJSX( + _jsxSorted('span', { a: '1', b: '2', c: '3' }, null, [], 0, null) + ); + const test3 = _jsxSorted('span', { a: '1', b: '2', c: 'NEW' }, null, [], 0, null); + vnode_diff(container3, test3, vParent3, null); + vnode_applyJournal(container3.$journal$); + expect(vnode_getFirstChild(vParent3)).toMatchVDOM(test3); + }); + + it('should handle scoped events with different scopes', () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('span', {}, null, [], 0, null)); + const test = _jsxSorted( + 'span', + { 'document:onClick$': () => null, 'window:onScroll$': () => null }, + null, + [], + 0, + null + ); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + const element = vnode_getNode(firstChild) as QElement; + expect(element.qDispatchEvent).toBeDefined(); + }); + + it('should handle complex mix: add/remove/update/keep', () => { + const { vParent, container } = vnode_fromJSX( + _jsxSorted( + 'span', + { + about: 'remove', + class: 'update-old', + id: 'keep', + name: 'also-remove', + }, + null, + [], + 0, + null + ) + ); + const test = _jsxSorted( + 'span', + { + class: 'update-new', + id: 'keep', + onClick$: () => null, + title: 'add', + }, + null, + [], + 0, + null + ); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + const element = vnode_getNode(firstChild) as QElement; + expect(element.qDispatchEvent).toBeDefined(); + }); + + it('should handle updating signal attribute values', () => { + const { vParent, container } = vnode_fromJSX(_jsxSorted('span', {}, null, [], 0, null)); + const signal1 = createSignal('value1'); + const test1 = _jsxSorted('span', { class: signal1 }, null, [], 0, null); + vnode_diff(container, test1, vParent, null); + vnode_applyJournal(container.$journal$); + const firstChild = vnode_getFirstChild(vParent); + expect(firstChild).toMatchVDOM(); + + // Update with different signal + const signal2 = createSignal('value2'); + const test2 = _jsxSorted('span', { class: signal2 }, null, [], 0, null); + container.$journal$ = []; + vnode_diff(container, test2, vParent, null); + vnode_applyJournal(container.$journal$); + expect(firstChild).toMatchVDOM(); + }); + + it('should handle very long attribute list', () => { + const manyAttrs: Record = {}; + const manyAttrsUpdated: Record = {}; + for (let i = 0; i < 50; i++) { + const key = `attr${String(i).padStart(3, '0')}`; + manyAttrs[key] = `value${i}`; + manyAttrsUpdated[key] = `updated${i}`; + } + + const { vParent, container } = vnode_fromJSX( + _jsxSorted('span', manyAttrs, null, [], 0, null) + ); + const test = _jsxSorted('span', manyAttrsUpdated, null, [], 0, null); + vnode_diff(container, test, vParent, null); + vnode_applyJournal(container.$journal$); + expect(vnode_getFirstChild(vParent)).toMatchVDOM(test); + }); + }); }); });