Support shadow dom in webgl renderer #5334
Conversation
CGNonofr
force-pushed
the
webgl-renderer-shadow-dom
branch
from
306427a to
08de2e9
Compare
| @@ -1280,10 +1280,10 @@ enum CellColorPosition { | |||
| * treatment. | |||
| */ | |||
| export function injectSharedRendererTestsStandalone(ctx: ISharedRendererTestContext, setupCb: () => Promise<void> | void): void { | |||
| test.describe('standalone tests', () => { | |||
| const run = (shadowDom: boolean): void => { | |||
There was a problem hiding this comment.
setupTests({ useShadowDom: false }) would read better than this when it's called below, run(false) isn't easily understandable without following the symbol.
| @@ -1332,7 +1339,7 @@ function getCellColor(ctx: ITestContext, col: number, row: number, position: Cel | |||
| let frameDetails: { cols: number, rows: number, decoded: IImage32 } | undefined = undefined; | |||
| async function getFrameDetails(ctx: ITestContext): Promise<{ cols: number, rows: number, decoded: IImage32 }> { | |||
| const screenshotOptions: LocatorScreenshotOptions | undefined = process.env.DEBUG ? { path: 'out-esbuild-test/playwright/screenshot.png' } : undefined; | |||
| const buffer = await ctx.page.locator('#terminal-container .xterm-screen').screenshot(screenshotOptions); | |||
| const buffer = await ctx.page.locator('#terminal-container').locator('.xterm-screen').screenshot(screenshotOptions); | |||
There was a problem hiding this comment.
Can you switch this change back unless it does something useful?
test/playwright/TestUtils.ts
Outdated
| await ctx.page.evaluate(` | ||
| window.unicode = new UnicodeGraphemesAddon(); | ||
| window.term.loadAddon(window.unicode); | ||
| window.term.unicode.activeVersion = '15-graphemes'; | ||
| `); | ||
| } | ||
| await ctx.page.waitForSelector('.xterm-rows'); | ||
| await ctx.page.locator('.xterm-rows').waitFor(); |
There was a problem hiding this comment.
Can you switch this change back unless it does something useful?
There was a problem hiding this comment.
Amended as well
note that the waitForSelector method comment suggests to use that syntax instead
| @@ -412,7 +412,7 @@ class TerminalCoreProxy { | |||
| } | |||
| } | |||
|
|
|||
| export async function openTerminal(ctx: ITestContext, options: ITerminalOptions | ITerminalInitOnlyOptions = {}, testOptions: { loadUnicodeGraphemesAddon: boolean } = { loadUnicodeGraphemesAddon: true }): Promise<void> { | |||
| export async function openTerminal(ctx: ITestContext, options: ITerminalOptions | ITerminalInitOnlyOptions = {}, { useShadowDom = false, loadUnicodeGraphemesAddon = true }: { loadUnicodeGraphemesAddon?: boolean, useShadowDom?: boolean } = { }): Promise<void> { | |||
There was a problem hiding this comment.
Can you keep testOptions as it was?
There was a problem hiding this comment.
Then I'm not sure how to be able to:
- keep the
loadUnicodeGraphemesAddondefault value - have a default value on
useShadowDom - make it work when only providing the
useShadowDomparameter
Without adding some boilerplate in the function itself 🤔 what is the issue with that syntax?
| // Remove shadow root if it exists | ||
| const newElement = element.cloneNode(false); | ||
| element.replaceWith(newElement); | ||
| element = newElement |
There was a problem hiding this comment.
Shouldn't this stuff only happen if !!useShadowDom?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
(if an element has a shadow dom attached, it doesn't seem we can still continue injecting other elements in it)
test/playwright/TestUtils.ts
Outdated
| ${useShadowDom ? ` | ||
| const shadowRoot = element.attachShadow({ mode: "open" }); | ||
|
|
||
| // Copy parent styles to shadow DOM | ||
| const styles = Array.from(document.querySelectorAll('link[rel="stylesheet"]')); | ||
| styles.forEach((styleEl) => { | ||
| const clone = document.createElement('link'); | ||
| clone.rel = 'stylesheet'; | ||
| clone.href = styleEl.href; | ||
| shadowRoot.appendChild(clone); | ||
| }); | ||
|
|
||
| // Create new element inside the shadow DOM | ||
| element = document.createElement('div'); | ||
| element.style.width = '100%'; | ||
| element.style.height = '100%'; | ||
| shadowRoot.appendChild(element); | ||
| `: ` | ||
| `} |
There was a problem hiding this comment.
This reads hard, part of it is the indentation. Assembling the string before the evaluate call is a more readable way to handle this:
let script = '...'
if (useShadowDom) {
script += '...';
}
script += '...';
There was a problem hiding this comment.
I won't agree on this one but It's not a big deal to me, changed and amended as well
CGNonofr
force-pushed
the
webgl-renderer-shadow-dom
branch
from
08de2e9 to
3ad83dd
Compare
|
ping @Tyriar ? |
|
Is there anything I can do to make it move forward? |
Similar to what has been done in #3239, but for the webgl renderer
fix #5338