fix: issue with Node.textContent returning text in comment nodes #1461
Conversation
jodarove
force-pushed
the
jodarove/textContent-on-comment-nodes
branch
from
abf4c14
to
be3df86
Compare
|
|
||
| export function getTextContent(node: Node): string { | ||
| switch (node.nodeType) { | ||
| case ELEMENT_NODE: { | ||
| const childNodes = getFilteredChildNodes(node); | ||
| let content = ''; | ||
| for (let i = 0, len = childNodes.length; i < len; i += 1) { | ||
| content += getTextContent(childNodes[i]); | ||
| if (childNodes[i].nodeType !== COMMENT_NODE) { |
There was a problem hiding this comment.
Should we be more conservative here and implement a whitelist for Node.TEXT_NODE?
There was a problem hiding this comment.
it is not about text node only... elements and eveyething that can have a text inside is valid here.
There was a problem hiding this comment.
another optimization here is to cache the childNodes[i] in a const, instead of accessing it twice in the same block.
| @@ -0,0 +1,16 @@ | |||
| describe('Node.textContent', () => { | |||
| it('should not return comment text when Node.nodeType is ELEMENT_NODE', () => { | |||
There was a problem hiding this comment.
add another test that has the context in the second level, not as a root element, just to make sure that we cover that too.
|
|
||
| elm.appendChild(secondLevelElement); | ||
|
|
||
| elm.appendChild(document.createComment('Some other comment')); |
There was a problem hiding this comment.
Nit: I think that using innerHTML to construct the DOM would make this test easier to understand.
| @@ -476,7 +477,9 @@ const NodePatchDescriptors = { | |||
| const childNodes = getInternalChildNodes(this); | |||
| let textContent = ''; | |||
| for (let i = 0, len = childNodes.length; i < len; i += 1) { | |||
| textContent += getTextContent(childNodes[i]); | |||
| if (childNodes[i].nodeType !== COMMENT_NODE) { | |||
| textContent += getTextContent(childNodes[i]); | |||
There was a problem hiding this comment.
same... cache childNodes[i] ref
|
merged because perf and compare was failing because of other reasons. |
Details
From https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent:
This pr fixes the current implementation to match the standard.
Does this PR introduce breaking changes?
No, it does not introduce breaking changes.