From 4f47b164d19486545bcf49f7fec700830d765bf3 Mon Sep 17 00:00:00 2001 From: Dan Bjorge Date: Wed, 31 Jan 2024 19:44:12 -0500 Subject: [PATCH 1/2] avoid reading node properties unnecessarily --- lib/core/base/virtual-node/virtual-node.js | 29 ++++++++----------- test/core/base/virtual-node/virtual-node.js | 32 +++++++++++++++++++++ 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/lib/core/base/virtual-node/virtual-node.js b/lib/core/base/virtual-node/virtual-node.js index 589b61237f..4a19c8b021 100644 --- a/lib/core/base/virtual-node/virtual-node.js +++ b/lib/core/base/virtual-node/virtual-node.js @@ -51,30 +51,25 @@ class VirtualNode extends AbstractVirtualNode { // add to the prototype so memory is shared across all virtual nodes get props() { if (!this._cache.hasOwnProperty('props')) { - const { - nodeType, - nodeName, - id, - multiple, - nodeValue, - value, - selected, - checked, - indeterminate - } = this.actualNode; + const { nodeType, nodeName, id, nodeValue } = this.actualNode; this._cache.props = { nodeType, nodeName: this._isXHTML ? nodeName : nodeName.toLowerCase(), id, type: this._type, - multiple, - nodeValue, - value, - selected, - checked, - indeterminate + nodeValue }; + + // We avoid reading these on node types where they won't be relevant + // to work around issues like #4316. + if (nodeType === 1) { + this._cache.props.multiple = this.actualNode.multiple; + this._cache.props.value = this.actualNode.value; + this._cache.props.selected = this.actualNode.selected; + this._cache.props.checked = this.actualNode.checked; + this._cache.props.indeterminate = this.actualNode.indeterminate; + } } return this._cache.props; diff --git a/test/core/base/virtual-node/virtual-node.js b/test/core/base/virtual-node/virtual-node.js index 771d3693b1..cab5b9fbb7 100644 --- a/test/core/base/virtual-node/virtual-node.js +++ b/test/core/base/virtual-node/virtual-node.js @@ -47,6 +47,38 @@ describe('VirtualNode', () => { assert.equal(vNode.props.selected, true); }); + describe('props.value', () => { + it("should reflect an input element's value property", () => { + node = document.createElement('input'); + let vNode = new VirtualNode(node); + assert.equal(vNode.props.value, ''); + + node.value = 'test'; + vNode = new VirtualNode(node); + assert.equal(vNode.props.value, 'test'); + }); + + it('should be undefined for a text node', () => { + node = document.createTextNode('text content'); + let vNode = new VirtualNode(node); + assert.equal(vNode.props.value, undefined); + }); + + // Regression test for #4316 + it('should be resilient to text node with un-gettable value property', () => { + node = document.createTextNode('text content'); + Object.defineProperty(node, 'value', { + get() { + throw new Error('Unqueryable value'); + } + }); + let vNode = new VirtualNode(node); + assert.throws(() => node.value); + assert.doesNotThrow(() => vNode.props.value); + assert.equal(vNode.props.value, undefined); + }); + }); + it('should lowercase type', () => { node = document.createElement('input'); node.setAttribute('type', 'COLOR'); From 447a864e3d767df90907f3216df059298fd82f24 Mon Sep 17 00:00:00 2001 From: Dan Bjorge Date: Wed, 31 Jan 2024 19:55:07 -0500 Subject: [PATCH 2/2] generalize test cases to cover all affected props --- test/core/base/virtual-node/virtual-node.js | 72 ++++++++++----------- 1 file changed, 35 insertions(+), 37 deletions(-) diff --git a/test/core/base/virtual-node/virtual-node.js b/test/core/base/virtual-node/virtual-node.js index cab5b9fbb7..338442d75e 100644 --- a/test/core/base/virtual-node/virtual-node.js +++ b/test/core/base/virtual-node/virtual-node.js @@ -37,47 +37,45 @@ describe('VirtualNode', () => { assert.equal(vNode.props.type, 'text'); }); - it('should reflect selected property', () => { - node = document.createElement('option'); - let vNode = new VirtualNode(node); - assert.equal(vNode.props.selected, false); - - node.selected = true; - vNode = new VirtualNode(node); - assert.equal(vNode.props.selected, true); - }); - - describe('props.value', () => { - it("should reflect an input element's value property", () => { - node = document.createElement('input'); - let vNode = new VirtualNode(node); - assert.equal(vNode.props.value, ''); - - node.value = 'test'; - vNode = new VirtualNode(node); - assert.equal(vNode.props.value, 'test'); - }); + for (const [prop, tagName, examplePropValue] of [ + ['value', 'input', 'test value'], + ['selected', 'option', true], + ['checked', 'input', true], + ['indeterminate', 'input', true], + ['multiple', 'select', true] + ]) { + describe(`props.${prop}`, () => { + it(`should reflect a ${tagName} element's ${prop} property`, () => { + node = document.createElement(tagName); + let vNode = new VirtualNode(node); + assert.equal(vNode.props[prop], ''); + + node[prop] = examplePropValue; + vNode = new VirtualNode(node); + assert.equal(vNode.props[prop], examplePropValue); + }); - it('should be undefined for a text node', () => { - node = document.createTextNode('text content'); - let vNode = new VirtualNode(node); - assert.equal(vNode.props.value, undefined); - }); + it('should be undefined for a text node', () => { + node = document.createTextNode('text content'); + let vNode = new VirtualNode(node); + assert.equal(vNode.props[prop], undefined); + }); - // Regression test for #4316 - it('should be resilient to text node with un-gettable value property', () => { - node = document.createTextNode('text content'); - Object.defineProperty(node, 'value', { - get() { - throw new Error('Unqueryable value'); - } + // Regression test for #4316 + it(`should be resilient to text node with un-gettable ${prop} property`, () => { + node = document.createTextNode('text content'); + Object.defineProperty(node, prop, { + get() { + throw new Error('Unqueryable value'); + } + }); + let vNode = new VirtualNode(node); + assert.throws(() => node[prop]); + assert.doesNotThrow(() => vNode.props[prop]); + assert.equal(vNode.props[prop], undefined); }); - let vNode = new VirtualNode(node); - assert.throws(() => node.value); - assert.doesNotThrow(() => vNode.props.value); - assert.equal(vNode.props.value, undefined); }); - }); + } it('should lowercase type', () => { node = document.createElement('input');