From 7b8ee62c169c323219067dc6156ef6c51bbc8399 Mon Sep 17 00:00:00 2001 From: HcySunYang Date: Thu, 21 Jan 2021 15:42:26 +0800 Subject: [PATCH 1/4] fix(rumtime-core): custom props should be cloned when cloning a hoisted DOM --- packages/runtime-core/src/renderer.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/runtime-core/src/renderer.ts b/packages/runtime-core/src/renderer.ts index 898ae077d78..535d1083e25 100644 --- a/packages/runtime-core/src/renderer.ts +++ b/packages/runtime-core/src/renderer.ts @@ -712,7 +712,14 @@ function baseCreateRenderer( // Only static vnodes can be reused, so its mounted DOM nodes should be // exactly the same, and we can simply do a clone here. // only do this in production since cloned trees cannot be HMR updated. - el = vnode.el = hostCloneNode(vnode.el) + el = hostCloneNode(vnode.el) + // #3072 + // should clone custom DOM props, + // one of the use cases is that the input tag or other form tags are hoisted, + // and for performance reasons, we intentionally avoid using the hostPatchProp + el._value = vnode.el._value + + vnode.el = el } else { el = vnode.el = hostCreateElement( vnode.type as string, From 0a054db620ab6b0ccf53ad55bec51ef7c5b7e8e0 Mon Sep 17 00:00:00 2001 From: HcySunYang Date: Mon, 15 Feb 2021 13:14:01 +0800 Subject: [PATCH 2/4] fix: don't pollute the core renderer & add tests --- packages/runtime-core/src/renderer.ts | 9 +-------- packages/runtime-dom/__tests__/nodeOps.spec.ts | 12 ++++++++++++ packages/runtime-dom/src/nodeOps.ts | 12 +++++++++++- 3 files changed, 24 insertions(+), 9 deletions(-) create mode 100644 packages/runtime-dom/__tests__/nodeOps.spec.ts diff --git a/packages/runtime-core/src/renderer.ts b/packages/runtime-core/src/renderer.ts index 535d1083e25..24e7c0f0b8b 100644 --- a/packages/runtime-core/src/renderer.ts +++ b/packages/runtime-core/src/renderer.ts @@ -712,14 +712,7 @@ function baseCreateRenderer( // Only static vnodes can be reused, so its mounted DOM nodes should be // exactly the same, and we can simply do a clone here. // only do this in production since cloned trees cannot be HMR updated. - el = hostCloneNode(vnode.el) - // #3072 - // should clone custom DOM props, - // one of the use cases is that the input tag or other form tags are hoisted, - // and for performance reasons, we intentionally avoid using the hostPatchProp - el._value = vnode.el._value - - vnode.el = el + vnode.el = hostCloneNode(vnode.el) } else { el = vnode.el = hostCreateElement( vnode.type as string, diff --git a/packages/runtime-dom/__tests__/nodeOps.spec.ts b/packages/runtime-dom/__tests__/nodeOps.spec.ts new file mode 100644 index 00000000000..3f72379d624 --- /dev/null +++ b/packages/runtime-dom/__tests__/nodeOps.spec.ts @@ -0,0 +1,12 @@ +import { nodeOps } from '../src/nodeOps' + +describe('nodeOps', () => { + test('the _value property should be cloned', () => { + const el = nodeOps.createElement('input') as HTMLDivElement & { + _value: any + } + el._value = 1 + const cloned = nodeOps.cloneNode!(el) as HTMLDivElement & { _value: any } + expect(cloned._value).toBe(1) + }) +}) diff --git a/packages/runtime-dom/src/nodeOps.ts b/packages/runtime-dom/src/nodeOps.ts index 72aa25a3930..b3eaa57d817 100644 --- a/packages/runtime-dom/src/nodeOps.ts +++ b/packages/runtime-dom/src/nodeOps.ts @@ -47,7 +47,17 @@ export const nodeOps: Omit, 'patchProp'> = { }, cloneNode(el) { - return el.cloneNode(true) + const cloned = el.cloneNode(true) + // #3072 + // should clone the custom DOM props, + // in `patchDOMProp`, we store the actual value in the `el._value` property, + // but these elements may be hoisted, and the hoisted elements will be cloned in the prod env, + // so we should keep it when cloning, and in the future, + // we may clone more custom DOM props when necessary, not just `_value`. + if (cloned.nodeType === Node.ELEMENT_NODE) { + ;(cloned as any)._value = (el as any)._value + } + return cloned }, // __UNSAFE__ From 47c44c6493ab1c1092790ceb290c215d73a6b447 Mon Sep 17 00:00:00 2001 From: HcySunYang Date: Mon, 15 Feb 2021 14:37:05 +0800 Subject: [PATCH 3/4] fix: set el --- packages/runtime-core/src/renderer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/runtime-core/src/renderer.ts b/packages/runtime-core/src/renderer.ts index 24e7c0f0b8b..898ae077d78 100644 --- a/packages/runtime-core/src/renderer.ts +++ b/packages/runtime-core/src/renderer.ts @@ -712,7 +712,7 @@ function baseCreateRenderer( // Only static vnodes can be reused, so its mounted DOM nodes should be // exactly the same, and we can simply do a clone here. // only do this in production since cloned trees cannot be HMR updated. - vnode.el = hostCloneNode(vnode.el) + el = vnode.el = hostCloneNode(vnode.el) } else { el = vnode.el = hostCreateElement( vnode.type as string, From 134578e60c2ac35bd06ab674a0972e1ac91d32c2 Mon Sep 17 00:00:00 2001 From: Evan You Date: Thu, 25 Mar 2021 10:21:16 -0400 Subject: [PATCH 4/4] chore: adjust check and comments --- packages/runtime-dom/src/nodeOps.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/runtime-dom/src/nodeOps.ts b/packages/runtime-dom/src/nodeOps.ts index b3eaa57d817..06e59221113 100644 --- a/packages/runtime-dom/src/nodeOps.ts +++ b/packages/runtime-dom/src/nodeOps.ts @@ -49,12 +49,15 @@ export const nodeOps: Omit, 'patchProp'> = { cloneNode(el) { const cloned = el.cloneNode(true) // #3072 - // should clone the custom DOM props, - // in `patchDOMProp`, we store the actual value in the `el._value` property, - // but these elements may be hoisted, and the hoisted elements will be cloned in the prod env, - // so we should keep it when cloning, and in the future, - // we may clone more custom DOM props when necessary, not just `_value`. - if (cloned.nodeType === Node.ELEMENT_NODE) { + // - in `patchDOMProp`, we store the actual value in the `el._value` property. + // - normally, elements using `:value` bindings will not be hoisted, but if + // the bound value is a constant, e.g. `:value="true"` - they do get + // hoisted. + // - in production, hoisted nodes are cloned when subsequent inserts, but + // cloneNode() does not copy the custom property we attached. + // - This may need to account for other custom DOM properties we attach to + // elements in addition to `_value` in the future. + if (`_value` in el) { ;(cloned as any)._value = (el as any)._value } return cloned