From 29ac6a153ee9b0489eb45daa45a537d0416fe5d5 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 9 Jun 2017 11:48:10 -0700 Subject: [PATCH 1/2] Fix issue with strings in wrapped spans being duplicated This was happening because the linkifier assumed that top-level spans in the terminal would only contain single text nodes Fixes #642 --- src/Linkifier.ts | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/src/Linkifier.ts b/src/Linkifier.ts index f99cdaa4..bc4949b1 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -200,7 +200,7 @@ export class Linkifier { * Linkifies a row given a specific handler. * @param {HTMLElement} row The row to linkify. * @param {LinkMatcher} matcher The link matcher for this line. - * @return The link element if it was added, otherwise undefined. + * @return The link element(s) that were added. */ private _doLinkifyRow(row: HTMLElement, matcher: LinkMatcher): HTMLElement[] { // Iterate over nodes as we want to consider text nodes @@ -235,8 +235,21 @@ export class Linkifier { element.innerHTML = ''; element.appendChild(linkElement); } + } else if (node.childNodes.length > 1) { + // Matches part of string in an element with multiple child nodes + for (let j = 0; j < node.childNodes.length; j++) { + const childNode = node.childNodes[j]; + const childSearchIndex = childNode.textContent.indexOf(uri); + if (childSearchIndex !== -1) { + // Match found in currentNode + this._replaceNodeSubstringWithNode(childNode, linkElement, uri, childSearchIndex); + // Don't need to count nodesAdded by replacing the node as this + // is a child node, not a top-level node. + break; + } + } } else { - // Matches part of string + // Matches part of string in a single text node const nodesAdded = this._replaceNodeSubstringWithNode(node, linkElement, uri, searchIndex); // No need to consider the new nodes i += nodesAdded; @@ -308,25 +321,26 @@ export class Linkifier { * @return The number of nodes to skip when searching for the next uri. */ private _replaceNodeSubstringWithNode(targetNode: Node, newNode: Node, substring: string, substringIndex: number): number { - let node = targetNode; - if (node.nodeType !== 3/*Node.TEXT_NODE*/) { - node = node.childNodes[0]; + // If the targetNode is a non-text node with a single child, make the child + // the new targetNode. + if (targetNode.childNodes.length === 1) { + targetNode = targetNode.childNodes[0]; } // The targetNode will be either a text node or a . The text node // (targetNode or its only-child) needs to be replaced with newNode plus new // text nodes potentially on either side. - if (node.childNodes.length === 0 && node.nodeType !== 3/*Node.TEXT_NODE*/) { + if (targetNode.nodeType !== 3/*Node.TEXT_NODE*/) { throw new Error('targetNode must be a text node or only contain a single text node'); } - const fullText = node.textContent; + const fullText = targetNode.textContent; if (substringIndex === 0) { // Replace with const rightText = fullText.substring(substring.length); const rightTextNode = this._document.createTextNode(rightText); - this._replaceNode(node, newNode, rightTextNode); + this._replaceNode(targetNode, newNode, rightTextNode); return 0; } @@ -334,7 +348,7 @@ export class Linkifier { // Replace with const leftText = fullText.substring(0, substringIndex); const leftTextNode = this._document.createTextNode(leftText); - this._replaceNode(node, leftTextNode, newNode); + this._replaceNode(targetNode, leftTextNode, newNode); return 0; } @@ -343,7 +357,7 @@ export class Linkifier { const leftTextNode = this._document.createTextNode(leftText); const rightText = fullText.substring(substringIndex + substring.length); const rightTextNode = this._document.createTextNode(rightText); - this._replaceNode(node, leftTextNode, newNode, rightTextNode); + this._replaceNode(targetNode, leftTextNode, newNode, rightTextNode); return 1; } } From ee1da73a693beb9da99e69a95a9d2dc56113d3a6 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 9 Jun 2017 11:59:18 -0700 Subject: [PATCH 2/2] Add regression test --- src/Linkifier.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Linkifier.test.ts b/src/Linkifier.test.ts index 2450ffb9..c6e59d84 100644 --- a/src/Linkifier.test.ts +++ b/src/Linkifier.test.ts @@ -119,6 +119,12 @@ describe('Linkifier', () => { it('should match a link immediately after a link at the end of a text node', done => { assertLinkifiesRow('foo barbaz', /bar|baz/, 'foo barbaz', done); }); + it('should not duplicate text after a unicode character (wrapped in a span)', done => { + // This is a regression test for an issue that came about when using + // an oh-my-zsh theme that added the large blue diamond unicode + // character (U+1F537) which caused the path to be duplicated. See #642. + assertLinkifiesRow('echo \'🔷foo\'', /foo/, 'echo \'🔷foo\'', done); + }); }); describe('validationCallback', () => {