From 603f22a7bb2832baa9b97663862965ee3920ed82 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Tue, 6 Jun 2017 14:31:57 -0700 Subject: [PATCH 1/6] Set row height when char size is changed When zoomed in line-height and actual rendered row height differs. This can be seen in the viewport being too large when zoomed in right now but also has additional impacts in the new selection model #207 --- src/xterm.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/xterm.js b/src/xterm.js index f92a25f7..cd9f127e 100644 --- a/src/xterm.js +++ b/src/xterm.js @@ -763,7 +763,8 @@ Terminal.loadAddon = function(addon, callback) { Terminal.prototype.updateCharSizeCSS = function() { this.charSizeStyleElement.textContent = `.xterm-wide-char{width:${this.charMeasure.width * 2}px;}` + - `.xterm-normal-char{width:${this.charMeasure.width}px;}` + `.xterm-normal-char{width:${this.charMeasure.width}px;}` + + `.xterm-rows > div{height:${this.charMeasure.height}px;}`; } /** From 29ac6a153ee9b0489eb45daa45a537d0416fe5d5 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 9 Jun 2017 11:48:10 -0700 Subject: [PATCH 2/6] 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 3/6] 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', () => { From 9246d524cb54f1453ccddb5249857e9b3ed5ef75 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 9 Jun 2017 16:35:21 -0700 Subject: [PATCH 4/6] Use a single element for the middle of the selection This reduces the number of DOM nodes used for the selection dramatically. --- src/Renderer.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Renderer.ts b/src/Renderer.ts index f0f50f61..7d8c6f85 100644 --- a/src/Renderer.ts +++ b/src/Renderer.ts @@ -353,9 +353,8 @@ export class Renderer { const endCol = viewportCappedStartRow === viewportCappedEndRow ? end[0] : this._terminal.cols; documentFragment.appendChild(this._createSelectionElement(viewportCappedStartRow, startCol, endCol)); // Draw middle rows - for (let i = viewportCappedStartRow + 1; i < viewportCappedEndRow; i++) { - documentFragment.appendChild(this._createSelectionElement(i, 0, this._terminal.cols)); - } + const middleRowsCount = viewportCappedEndRow - viewportCappedStartRow - 1; + documentFragment.appendChild(this._createSelectionElement(viewportCappedStartRow + 1, 0, this._terminal.cols, middleRowsCount)); // Draw final row if (viewportCappedStartRow !== viewportCappedEndRow) { // Only draw viewportEndRow if it's not the same as viewporttartRow @@ -371,9 +370,9 @@ export class Renderer { * @param colStart The start column. * @param colEnd The end columns. */ - private _createSelectionElement(row: number, colStart: number, colEnd: number): HTMLElement { + private _createSelectionElement(row: number, colStart: number, colEnd: number, rowCount: number = 1): HTMLElement { const element = document.createElement('div'); - element.style.height = `${this._terminal.charMeasure.height}px`; + element.style.height = `${rowCount * this._terminal.charMeasure.height}px`; element.style.top = `${row * this._terminal.charMeasure.height}px`; element.style.left = `${colStart * this._terminal.charMeasure.width}px`; element.style.width = `${this._terminal.charMeasure.width * (colEnd - colStart)}px`; From 346b5177bcb261b703ea60be7d0265af1f30e992 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 9 Jun 2017 17:12:00 -0700 Subject: [PATCH 5/6] Retain wrapped line state when copying Fixes #443 --- src/InputHandler.ts | 2 +- src/SelectionManager.ts | 16 ++++++++++++++-- src/xterm.js | 17 +++++++++++++---- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/InputHandler.ts b/src/InputHandler.ts index e1223e7e..34131a15 100644 --- a/src/InputHandler.ts +++ b/src/InputHandler.ts @@ -57,7 +57,7 @@ export class InputHandler implements IInputHandler { this._terminal.y++; if (this._terminal.y > this._terminal.scrollBottom) { this._terminal.y--; - this._terminal.scroll(); + this._terminal.scroll(true); } } else { if (ch_width === 2) // FIXME: check for xterm behavior diff --git a/src/SelectionManager.ts b/src/SelectionManager.ts index 906d5b01..314897ec 100644 --- a/src/SelectionManager.ts +++ b/src/SelectionManager.ts @@ -180,12 +180,24 @@ export class SelectionManager extends EventEmitter { // Get middle rows for (let i = start[1] + 1; i <= end[1] - 1; i++) { - result.push(this._translateBufferLineToString(this._buffer.get(i), true)); + const bufferLine = this._buffer.get(i); + const lineText = this._translateBufferLineToString(bufferLine, true); + if (bufferLine.isWrapped) { + result[result.length - 1] += lineText; + } else { + result.push(lineText); + } } // Get final row if (start[1] !== end[1]) { - result.push(this._translateBufferLineToString(this._buffer.get(end[1]), true, 0, end[0])); + const bufferLine = this._buffer.get(end[1]); + const lineText = this._translateBufferLineToString(bufferLine, true, 0, end[0]); + if (bufferLine.isWrapped) { + result[result.length - 1] += lineText; + } else { + result.push(lineText); + } } // Format string by replacing non-breaking space chars with regular spaces diff --git a/src/xterm.js b/src/xterm.js index a476fd7e..f5186405 100644 --- a/src/xterm.js +++ b/src/xterm.js @@ -1116,8 +1116,10 @@ Terminal.prototype.showCursor = function() { /** * Scroll the terminal down 1 row, creating a blank line. + * @param {boolean} isWrapped Whether the new line is wrapped from the previous + * line. */ -Terminal.prototype.scroll = function() { +Terminal.prototype.scroll = function(isWrapped) { var row; // Make room for the new row in lines @@ -1144,10 +1146,10 @@ Terminal.prototype.scroll = function() { if (row === this.lines.length) { // Optimization: pushing is faster than splicing when they amount to the same behavior - this.lines.push(this.blankLine()); + this.lines.push(this.blankLine(undefined, isWrapped)); } else { // add our new line - this.lines.splice(row, 0, this.blankLine()); + this.lines.splice(row, 0, this.blankLine(undefined, isWrapped)); } if (this.scrollTop !== 0) { @@ -2106,8 +2108,9 @@ Terminal.prototype.eraseLine = function(y) { /** * Return the data array of a blank line * @param {number} cur First bunch of data for each "blank" character. + * @param {boolean} isWrapped Whether the new line is wrapped from the previous line. */ -Terminal.prototype.blankLine = function(cur) { +Terminal.prototype.blankLine = function(cur, isWrapped) { var attr = cur ? this.eraseAttr() : this.defAttr; @@ -2116,6 +2119,12 @@ Terminal.prototype.blankLine = function(cur) { , line = [] , i = 0; + // TODO: It is not ideal that this is a property on an array, a buffer line + // class should be added that will hold this data and other useful functions. + if (isWrapped) { + line.isWrapped = isWrapped; + } + for (; i < this.cols; i++) { line[i] = ch; } From 2a0f44b0fbee895aa1556bee2f56add7a5286f75 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Mon, 12 Jun 2017 14:51:58 -0700 Subject: [PATCH 6/6] Prevent mousedown event default selection behavior Fixes #698 --- src/SelectionManager.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/SelectionManager.ts b/src/SelectionManager.ts index 906d5b01..7cb99ccd 100644 --- a/src/SelectionManager.ts +++ b/src/SelectionManager.ts @@ -336,6 +336,9 @@ export class SelectionManager extends EventEmitter { return; } + // Tell the browser not to start a regular selection + event.preventDefault(); + // Reset drag scroll state this._dragScrollAmount = 0;