Xterm.js: Consider separating Terminal into different layers to prevent DOM access exceptions

[Tyriar]

These happen periodically https://github.com/xtermjs/xterm.js/issues/1379 and are difficult to catch. A better way to handle this would be to completely isolate the headless terminal and DOM-access terminal.

[Tyriar]

Some rough options below. Delegation seems like the cleanest to me, probably needs a little more boilerplate though.

Delegation

• Terminal owns HeadlessTerminal or AttachedTerminal
• AttachedTerminal owns HeadlessTerminal
• HeadlessTerminal does not know about AttachedTerminal

interface ITerminal {
  open(element: HTMLElement): void;
  reset(): void;
}

class Terminal implements ITerminal {
  private _delegate: ITerminal;

  open(element: HTMLElement): void {
    // Special case for this method
    if (this._delegate instanceof HeadlessTerminal) {
      this._delegate = new AttachedTerminal(this._delegate, element);
    }
  }
  reset(): void {
    this._delegate.reset();
  }
}

class HeadlessTerminal implements ITerminal {
  // buffer, options, etc. anything that does not need the DOM

  open(element: HTMLElement): void { /* noop */ }
  reset(): void { console.log('reset'); }
}

class AttachedTerminal implements ITerminal {
  // viewport, textarea, etc. anything that needs the DOM

  // Use HeadlessTerminal here so that we don't need to expose a bunch of stuff on ITerminal
  constructor(private _headlessTerminal: HeadlessTerminal, element: HTMLElement) {
  }
  open(element: HTMLElement): void {
    this._headlessTerminal.open();
    // Attach to DOM
  }
  reset(): void {
    this._headlessTerminal.open();
    // DOM access
  }
}

Decorator (sort of)

• Terminal owns AttachedTerminal
• AttachedTerminal depends on Terminal (circular dep)
• Needs an if statements to check for attachedTerminal on every ITerminal method

interface ITerminal {
  open(element: HTMLElement): void;
  reset(): void;
}

class Terminal implements ITerminal {
  // buffer, options, etc. anything that does not need the DOM
  private attachedTerminal: AttachedTerminal;

  open(element: HTMLElement): void {
    if (!attachedTerminal) {
      attachedTerminal = new AttachedTerminal(this, element);
    }
  }
  reset(): void {
    console.log('reset');
    // We would need to put this on every single method that needs DOM access
    if (attachedTerminal) {
      attachedTerminal.reset();
    }
  }
}

class AttachedTerminal implements ITerminal {
    constructor(private _headlessTerminal: Terminal, element: HTMLElement) {
    this.open(element);
  }
  open(element: HTMLElement): void {
    // Attach to DOM
  }
  reset(): void {
    // DOM access
  }
}

Events

• Terminal owns AttachedTerminal
• No circular dep
• Lots of events, consumers will probably be naughty and hook into them

interface ITerminal {
  open(element: HTMLElement): void;
  reset(): void;
}

class Terminal implements ITerminal {
  // buffer, options, etc. anything that does not need the DOM

  private attachedTerminal: AttachedTerminal;

  open(element: HTMLElement): void {
    if (!attachedTerminal) {
      attachedTerminal = new AttachedTerminal(this, element);
    }
  }
  reset(): void {
    console.log('reset');
    this.emit('postreset');
  }
}

class AttachedTerminal {
    constructor(private _terminal: ITerminal, element: HTMLElement) {
    this.on('postreset', reset);
  }

  reset(): void {
    // DOM access
  }
}

[mofux]

How about separating it into ITerminalFrontend and ITerminalModel. The ITerminalModel would hold and manage the state, while ITerminalFrontend does the rendering. This would also allow us later on to move part of the model logic to a WebWorker?

[Tyriar]

@mofux isn't extracting all the model information a separate (still useful) refactor aiming at solving a different problem? I don't think it would solve the problem of ensuring bad DOM access doesn't happen.

Also just now I needed to disable a new test because open in a limited test DOM environment is horrible, was wishing the selection code was separate too https://github.com/Microsoft/vscode/commit/43edf975d28ad4c2bc75697fd52c437f851d1818

[mofux]

If I understand the problem correctly, the terminal currently assumes that it is attached to the DOM once the terminal.open(element) is called. During the open phase we initialise most of our internal services and managers, lots of them need direct DOM access at that time (like the Renderer, SelectionManager and Linkifier that create their own canvases).

This is bad because these services have a direct dependency to the DOM. It would be much better if things like selection, links etc would be tracked on the model and then would be drawn during the render phase using the information stored with the model. That would also mean that things would continue to work if no renderer is attached (because only the renderer would then have a dependency to the DOM, but not the model).

The monaco editor uses a very similar approach, where all the state is saved with the TextModel, and the editor itself is only a rendering component that draws the model. Things like linkifiers or syntax highlighters add "decorations" to the model. The renderer later on takes those decorations and transform them into spans with classes that have the styling instructions for those decoration types.

In our case, decorations could look like this (every buffer has its own decorations):

{
  // range: [x1, y1, x2, y2]
  decorations: [

      // foreground color decoration
      { type: 'fg', range: [0,0,0,10], color: 'red' },

      // selection decoration
      { type: 'selection', range: [0,2,12,4] },

      // background color decoration
      { type: 'bg', range: [0,0,0,84], color: 'blue' },

      // link decoration
      { type: 'link', range: [10,0,5,1] }

  ]
}

Based on these decorations it is very easy to write a generic renderer that doesn't need to know about the linkifier or the selection manager at all, it only renders the decorations.

We'll want to maintain these decorations for the whole scrollback + screen area, so we only have to calculate them once (and every time that particular buffer line is altered).

Hope you can see where I'm going with this 😅

[Tyriar]

Duplicate https://github.com/xtermjs/xterm.js/issues/1507