Skip to content

Commit

Permalink
fix: avoid reading element-specific node properties of non-element no…
Browse files Browse the repository at this point in the history
…de types (#4317)

Updates VirtualNode's `props` initialization path to avoid reading
properties that we know based on `nodeType` won't be present anyway.
This should mitigate #4316 by avoiding reading the problematic `value`
prop present on certain text nodes.

I also cleaned up some missing test coverage in the impacted code.

Closes: #4316
  • Loading branch information
dbjorge authored and straker committed Feb 5, 2024
1 parent 139c553 commit a2a6935
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 26 deletions.
29 changes: 12 additions & 17 deletions lib/core/base/virtual-node/virtual-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
48 changes: 39 additions & 9 deletions test/core/base/virtual-node/virtual-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +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);
});
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[prop], undefined);
});

// 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);
});
});
}

it('should lowercase type', () => {
node = document.createElement('input');
Expand Down

0 comments on commit a2a6935

Please sign in to comment.