From 08fd050c3f9bfd3eafe14ebfb41e1426bfaff973 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 31 Mar 2017 01:48:35 -0700 Subject: [PATCH 1/5] Support multiple link matches in a single row Fixes #612 --- src/Linkifier.ts | 98 +++++++++++++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 42 deletions(-) diff --git a/src/Linkifier.ts b/src/Linkifier.ts index d9a16940..85d1bd5e 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -155,16 +155,17 @@ export class Linkifier { const text = row.textContent; for (let i = 0; i < this._linkMatchers.length; i++) { const matcher = this._linkMatchers[i]; - const uri = this._findLinkMatch(text, matcher.regex, matcher.matchIndex); - if (uri) { - const linkElement = this._doLinkifyRow(rowIndex, uri, matcher.handler, matcher.id === HYPERTEXT_LINK_MATCHER_ID); + const linkElements = this._doLinkifyRow(row, matcher); + if (linkElements.length > 0) { // Fire validation callback - if (linkElement && matcher.validationCallback) { - matcher.validationCallback(uri, isValid => { - if (!isValid) { - linkElement.classList.add(INVALID_LINK_CLASS); - } - }); + if (matcher.validationCallback) { + for (let j = 0; j < linkElements.length; j++) { + matcher.validationCallback(linkElements[j].textContent, isValid => { + if (!isValid) { + linkElements[j].classList.add(INVALID_LINK_CLASS); + } + }); + } } // Only allow a single LinkMatcher to trigger on any given row. return; @@ -174,22 +175,32 @@ export class Linkifier { /** * Linkifies a row given a specific handler. - * @param {number} rowIndex The index of the row to linkify. - * @param {string} uri The uri that has been found. - * @param {handler} handler The handler to trigger when the link is triggered. + * @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. */ - private _doLinkifyRow(rowIndex: number, uri: string, handler: LinkMatcherHandler, isHttpLinkMatcher: boolean): HTMLElement { + private _doLinkifyRow(row: HTMLElement, matcher: LinkMatcher): HTMLElement[] { // Iterate over nodes as we want to consider text nodes - const nodes = this._rows[rowIndex].childNodes; + let result = []; + const isHttpLinkMatcher = matcher.id === HYPERTEXT_LINK_MATCHER_ID; + const nodes = row.childNodes; + + // Find the first match + let match = row.textContent.match(matcher.regex); + if (!match || match.length === 0) { + return result; + } + let uri = match[typeof matcher.matchIndex !== 'number' ? 0 : matcher.matchIndex]; + // Set the next searches start index + let rowStartIndex = match.index + uri.length; + for (let i = 0; i < nodes.length; i++) { const node = nodes[i]; const searchIndex = node.textContent.indexOf(uri); if (searchIndex >= 0) { - const linkElement = this._createAnchorElement(uri, handler, isHttpLinkMatcher); + const linkElement = this._createAnchorElement(uri, matcher.handler, isHttpLinkMatcher); if (node.textContent.length === uri.length) { // Matches entire string - if (node.nodeType === 3 /*Node.TEXT_NODE*/) { this._replaceNode(node, linkElement); } else { @@ -203,25 +214,22 @@ export class Linkifier { } } else { // Matches part of string - this._replaceNodeSubstringWithNode(node, linkElement, uri, searchIndex); + const nodesAdded = this._replaceNodeSubstringWithNode(node, linkElement, uri, searchIndex); + // No need to consider the new nodes + i += nodesAdded - 1; } - return linkElement; + result.push(linkElement); + + // Find the next match + match = row.textContent.substring(rowStartIndex).match(matcher.regex); + if (!match || match.length === 0) { + return result; + } + uri = match[typeof matcher.matchIndex !== 'number' ? 0 : matcher.matchIndex]; + rowStartIndex += match.index + uri.length; } } - } - - /** - * Finds a link match in a piece of text. - * @param {string} text The text to search. - * @param {number} matchIndex The regex match index of the link. - * @return {string} The matching URI or null if not found. - */ - private _findLinkMatch(text: string, regex: RegExp, matchIndex?: number): string { - const match = text.match(regex); - if (!match || match.length === 0) { - return null; - } - return match[typeof matchIndex !== 'number' ? 0 : matchIndex]; + return result; } /** @@ -274,8 +282,9 @@ export class Linkifier { * @param {Node} newNode The new node to insert. * @param {string} substring The substring to replace. * @param {number} substringIndex The index of the substring within the string. + * @return The number of nodes to skip when searching for the next uri. */ - private _replaceNodeSubstringWithNode(targetNode: Node, newNode: Node, substring: string, substringIndex: number): void { + 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]; @@ -284,7 +293,7 @@ export class Linkifier { // 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 !== Node.TEXT_NODE) { + if (node.childNodes.length === 0 && node.nodeType !== 3/*Node.TEXT_NODE*/) { throw new Error('targetNode must be a text node or only contain a single text node'); } @@ -295,18 +304,23 @@ export class Linkifier { const rightText = fullText.substring(substring.length); const rightTextNode = this._document.createTextNode(rightText); this._replaceNode(node, newNode, rightTextNode); - } else if (substringIndex === targetNode.textContent.length - substring.length) { + return 0; + } + + if (substringIndex === targetNode.textContent.length - substring.length) { // Replace with const leftText = fullText.substring(0, substringIndex); const leftTextNode = this._document.createTextNode(leftText); this._replaceNode(node, leftTextNode, newNode); - } else { - // Replace with - const leftText = fullText.substring(0, substringIndex); - 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); + return 1; } + + // Replace with + const leftText = fullText.substring(0, substringIndex); + 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); + return 1; } } From d8140097214cc17d093cf9a14b3be5125ed3a571 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 31 Mar 2017 01:48:51 -0700 Subject: [PATCH 2/5] Add tests --- src/Linkifier.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/Linkifier.test.ts b/src/Linkifier.test.ts index ca393c21..f70718ea 100644 --- a/src/Linkifier.test.ts +++ b/src/Linkifier.test.ts @@ -89,6 +89,21 @@ describe('Linkifier', () => { // Allow time for the click to be performed setTimeout(() => done(), 10); }); + + it('should trigger for multiple link matches on one row', done => { + addRow('test test'); + let count = 0; + linkifier.registerLinkMatcher(/test/, () => assert.fail(), { + validationCallback: (url, cb) => { + count += 1; + if (count === 2) { + done(); + } + cb(false); + } + }); + linkifier.linkifyRow(0); + }); }); describe('priority', () => { From 5546baa927e9dd97e527ac6da763f7a9dfcc9d2e Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 31 Mar 2017 01:54:21 -0700 Subject: [PATCH 3/5] Fix edge case --- src/Linkifier.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Linkifier.ts b/src/Linkifier.ts index 85d1bd5e..f305945c 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -207,7 +207,7 @@ export class Linkifier { const element = (node); if (element.nodeName === 'A') { // This row has already been linkified - return; + return result; } element.innerHTML = ''; element.appendChild(linkElement); @@ -216,7 +216,7 @@ export class Linkifier { // Matches part of string const nodesAdded = this._replaceNodeSubstringWithNode(node, linkElement, uri, searchIndex); // No need to consider the new nodes - i += nodesAdded - 1; + i += nodesAdded; } result.push(linkElement); From 8c2db8ddb8799771b45500af9dbcc6f4f5fbf08e Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 31 Mar 2017 02:10:07 -0700 Subject: [PATCH 4/5] More tests --- src/Linkifier.test.ts | 45 +++++++++++++++++++++++++++++++++++++------ src/Linkifier.ts | 2 +- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/Linkifier.test.ts b/src/Linkifier.test.ts index f70718ea..16e388fa 100644 --- a/src/Linkifier.test.ts +++ b/src/Linkifier.test.ts @@ -36,9 +36,9 @@ describe('Linkifier', () => { }); }); - function addRow(text: string) { + function addRow(html: string) { const element = document.createElement('div'); - element.textContent = text; + element.innerHTML = html; container.appendChild(element); rows.push(element); } @@ -49,7 +49,8 @@ describe('Linkifier', () => { element.dispatchEvent(event); } - function assertLinkifiesEntireRow(uri: string, done: MochaDone) { + describe('http links', () => { + function assertLinkifiesEntireRow(uri: string, done: MochaDone) { addRow(uri); linkifier.linkifyRow(0); setTimeout(() => { @@ -57,12 +58,44 @@ describe('Linkifier', () => { assert.equal((rows[0].firstChild).textContent, uri); done(); }, 0); - } - - describe('http links', () => { + } it('should allow ~ character in URI path', done => assertLinkifiesEntireRow('http://foo.com/a~b#c~d?e~f', done)); }); + describe('link matcher', () => { + function assertLinkifiesRow(rowText: string, linkMatcherRegex: RegExp, expectedHtml: string, done: MochaDone) { + addRow(rowText); + linkifier.registerLinkMatcher(linkMatcherRegex, () => {}); + linkifier.linkifyRow(0); + // Allow linkify to happen + setTimeout(() => { + assert.equal(rows[0].innerHTML, expectedHtml); + done(); + }, 0); + } + it('should match a single link', done => { + assertLinkifiesRow('foo', /foo/, 'foo', done); + }); + it('should match a single link at the start of a text node', done => { + assertLinkifiesRow('foo bar', /foo/, 'foo bar', done); + }); + it('should match a single link in the middle of a text node', done => { + assertLinkifiesRow('foo bar baz', /bar/, 'foo bar baz', done); + }); + it('should match a single link at the end of a text node', done => { + assertLinkifiesRow('foo bar', /bar/, 'foo bar', done); + }); + it('should match a link after a link at the start of a text node', done => { + assertLinkifiesRow('foo bar', /foo|bar/, 'foo bar', done); + }); + it('should match a link after a link in the middle of a text node', done => { + assertLinkifiesRow('foo bar baz', /bar|baz/, 'foo bar baz', done); + }); + 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); + }); + }); + describe('validationCallback', () => { it('should enable link if true', done => { addRow('test'); diff --git a/src/Linkifier.ts b/src/Linkifier.ts index f305945c..b229c775 100644 --- a/src/Linkifier.ts +++ b/src/Linkifier.ts @@ -312,7 +312,7 @@ export class Linkifier { const leftText = fullText.substring(0, substringIndex); const leftTextNode = this._document.createTextNode(leftText); this._replaceNode(node, leftTextNode, newNode); - return 1; + return 0; } // Replace with From 64663bf4a8145b8aa0dc31cb1246fe6d110a6390 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Tue, 4 Apr 2017 09:35:52 -0700 Subject: [PATCH 5/5] Fix tests --- src/test/test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/test.js b/src/test/test.js index 716b48c4..1644e580 100644 --- a/src/test/test.js +++ b/src/test/test.js @@ -21,7 +21,8 @@ describe('xterm.js', function() { }; xterm.element = { classList: { - toggle: function(){} + toggle: function(){}, + remove: function(){} } }; });