Skip to content

Support shadow dom in webgl renderer#5334

Merged
Tyriar merged 3 commits intoxtermjs:masterfrom
CodinGame:webgl-renderer-shadow-dom
Jul 31, 2025
Merged

Support shadow dom in webgl renderer#5334
Tyriar merged 3 commits intoxtermjs:masterfrom
CodinGame:webgl-renderer-shadow-dom

Conversation

@CGNonofr
Copy link
Copy Markdown

@CGNonofr CGNonofr commented May 5, 2025

Similar to what has been done in #3239, but for the webgl renderer

fix #5338

@CGNonofr CGNonofr force-pushed the webgl-renderer-shadow-dom branch from 306427a to 08de2e9 Compare May 20, 2025 14:31
Tyriar
Tyriar previously requested changes May 21, 2025
`);
}
await ctx.page.waitForSelector('.xterm-rows');
await ctx.page.locator('.xterm-rows').waitFor();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you switch this change back unless it does something useful?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Amended as well

note that the waitForSelector method comment suggests to use that syntax instead

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good to know, don't want to be inconsistent though and instead migrate all at once

Comment on lines +430 to +434
// Remove shadow root if it exists
const newElement = element.cloneNode(false);
element.replaceWith(newElement);
element = newElement
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this stuff only happen if !!useShadowDom?

Copy link
Copy Markdown
Author

@CGNonofr CGNonofr May 21, 2025

Choose a reason for hiding this comment

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

No, because if the previous test was using shadow dom, the next test need to clean it no matter what (just like the call to dispose on any existing term instance a few line above)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

(if an element has a shadow dom attached, it doesn't seem we can still continue injecting other elements in it)

@CGNonofr CGNonofr force-pushed the webgl-renderer-shadow-dom branch from 08de2e9 to 3ad83dd Compare May 21, 2025 19:51
@CGNonofr
Copy link
Copy Markdown
Author

ping @Tyriar ?

@CGNonofr
Copy link
Copy Markdown
Author

CGNonofr commented Jun 9, 2025

Is there anything I can do to make it move forward?

@CGNonofr
Copy link
Copy Markdown
Author

CGNonofr commented Jul 8, 2025

Is there still hope? It's a bit frustrating to invest time in a fix to see it sleep for months

@CGNonofr
Copy link
Copy Markdown
Author

Ping @Tyriar

@Tyriar Tyriar closed this Jul 31, 2025
@Tyriar Tyriar reopened this Jul 31, 2025
@Tyriar
Copy link
Copy Markdown
Member

Tyriar commented Jul 31, 2025

@CGNonofr I feel you, this is what you're competing with though as I'm the person that typically merges xterm.js PRs 😅

image

@CGNonofr
Copy link
Copy Markdown
Author

I would be happy to help you bring this number down to 683 (:

Copy link
Copy Markdown
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Just the one outstanding comment in #5334 (comment), then good to merge 👌

@Tyriar Tyriar added this to the 6.0.0 milestone Jul 31, 2025
@Tyriar Tyriar self-assigned this Jul 31, 2025
Copy link
Copy Markdown
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks a bunch, sorry again about the delay

@Tyriar Tyriar enabled auto-merge July 31, 2025 15:24
@CGNonofr
Copy link
Copy Markdown
Author

no worries! thanks and good luck for the other 683 tasks!

@Tyriar Tyriar merged commit 4c0cf27 into xtermjs:master Jul 31, 2025
12 checks passed
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.

WebGL renderer doesn't work in a shadow dom

2 participants