From 5af1554bbfda78300acea7b731e3b22e0ae654f4 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 3 Jun 2020 16:05:53 -0400 Subject: [PATCH 1/9] Only removeChild valid children in DomRenderer dispose --- src/browser/renderer/dom/DomRenderer.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/browser/renderer/dom/DomRenderer.ts b/src/browser/renderer/dom/DomRenderer.ts index a01c2208..d9faf79c 100644 --- a/src/browser/renderer/dom/DomRenderer.ts +++ b/src/browser/renderer/dom/DomRenderer.ts @@ -95,10 +95,20 @@ export class DomRenderer extends Disposable implements IRenderer { public dispose(): void { this._element.classList.remove(TERMINAL_CLASS_PREFIX + this._terminalClass); - this._screenElement.removeChild(this._rowContainer); - this._screenElement.removeChild(this._selectionContainer); - this._screenElement.removeChild(this._themeStyleElement); - this._screenElement.removeChild(this._dimensionsStyleElement); + + // Outside influences such as React unmounts may manipulate the DOM before our disposal. + // https://github.com/xtermjs/xterm.js/issues/2960 + for (const element of [ + this._rowContainer, + this._selectionContainer, + this._themeStyleElement, + this._dimensionsStyleElement, + ]) { + if (element.parentElement === this._screenElement) { + this._screenElement.removeChild(element); + } + } + super.dispose(); } From 8ee08a3be7da3ab724b730646123ad30d4940d3b Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Wed, 3 Jun 2020 16:16:00 -0400 Subject: [PATCH 2/9] ESLint? What is that? Never heard of the thing. --- src/browser/renderer/dom/DomRenderer.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/browser/renderer/dom/DomRenderer.ts b/src/browser/renderer/dom/DomRenderer.ts index d9faf79c..5e6231f3 100644 --- a/src/browser/renderer/dom/DomRenderer.ts +++ b/src/browser/renderer/dom/DomRenderer.ts @@ -102,13 +102,13 @@ export class DomRenderer extends Disposable implements IRenderer { this._rowContainer, this._selectionContainer, this._themeStyleElement, - this._dimensionsStyleElement, + this._dimensionsStyleElement ]) { if (element.parentElement === this._screenElement) { this._screenElement.removeChild(element); } } - + super.dispose(); } From c5d30a9c63260993bb5cd5afe74dba59b6a4c93c Mon Sep 17 00:00:00 2001 From: "dependabot-preview[bot]" <27856297+dependabot-preview[bot]@users.noreply.github.com> Date: Thu, 4 Jun 2020 08:17:01 +0000 Subject: [PATCH 3/9] Bump @types/ws from 7.2.4 to 7.2.5 Bumps [@types/ws](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/ws) from 7.2.4 to 7.2.5. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/ws) Signed-off-by: dependabot-preview[bot] --- package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 51c8c856..ce503595 100644 --- a/package.json +++ b/package.json @@ -45,7 +45,7 @@ "@types/node": "^10.17.17", "@types/utf8": "^2.1.6", "@types/webpack": "^4.41.17", - "@types/ws": "^7.2.4", + "@types/ws": "^7.2.5", "@typescript-eslint/eslint-plugin": "^2.34.0", "@typescript-eslint/parser": "^2.34.0", "chai": "^4.2.0", diff --git a/yarn.lock b/yarn.lock index 925680bf..924999ca 100644 --- a/yarn.lock +++ b/yarn.lock @@ -364,10 +364,10 @@ "@types/webpack-sources" "*" source-map "^0.6.0" -"@types/ws@^7.2.4": - version "7.2.4" - resolved "https://registry.yarnpkg.com/@types/ws/-/ws-7.2.4.tgz#b3859f7b9c243b220efac9716ec42c716a72969d" - integrity sha512-9S6Ask71vujkVyeEXKxjBSUV8ZUB0mjL5la4IncBoheu04bDaYyUKErh1BQcY9+WzOUOiKqz/OnpJHYckbMfNg== +"@types/ws@^7.2.5": + version "7.2.5" + resolved "https://registry.yarnpkg.com/@types/ws/-/ws-7.2.5.tgz#513f28b04a1ea1aa9dc2cad3f26e8e37c88aae49" + integrity sha512-4UEih9BI1nBKii385G9id1oFrSkLcClbwtDfcYj8HJLQqZVAtb/42vXVrYvRWCcufNF/a+rZD3MxNwghA7UmCg== dependencies: "@types/node" "*" From 9b91e3d5e2ec3028aa81e129dfdf2b317da07994 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Thu, 4 Jun 2020 10:07:54 -0400 Subject: [PATCH 4/9] Bumped Dockerfile Node from 8 to 12 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This seemed to work for me, but I'm new here and don't particularly trust myself. 😄 --- Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index 1c72e679..c346e554 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -FROM node:8 +FROM node:12 MAINTAINER Paris Kasidiaris # Set the working directory From fbb8c6273b38f879f1186e2be3121aab1402cd92 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Thu, 4 Jun 2020 10:12:36 -0400 Subject: [PATCH 5/9] Switched to parentElement optionals --- src/browser/renderer/dom/DomRenderer.ts | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/browser/renderer/dom/DomRenderer.ts b/src/browser/renderer/dom/DomRenderer.ts index 5e6231f3..9082e36b 100644 --- a/src/browser/renderer/dom/DomRenderer.ts +++ b/src/browser/renderer/dom/DomRenderer.ts @@ -98,16 +98,10 @@ export class DomRenderer extends Disposable implements IRenderer { // Outside influences such as React unmounts may manipulate the DOM before our disposal. // https://github.com/xtermjs/xterm.js/issues/2960 - for (const element of [ - this._rowContainer, - this._selectionContainer, - this._themeStyleElement, - this._dimensionsStyleElement - ]) { - if (element.parentElement === this._screenElement) { - this._screenElement.removeChild(element); - } - } + this._rowContainer.parentElement?.removeChild(this._rowContainer); + this._selectionContainer.parentElement?.removeChild(this._selectionContainer); + this._themeStyleElement.parentElement?.removeChild(this._themeStyleElement); + this._dimensionsStyleElement.parentElement?.removeChild(this._dimensionsStyleElement); super.dispose(); } From 15f17595807ced364f0933fe56b8971c484a17a4 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Thu, 4 Jun 2020 07:20:59 -0700 Subject: [PATCH 6/9] Introduce removeElementFromParent helper --- src/browser/AccessibilityManager.ts | 7 +++---- src/browser/Dom.ts | 10 ++++++++++ src/browser/renderer/BaseRenderLayer.ts | 3 ++- src/browser/renderer/dom/DomRenderer.ts | 6 ++---- 4 files changed, 17 insertions(+), 9 deletions(-) create mode 100644 src/browser/Dom.ts diff --git a/src/browser/AccessibilityManager.ts b/src/browser/AccessibilityManager.ts index 2a34d6a3..af9fee6f 100644 --- a/src/browser/AccessibilityManager.ts +++ b/src/browser/AccessibilityManager.ts @@ -12,6 +12,7 @@ import { addDisposableDomListener } from 'browser/Lifecycle'; import { Disposable } from 'common/Lifecycle'; import { ScreenDprMonitor } from 'browser/ScreenDprMonitor'; import { IRenderService } from 'browser/services/Services'; +import { removeElementFromParent } from 'browser/Dom'; const MAX_ROWS_TO_READ = 20; @@ -106,7 +107,7 @@ export class AccessibilityManager extends Disposable { public dispose(): void { super.dispose(); - this._terminal.element?.removeChild(this._accessibilityTreeRoot); + removeElementFromParent(this._accessibilityTreeRoot); this._rowElements.length = 0; } @@ -240,9 +241,7 @@ export class AccessibilityManager extends Disposable { // Only detach/attach on mac as otherwise messages can go unaccounced if (isMac) { - if (this._liveRegion.parentNode) { - this._accessibilityTreeRoot.removeChild(this._liveRegion); - } + removeElementFromParent(this._liveRegion); } } diff --git a/src/browser/Dom.ts b/src/browser/Dom.ts new file mode 100644 index 00000000..c558a8b1 --- /dev/null +++ b/src/browser/Dom.ts @@ -0,0 +1,10 @@ +/** + * Copyright (c) 2020 The xterm.js authors. All rights reserved. + * @license MIT + */ + +export function removeElementFromParent(...elements: (HTMLElement | undefined)[]): void { + for (const e of elements) { + e?.parentElement?.removeChild(e); + } +} diff --git a/src/browser/renderer/BaseRenderLayer.ts b/src/browser/renderer/BaseRenderLayer.ts index 7d9fb5b8..8afec352 100644 --- a/src/browser/renderer/BaseRenderLayer.ts +++ b/src/browser/renderer/BaseRenderLayer.ts @@ -16,6 +16,7 @@ import { CellData } from 'common/buffer/CellData'; import { IBufferService, IOptionsService } from 'common/services/Services'; import { throwIfFalsy } from 'browser/renderer/RendererUtils'; import { channels, color, rgba } from 'browser/Color'; +import { removeElementFromParent } from 'browser/Dom'; export abstract class BaseRenderLayer implements IRenderLayer { private _canvas: HTMLCanvasElement; @@ -60,7 +61,7 @@ export abstract class BaseRenderLayer implements IRenderLayer { } public dispose(): void { - this._canvas.parentElement?.removeChild(this._canvas); + removeElementFromParent(this._canvas); this._charAtlas?.dispose(); } diff --git a/src/browser/renderer/dom/DomRenderer.ts b/src/browser/renderer/dom/DomRenderer.ts index 9082e36b..572b7114 100644 --- a/src/browser/renderer/dom/DomRenderer.ts +++ b/src/browser/renderer/dom/DomRenderer.ts @@ -12,6 +12,7 @@ import { ICharSizeService } from 'browser/services/Services'; import { IOptionsService, IBufferService } from 'common/services/Services'; import { EventEmitter, IEvent } from 'common/EventEmitter'; import { color } from 'browser/Color'; +import { removeElementFromParent } from 'browser/Dom'; const TERMINAL_CLASS_PREFIX = 'xterm-dom-renderer-owner-'; const ROW_CONTAINER_CLASS = 'xterm-rows'; @@ -98,10 +99,7 @@ export class DomRenderer extends Disposable implements IRenderer { // Outside influences such as React unmounts may manipulate the DOM before our disposal. // https://github.com/xtermjs/xterm.js/issues/2960 - this._rowContainer.parentElement?.removeChild(this._rowContainer); - this._selectionContainer.parentElement?.removeChild(this._selectionContainer); - this._themeStyleElement.parentElement?.removeChild(this._themeStyleElement); - this._dimensionsStyleElement.parentElement?.removeChild(this._dimensionsStyleElement); + removeElementFromParent(this._rowContainer, this._selectionContainer, this._themeStyleElement, this._dimensionsStyleElement); super.dispose(); } From a1dc0d1936915993575062280036a50f5f60c2c1 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Thu, 4 Jun 2020 07:32:34 -0700 Subject: [PATCH 7/9] Add tests for removeElementFromParent --- src/browser/Dom.test.ts | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 src/browser/Dom.test.ts diff --git a/src/browser/Dom.test.ts b/src/browser/Dom.test.ts new file mode 100644 index 00000000..14f3d26f --- /dev/null +++ b/src/browser/Dom.test.ts @@ -0,0 +1,41 @@ +/** + * Copyright (c) 2020 The xterm.js authors. All rights reserved. + * @license MIT + */ + +import jsdom = require('jsdom'); +import { removeElementFromParent } from 'browser/Dom'; +import { strictEqual, doesNotThrow } from 'assert'; + +describe('Dom', () => { + const dom = new jsdom.JSDOM(); + const document = dom.window.document; + + describe('removeElementFromParent', () => { + it('should remove single child', () => { + const e = document.createElement('div'); + document.body.appendChild(e); + strictEqual(e.parentElement, document.body); + removeElementFromParent(e); + strictEqual(e.parentElement, null); + }); + it('should remove multiple elements', () => { + const e1 = document.createElement('div'); + const e2 = document.createElement('div'); + document.body.appendChild(e1); + document.body.appendChild(e2); + strictEqual(e1.parentElement, document.body); + strictEqual(e2.parentElement, document.body); + removeElementFromParent(e1, e2); + strictEqual(e1.parentElement, null); + strictEqual(e2.parentElement, null); + }); + it('should not throw on undefined', () => { + const e = document.createElement('div'); + document.body.appendChild(e); + strictEqual(e.parentElement, document.body); + doesNotThrow(() => removeElementFromParent(undefined, e)); + strictEqual(e.parentElement, null); + }); + }); +}); From bc6bb417ac4e84d1ddb6821365428653a475d686 Mon Sep 17 00:00:00 2001 From: "dependabot-preview[bot]" <27856297+dependabot-preview[bot]@users.noreply.github.com> Date: Fri, 5 Jun 2020 07:50:19 +0000 Subject: [PATCH 8/9] Bump @types/glob from 7.1.1 to 7.1.2 Bumps [@types/glob](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/glob) from 7.1.1 to 7.1.2. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/glob) Signed-off-by: dependabot-preview[bot] --- package.json | 2 +- yarn.lock | 14 ++++---------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/package.json b/package.json index ce503595..e90a4e6f 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,7 @@ "@types/chai": "^4.2.11", "@types/debug": "^4.1.5", "@types/deep-equal": "^1.0.1", - "@types/glob": "^7.1.1", + "@types/glob": "^7.1.2", "@types/jsdom": "^16.2.3", "@types/mocha": "^7.0.2", "@types/node": "^10.17.17", diff --git a/yarn.lock b/yarn.lock index 924999ca..af14883e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -232,11 +232,6 @@ resolved "https://registry.yarnpkg.com/@types/eslint-visitor-keys/-/eslint-visitor-keys-1.0.0.tgz#1ee30d79544ca84d68d4b3cdb0af4f205663dd2d" integrity sha512-OCutwjDZ4aFS6PB1UZ988C4YgwlBHJd6wCeQqaLdmadZ/7e+w79+hbMUFC1QXDNCmdyoRfAFdm0RypzwR+Qpag== -"@types/events@*": - version "3.0.0" - resolved "https://registry.yarnpkg.com/@types/events/-/events-3.0.0.tgz#2862f3f58a9a7f7c3e78d79f130dd4d71c25c2a7" - integrity sha512-EaObqwIvayI5a8dCzhFrjKzVwKLxjoG9T6Ppd5CEo07LRKfQ8Yokw54r5+Wq7FaBQ+yXRvQAYPrHwya1/UFt9g== - "@types/fs-extra@^7.0.0": version "7.0.0" resolved "https://registry.yarnpkg.com/@types/fs-extra/-/fs-extra-7.0.0.tgz#9c4ad9e1339e7448a76698829def1f159c1b636c" @@ -244,12 +239,11 @@ dependencies: "@types/node" "*" -"@types/glob@^7.1.1": - version "7.1.1" - resolved "https://registry.yarnpkg.com/@types/glob/-/glob-7.1.1.tgz#aa59a1c6e3fbc421e07ccd31a944c30eba521575" - integrity sha512-1Bh06cbWJUHMC97acuD6UMG29nMt0Aqz1vF3guLfG+kHHJhy3AyohZFFxYk2f7Q1SQIrNwvncxAE0N/9s70F2w== +"@types/glob@^7.1.2": + version "7.1.2" + resolved "https://registry.yarnpkg.com/@types/glob/-/glob-7.1.2.tgz#06ca26521353a545d94a0adc74f38a59d232c987" + integrity sha512-VgNIkxK+j7Nz5P7jvUZlRvhuPSmsEfS03b0alKcq5V/STUKAa3Plemsn5mrQUO7am6OErJ4rhGEGJbACclrtRA== dependencies: - "@types/events" "*" "@types/minimatch" "*" "@types/node" "*" From c669cfbb2373520e157a890cabbe395b389b34ea Mon Sep 17 00:00:00 2001 From: "dependabot-preview[bot]" <27856297+dependabot-preview[bot]@users.noreply.github.com> Date: Fri, 5 Jun 2020 13:05:50 +0000 Subject: [PATCH 9/9] Bump typescript from 3.9.3 to 3.9.5 Bumps [typescript](https://github.com/Microsoft/TypeScript) from 3.9.3 to 3.9.5. - [Release notes](https://github.com/Microsoft/TypeScript/releases) - [Commits](https://github.com/Microsoft/TypeScript/compare/v3.9.3...v3.9.5) Signed-off-by: dependabot-preview[bot] --- yarn.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/yarn.lock b/yarn.lock index af14883e..c07fff9e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5600,9 +5600,9 @@ typedarray@^0.0.6: integrity sha1-hnrHTjhkGHsdPUfZlqeOxciDB3c= typescript@3.9, typescript@^3.5.1: - version "3.9.3" - resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.9.3.tgz#d3ac8883a97c26139e42df5e93eeece33d610b8a" - integrity sha512-D/wqnB2xzNFIcoBG9FG8cXRDjiqSTbG2wd8DMZeQyJlP1vfTkIxH4GKveWaEBYySKIg+USu+E+EDIR47SqnaMQ== + version "3.9.5" + resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.9.5.tgz#586f0dba300cde8be52dd1ac4f7e1009c1b13f36" + integrity sha512-hSAifV3k+i6lEoCJ2k6R2Z/rp/H3+8sdmcn5NrS3/3kE7+RyZXm9aqvxWqjEXHAd8b0pShatpcdMTvEdvAJltQ== union-value@^1.0.0: version "1.0.1"