From 692cdfa390943c0133a8fbd1a27e6b1a9ba721a8 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Thu, 22 Nov 2018 11:32:08 -0800 Subject: [PATCH 1/5] DOM renderer: Don't output empty cells at end Fixes #1609 --- src/renderer/dom/DomRendererRowFactory.ts | 27 ++++++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/renderer/dom/DomRendererRowFactory.ts b/src/renderer/dom/DomRendererRowFactory.ts index 4bb5990223..bcb0cc4863 100644 --- a/src/renderer/dom/DomRendererRowFactory.ts +++ b/src/renderer/dom/DomRendererRowFactory.ts @@ -3,7 +3,7 @@ * @license MIT */ -import { CHAR_DATA_CHAR_INDEX, CHAR_DATA_ATTR_INDEX, CHAR_DATA_WIDTH_INDEX } from '../../Buffer'; +import { CHAR_DATA_CHAR_INDEX, CHAR_DATA_ATTR_INDEX, CHAR_DATA_WIDTH_INDEX, CHAR_DATA_CODE_INDEX, NULL_CELL_CODE } from '../../Buffer'; import { FLAGS } from '../Types'; import { IBufferLine } from '../../Types'; @@ -23,17 +23,28 @@ export class DomRendererRowFactory { public createRow(lineData: IBufferLine, isCursorRow: boolean, cursorStyle: string | undefined, cursorX: number, cellWidth: number, cols: number): DocumentFragment { const fragment = this._document.createDocumentFragment(); let colCount = 0; + let nonNullCellFound = false; - for (let x = 0; x < lineData.length; x++) { + for (let x = lineData.length - 1; x >= 0; x--) { // Don't allow any buffer to the right to be displayed if (colCount >= cols) { continue; } const charData = lineData.get(x); - const char: string = charData[CHAR_DATA_CHAR_INDEX]; - const attr: number = charData[CHAR_DATA_ATTR_INDEX]; - const width: number = charData[CHAR_DATA_WIDTH_INDEX]; + + if (!nonNullCellFound) { + const code = charData[CHAR_DATA_CODE_INDEX]; + if (code === NULL_CELL_CODE && !(isCursorRow && x === cursorX)) { + continue; + } else { + nonNullCellFound = true; + } + } + + const char = charData[CHAR_DATA_CHAR_INDEX]; + const attr = charData[CHAR_DATA_ATTR_INDEX]; + const width = charData[CHAR_DATA_WIDTH_INDEX]; // The character to the left is a wide character, drawing is owned by the char at x-1 if (width === 0) { @@ -97,7 +108,11 @@ export class DomRendererRowFactory { if (bg !== 256) { charElement.classList.add(`xterm-bg-${bg}`); } - fragment.appendChild(charElement); + if (fragment.childNodes.length === 0) { + fragment.appendChild(charElement); + } else { + fragment.insertBefore(charElement, fragment.firstChild); + } colCount += width; } return fragment; From dc077a181e3b4f34c49f22f9579b1fd9dca653ff Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Thu, 22 Nov 2018 12:03:08 -0800 Subject: [PATCH 2/5] Fix tests and the colCount feature --- .../dom/DomRendererRowFactory.test.ts | 32 ++++++----------- src/renderer/dom/DomRendererRowFactory.ts | 35 +++++++++---------- 2 files changed, 28 insertions(+), 39 deletions(-) diff --git a/src/renderer/dom/DomRendererRowFactory.test.ts b/src/renderer/dom/DomRendererRowFactory.test.ts index 2c46d8cca8..03f4178445 100644 --- a/src/renderer/dom/DomRendererRowFactory.test.ts +++ b/src/renderer/dom/DomRendererRowFactory.test.ts @@ -23,11 +23,10 @@ describe('DomRendererRowFactory', () => { }); describe('createRow', () => { - it('should create an element for every character in the row', () => { + it('should not create anything for an empty row', () => { const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20); assert.equal(getFragmentHtml(fragment), - ' ' + - ' ' + '' ); }); @@ -45,8 +44,7 @@ describe('DomRendererRowFactory', () => { for (const style of ['block', 'bar', 'underline']) { const fragment = rowFactory.createRow(lineData, true, style, 0, 5, 20); assert.equal(getFragmentHtml(fragment), - ` ` + - ' ' + ` ` ); } }); @@ -65,8 +63,7 @@ describe('DomRendererRowFactory', () => { lineData.set(0, [DEFAULT_ATTR | (FLAGS.BOLD << 18), 'a', 1, 'a'.charCodeAt(0)]); const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20); assert.equal(getFragmentHtml(fragment), - 'a' + - ' ' + 'a' ); }); @@ -74,8 +71,7 @@ describe('DomRendererRowFactory', () => { lineData.set(0, [DEFAULT_ATTR | (FLAGS.ITALIC << 18), 'a', 1, 'a'.charCodeAt(0)]); const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20); assert.equal(getFragmentHtml(fragment), - 'a' + - ' ' + 'a' ); }); @@ -85,8 +81,7 @@ describe('DomRendererRowFactory', () => { lineData.set(0, [defaultAttrNoFgColor | (i << 9), 'a', 1, 'a'.charCodeAt(0)]); const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20); assert.equal(getFragmentHtml(fragment), - `a` + - ' ' + `a` ); } }); @@ -97,8 +92,7 @@ describe('DomRendererRowFactory', () => { lineData.set(0, [defaultAttrNoBgColor | (i << 0), 'a', 1, 'a'.charCodeAt(0)]); const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20); assert.equal(getFragmentHtml(fragment), - `a` + - ' ' + `a` ); } }); @@ -107,8 +101,7 @@ describe('DomRendererRowFactory', () => { lineData.set(0, [(FLAGS.INVERSE << 18) | (2 << 9) | (1 << 0), 'a', 1, 'a'.charCodeAt(0)]); const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20); assert.equal(getFragmentHtml(fragment), - 'a' + - ' ' + 'a' ); }); @@ -116,8 +109,7 @@ describe('DomRendererRowFactory', () => { lineData.set(0, [(FLAGS.INVERSE << 18) | (257 << 9) | (1 << 0), 'a', 1, 'a'.charCodeAt(0)]); const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20); assert.equal(getFragmentHtml(fragment), - 'a' + - ' ' + 'a' ); }); @@ -125,8 +117,7 @@ describe('DomRendererRowFactory', () => { lineData.set(0, [(FLAGS.INVERSE << 18) | (1 << 9) | (256 << 0), 'a', 1, 'a'.charCodeAt(0)]); const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20); assert.equal(getFragmentHtml(fragment), - 'a' + - ' ' + 'a' ); }); @@ -135,8 +126,7 @@ describe('DomRendererRowFactory', () => { lineData.set(0, [(FLAGS.BOLD << 18) | (i << 9) | (256 << 0), 'a', 1, 'a'.charCodeAt(0)]); const fragment = rowFactory.createRow(lineData, false, undefined, 0, 5, 20); assert.equal(getFragmentHtml(fragment), - `a` + - ' ' + `a` ); } }); diff --git a/src/renderer/dom/DomRendererRowFactory.ts b/src/renderer/dom/DomRendererRowFactory.ts index bcb0cc4863..5f6b49fcc8 100644 --- a/src/renderer/dom/DomRendererRowFactory.ts +++ b/src/renderer/dom/DomRendererRowFactory.ts @@ -22,26 +22,29 @@ export class DomRendererRowFactory { public createRow(lineData: IBufferLine, isCursorRow: boolean, cursorStyle: string | undefined, cursorX: number, cellWidth: number, cols: number): DocumentFragment { const fragment = this._document.createDocumentFragment(); - let colCount = 0; - let nonNullCellFound = false; - for (let x = lineData.length - 1; x >= 0; x--) { + // Find the line length first, this prevents the need to output a bunch of + // empty cells at the end. This cannot easily be integrated into the main + // loop below because of the colCount feature (which can be removed after we + // properly support reflow and disallow data to go beyond the right-side of + // the viewport). + let lineLength = 0; + for (let x = 0; x < lineData.length; x++) { + const charData = lineData.get(x); + const code = charData[CHAR_DATA_CODE_INDEX]; + if (code !== NULL_CELL_CODE || (isCursorRow && x === cursorX)) { + lineLength = x + 1; + } + } + + let colCount = 0; + for (let x = 0; x < lineLength; x++) { // Don't allow any buffer to the right to be displayed if (colCount >= cols) { continue; } const charData = lineData.get(x); - - if (!nonNullCellFound) { - const code = charData[CHAR_DATA_CODE_INDEX]; - if (code === NULL_CELL_CODE && !(isCursorRow && x === cursorX)) { - continue; - } else { - nonNullCellFound = true; - } - } - const char = charData[CHAR_DATA_CHAR_INDEX]; const attr = charData[CHAR_DATA_ATTR_INDEX]; const width = charData[CHAR_DATA_WIDTH_INDEX]; @@ -108,11 +111,7 @@ export class DomRendererRowFactory { if (bg !== 256) { charElement.classList.add(`xterm-bg-${bg}`); } - if (fragment.childNodes.length === 0) { - fragment.appendChild(charElement); - } else { - fragment.insertBefore(charElement, fragment.firstChild); - } + fragment.appendChild(charElement); colCount += width; } return fragment; From 66272eb3105ff23b84e5a79b622a72e49352419c Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 23 Nov 2018 05:39:50 -0800 Subject: [PATCH 3/5] Get the line length by going backwards instead --- src/renderer/dom/DomRendererRowFactory.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/renderer/dom/DomRendererRowFactory.ts b/src/renderer/dom/DomRendererRowFactory.ts index 5f6b49fcc8..490360bf41 100644 --- a/src/renderer/dom/DomRendererRowFactory.ts +++ b/src/renderer/dom/DomRendererRowFactory.ts @@ -29,11 +29,12 @@ export class DomRendererRowFactory { // properly support reflow and disallow data to go beyond the right-side of // the viewport). let lineLength = 0; - for (let x = 0; x < lineData.length; x++) { + for (let x = lineData.length - 1; x >= 0; x--) { const charData = lineData.get(x); const code = charData[CHAR_DATA_CODE_INDEX]; if (code !== NULL_CELL_CODE || (isCursorRow && x === cursorX)) { lineLength = x + 1; + break; } } From d9513ce50df9ea1faa8e4ef7da076824df3be171 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 23 Nov 2018 06:48:37 -0800 Subject: [PATCH 4/5] Don't bother checking beyond cols --- src/renderer/dom/DomRendererRowFactory.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/renderer/dom/DomRendererRowFactory.ts b/src/renderer/dom/DomRendererRowFactory.ts index 490360bf41..07303e2427 100644 --- a/src/renderer/dom/DomRendererRowFactory.ts +++ b/src/renderer/dom/DomRendererRowFactory.ts @@ -29,7 +29,7 @@ export class DomRendererRowFactory { // properly support reflow and disallow data to go beyond the right-side of // the viewport). let lineLength = 0; - for (let x = lineData.length - 1; x >= 0; x--) { + for (let x = Math.min(lineData.length, cols) - 1; x >= 0; x--) { const charData = lineData.get(x); const code = charData[CHAR_DATA_CODE_INDEX]; if (code !== NULL_CELL_CODE || (isCursorRow && x === cursorX)) { @@ -38,13 +38,7 @@ export class DomRendererRowFactory { } } - let colCount = 0; for (let x = 0; x < lineLength; x++) { - // Don't allow any buffer to the right to be displayed - if (colCount >= cols) { - continue; - } - const charData = lineData.get(x); const char = charData[CHAR_DATA_CHAR_INDEX]; const attr = charData[CHAR_DATA_ATTR_INDEX]; @@ -113,7 +107,6 @@ export class DomRendererRowFactory { charElement.classList.add(`xterm-bg-${bg}`); } fragment.appendChild(charElement); - colCount += width; } return fragment; } From 478dfee6e2988fd81a61b4e85b14125b3e087e50 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 23 Nov 2018 06:51:39 -0800 Subject: [PATCH 5/5] Ensure wide chars don't overflow onto following row --- src/renderer/dom/DomRenderer.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/renderer/dom/DomRenderer.ts b/src/renderer/dom/DomRenderer.ts index fadd9b7211..73c63b4652 100644 --- a/src/renderer/dom/DomRenderer.ts +++ b/src/renderer/dom/DomRenderer.ts @@ -111,6 +111,8 @@ export class DomRenderer extends EventEmitter implements IRenderer { element.style.width = `${this.dimensions.canvasWidth}px`; element.style.height = `${this.dimensions.actualCellHeight}px`; element.style.lineHeight = `${this.dimensions.actualCellHeight}px`; + // Make sure rows don't overflow onto following row + element.style.overflow = 'hidden'; }); if (!this._dimensionsStyleElement) { @@ -330,7 +332,7 @@ export class DomRenderer extends EventEmitter implements IRenderer { const row = y + terminal.buffer.ydisp; const lineData = terminal.buffer.lines.get(row); const cursorStyle = terminal.options.cursorStyle; - rowElement.appendChild(this._rowFactory.createRow(lineData, row === cursorAbsoluteY, cursorStyle, cursorX, terminal.charMeasure.width, terminal.cols)); + rowElement.appendChild(this._rowFactory.createRow(lineData, row === cursorAbsoluteY, cursorStyle, cursorX, this.dimensions.actualCellWidth, terminal.cols)); } this._terminal.emit('refresh', {start, end});