Skip to content

fix: revert to canvas renderer for xterm #8138

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 27, 2023
+20 −5
Merged

fix: revert to canvas renderer for xterm #8138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion site/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@
"dependencies": {
"@emoji-mart/data": "1.0.5",
"@emoji-mart/react": "1.0.1",
"@fastly/performance-observer-polyfill": "2.0.0",
"@emotion/react": "11.10.8",
"@emotion/styled": "11.11.0",
"@fastly/performance-observer-polyfill": "2.0.0",
"@fontsource/ibm-plex-mono": "4.5.10",
"@fontsource/inter": "5.0.2",
"@monaco-editor/react": "4.5.0",
Expand Down Expand Up @@ -94,6 +94,7 @@
"vite": "4.3.9",
"xstate": "4.37.2",
"xterm": "5.1.0",
"xterm-addon-canvas": "0.4.0",
"xterm-addon-fit": "0.7.0",
"xterm-addon-web-links": "0.8.0",
"yup": "0.32.11"
Expand Down
2 changes: 1 addition & 1 deletion site/src/AppRouter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ export const AppRouter: FC = () => {
{/* Terminal and CLI auth pages don't have the dashboard layout */}
<Route
path="/:username/:workspace/terminal"
element={<TerminalPage />}
element={<TerminalPage renderer="canvas" />}
/>
<Route path="cli-auth" element={<CliAuthenticationPage />} />
</Route>
Expand Down
2 changes: 1 addition & 1 deletion site/src/pages/TerminalPage/TerminalPage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ Object.defineProperty(window, "TextEncoder", {
const renderTerminal = async (
route = `/${MockUser.username}/${MockWorkspace.name}/terminal`,
) => {
const utils = renderWithAuth(<TerminalPage />, {
const utils = renderWithAuth(<TerminalPage renderer="dom" />, {
Copy link
Member

Choose a reason for hiding this comment

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

Is this because Jest can't handle canvas elements? If so, how was this test working before? Asking because I'm curious if this canvas switch is the culprit behind #7948.

Copy link
Member Author

@code-asher code-asher Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yup the Jest test looks for HTML elements which will only be present in the DOM renderer. Technically setting renderer here is not necessary since the default is already DOM but I added it be explicit.

Before the major bump to xterm, back when the default was canvas and before it was broken out into an addon, we would explicitly set rendererType to dom so it would have been using the DOM renderer then too.

Unfortunately I think that means #7948 is unrelated. If it was using canvas I think we would get a different error since .xterm-rows should not exist at all (we should have seen the error no xterm-rows instead).

route,
path: "/:username/:workspace/terminal",
})
Expand Down
13 changes: 11 additions & 2 deletions site/src/pages/TerminalPage/TerminalPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { useNavigate, useParams, useSearchParams } from "react-router-dom"
import { colors } from "theme/colors"
import { v4 as uuidv4 } from "uuid"
import * as XTerm from "xterm"
import { CanvasAddon } from "xterm-addon-canvas"
import { FitAddon } from "xterm-addon-fit"
import { WebLinksAddon } from "xterm-addon-web-links"
import "xterm/css/xterm.css"
Expand Down Expand Up @@ -69,7 +70,11 @@ const useTerminalWarning = ({
}
}

const TerminalPage: FC = () => {
type TerminalPageProps = React.PropsWithChildren<{
renderer: "canvas" | "dom"
}>

const TerminalPage: FC<TerminalPageProps> = ({ renderer }) => {
const navigate = useNavigate()
const styles = useStyles()
const { proxy } = useProxy()
Expand Down Expand Up @@ -194,6 +199,10 @@ const TerminalPage: FC = () => {
background: colors.gray[16],
},
})
// DOM is the default renderer.
if (renderer === "canvas") {
terminal.loadAddon(new CanvasAddon())
}
const fitAddon = new FitAddon()
setFitAddon(fitAddon)
terminal.loadAddon(fitAddon)
Expand Down Expand Up @@ -230,7 +239,7 @@ const TerminalPage: FC = () => {
window.removeEventListener("resize", listener)
terminal.dispose()
}
}, [sendEvent, xtermRef, handleWebLink])
}, [renderer, sendEvent, xtermRef, handleWebLink])

// Triggers the initial terminal connection using
// the reconnection token and workspace name found
Expand Down
5 changes: 5 additions & 0 deletions site/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12043,6 +12043,11 @@ xtend@^4.0.0, xtend@~4.0.1:
resolved "https://registry.yarnpkg.com/xtend/-/xtend-4.0.2.tgz#bb72779f5fa465186b1f438f674fa347fdb5db54"
integrity sha512-LKYU1iAXJXUgAXn9URjiu+MWhyUXHsvfp7mcuYm9dSUKK0/CjtrUwFAxD82/mCWbtLsGjFIad0wIsod4zrTAEQ==

xterm-addon-canvas@0.4.0:
version "0.4.0"
resolved "https://registry.yarnpkg.com/xterm-addon-canvas/-/xterm-addon-canvas-0.4.0.tgz#a6ee6a56deb0c495fcef29afe6d94b7119a0f334"
integrity sha512-iTC8CdjX9+hGX7jiEuiDMXzHsY/FKJdVnbjep5xjRXNu7RKOk15xuecIkJ7HZORqMVPpr4DGS3jyd9XUoBuxqw==

xterm-addon-fit@0.7.0:
version "0.7.0"
resolved "https://registry.yarnpkg.com/xterm-addon-fit/-/xterm-addon-fit-0.7.0.tgz#b8ade6d96e63b47443862088f6670b49fb752c6a"
Expand Down