Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a fallback DOM renderer #1432

Merged
merged 28 commits into from
Jun 3, 2018
Merged

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented May 4, 2018

This change is motivated in order to provide a fallback for environments that do not support or who have performance issues using GPU canvas acceleration.

Fixes #1360

Notes

  • A lot of the implementation is similar to the old renderer which can be viewed in Render using canvas #938
  • I explored making this an addon but it depends on too many internal objects like ColorManager, RenderDebouncer and EventEmitter. Once TS 3 is released we should refactor out a "platform"/"base" project which can be shared easily depended on by the core and addons (and speed up builds since it only need to be rebuilt if files within it changes), see Use project references coming in TypeScript v3 #1337. In the meantime I think keeping it in core as an option is the way to go

@Tyriar Tyriar added the work-in-progress Do not merge label May 4, 2018
@Tyriar Tyriar self-assigned this May 4, 2018
@Tyriar Tyriar added this to the 3.5.0 milestone May 4, 2018
@Tyriar Tyriar removed the work-in-progress Do not merge label May 4, 2018
`.xterm .${BG_CLASS_PREFIX}${i} { background-color: ${c.css}; }`;
});

this._themeStyleElement.innerHTML = styles;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it is wise to create these styles for every single terminal instance. It will put quite some load on the CSS rendering engine with every new instance of the terminal spawned (because the classes created are not scoped to the DOM tree the style element is attached to). Shadow DOM might solve this problem...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is called sparingly though, only when the theme changes. I didn't want to spend too much time optimizing since this is just the fallback experience.

The way I was thinking we could fix this is by adding an ID (or a unique class) to the terminal element.

src/Terminal.ts Outdated
@@ -703,7 +705,11 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II
// Performance: Add viewport and helper elements from the fragment
this.element.appendChild(fragment);

this.renderer = new Renderer(this, this.options.theme);
if (this.options.rendererType === 'canvas') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only known renderer type seems to be 'canvas', any other string will use the dom renderer. Shouldn't we expect that 'dom' is passed if the DOM renderer should be used, and throw an exception if an unsupported renderer name gets passed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 will fix this up

@mofux
Copy link
Contributor

mofux commented May 8, 2018

In general I'm not a big fan of the current renderer API because it is too closely coupled. I'd much rather have a layer in the middle that holds abstract cell decoration instructions (foreground, background, bold, italic, underline, link, selected, etc...). Things like the linkifier or the selection manager would provide cell decorations instead of drawing directly - and then it would be up to the renderer to support and draw these decoration types. It will probably involve a lot of refactoring, but would open up xterm.js to more advanced plugins that could influence rendering more easily. Maybe we should iterate on something like that in a separate issue, potentially for a v4?

@Tyriar
Copy link
Member Author

Tyriar commented May 8, 2018

In general I'm not a big fan of the current renderer API because it is too closely coupled. I'd much rather have a layer in the middle that holds abstract cell decoration instructions (foreground, background, bold, italic, underline, link, selected, etc...). Things like the linkifier or the selection manager would provide cell decorations instead of drawing directly - and then it would be up to the renderer to support and draw these decoration types. It will probably involve a lot of refactoring, but would open up xterm.js to more advanced plugins that could influence rendering more easily. Maybe we should iterate on something like that in a separate issue, potentially for a v4?

More targeted PRs against those components would be a good in the future. We can also tweak IRenderer in the future, just take the DOM renderer along with the ride. I think the refactors you're proposing would also better enable us to pull the dom renderer into an addon which is where it should go in the end.

@Tyriar
Copy link
Member Author

Tyriar commented May 15, 2018

@mofux feedback addressed, ready for another round

switch (this.options.rendererType) {
case 'canvas': this.renderer = new Renderer(this, this.options.theme); break;
case 'dom': this.renderer = new DomRenderer(this, this.options.theme); break;
default: throw new Error(`Unrecognized rendererType "${this.options.rendererType}"`);
Copy link

@ghost ghost May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you can add an open option to provide custom renderers?

if (this.options.rendererType) {
  this.renderer = new this.options.rendererType(this, this.options.theme)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mofux and I will be improving how renderers attach soon, the DOM renderer will eventually be moved to an addon. Also I'm not sure about swapping out the renderer in its entirety in the API but I definitely want people to contribute layers that could sit on top.

@vincentwoo
Copy link
Contributor

I'd like to submit a request for at least some (possibly undocumented way) to pull out a DOM render without necessarily attaching xterm to an open DOM element, etc.

@Tyriar
Copy link
Member Author

Tyriar commented May 18, 2018

@vincentwoo you could pull from Terminal._buffer? I think we talked about this in some issue?

@mofux
Copy link
Contributor

mofux commented Jun 3, 2018

Just re-tested an I don't see any issues left. Good to merge from my side 👍

@Tyriar
Copy link
Member Author

Tyriar commented Jun 3, 2018

I'll do another check of it as well now

@Tyriar Tyriar merged commit 1cccec6 into xtermjs:master Jun 3, 2018
@Tyriar Tyriar deleted the 1360_dom_renderer branch June 3, 2018 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants