diff --git a/scripts/e2e-settings-visual-theme.test.mjs b/scripts/e2e-settings-visual-theme.test.mjs index 2a9d983..34c49c7 100644 --- a/scripts/e2e-settings-visual-theme.test.mjs +++ b/scripts/e2e-settings-visual-theme.test.mjs @@ -1,4 +1,5 @@ import assert from 'node:assert/strict'; +import { readFile } from 'node:fs/promises'; import test from 'node:test'; import { @@ -39,6 +40,20 @@ test('theme registry exposes the requested Monaco themes and safe fallback', () ]); }); +test('terminal viewport inherits the active visual theme background', async () => { + const css = await readFile(new URL('../src/ui/styles.css', import.meta.url), 'utf8'); + const app = await readFile(new URL('../src/ui/app.js', import.meta.url), 'utf8'); + assert.match(css, /\.xterm\s+\.xterm-viewport\s*\{[^}]*background-color:\s*var\(--terminal-background\);/s); + assert.ok( + app.indexOf("import '@xterm/xterm/css/xterm.css';") < app.indexOf("import './styles.css';"), + 'load application styles after xterm so terminal overrides win' + ); + assert.deepEqual( + [...css.matchAll(/--terminal-background:\s*(#[\da-f]{3,6});/gi)].map(([, color]) => color.toLowerCase()), + ['#11111b', '#fff', '#181818', '#fff', '#000'] + ); +}); + function contrastRatio(foreground, background) { const luminance = (color) => { const channels = color.match(/[0-9a-f]{2}/gi).map((channel) => parseInt(channel, 16) / 255); diff --git a/scripts/smoke-browser.mjs b/scripts/smoke-browser.mjs index a8d65c6..acc63e5 100644 --- a/scripts/smoke-browser.mjs +++ b/scripts/smoke-browser.mjs @@ -4,6 +4,7 @@ import os from 'node:os'; import path from 'node:path'; import { spawn } from 'node:child_process'; import { fileURLToPath } from 'node:url'; +import { PNG } from 'pngjs'; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); const distDir = path.join(repoRoot, 'dist'); @@ -281,7 +282,10 @@ async function evaluate(cdp, sessionId, expression, { awaitPromise = false } = { }, sessionId); if (result.exceptionDetails) { - throw new Error(`Browser evaluation failed: ${result.exceptionDetails.text}`); + throw new Error( + `Browser evaluation failed: ${result.exceptionDetails.text}; ` + + `${result.exceptionDetails.exception?.description || ''}` + ); } return result.result?.value; @@ -314,44 +318,141 @@ async function replaceEditorText(cdp, sessionId, source) { await cdp.send('Input.insertText', { text: source }, sessionId); } -async function selectTerminalText(cdp, sessionId) { - const bounds = await evaluate(cdp, sessionId, `(() => { - const screen = document.querySelector('#terminal-container .xterm-screen'); +async function selectTerminalText(cdp, sessionId, targetText = 'Compilation successful.') { + const targetLiteral = JSON.stringify(targetText); + const scrolled = await evaluate(cdp, sessionId, + `document.getElementById('terminal-container')?.__browserCppScrollToText?.(${targetLiteral}) || false`); + assert(scrolled, `Could not scroll terminal to ${JSON.stringify(targetText)}`); + const target = await waitFor(async () => evaluate(cdp, sessionId, `(() => { const state = document.getElementById('terminal-container')?.__browserCppReadTerminalSelection?.(); - if (!screen || !state) return null; - const rect = screen.getBoundingClientRect(); - return { - x: rect.x, - y: rect.y, - width: rect.width, - height: rect.height, - cols: state.cols, - rows: state.rows, - firstTextRow: state.firstTextRow, - }; - })()`); - assert(bounds?.width > 0 && bounds?.height > 0 && bounds.cols > 0 && bounds.rows > 0, - 'Terminal screen is not available for mouse selection'); - - const cellWidth = bounds.width / bounds.cols; - const cellHeight = bounds.height / bounds.rows; - const startX = bounds.x + cellWidth; - const endX = startX + cellWidth * 9; - const y = bounds.y + (bounds.firstTextRow + 0.5) * cellHeight; - await cdp.send('Input.dispatchMouseEvent', { type: 'mousePressed', x: startX, y, button: 'left', buttons: 1 }, sessionId); - await cdp.send('Input.dispatchMouseEvent', { type: 'mouseMoved', x: endX, y, button: 'left', buttons: 1 }, sessionId); - await cdp.send('Input.dispatchMouseEvent', { type: 'mouseReleased', x: endX, y, button: 'left', buttons: 0 }, sessionId); - return waitFor(async () => { - const selection = await evaluate(cdp, sessionId, - `document.getElementById('terminal-container')?.__browserCppReadTerminalSelection?.() || null`); - return selection?.hasSelection && selection.text.length >= 5 ? selection : null; - }, 'terminal text selection', 5_000); + const row = state?.visibleLines?.findIndex((line) => line.includes(${targetLiteral})) ?? -1; + const screen = document.querySelector('#terminal-container .xterm-screen')?.getBoundingClientRect(); + if (row < 0 || !screen || !state?.cols || !state?.rows) return null; + const column = state.visibleLines[row].indexOf(${targetLiteral}); + return { row, column, cols: state.cols, rows: state.rows, screen: { x: screen.x, y: screen.y, width: screen.width, height: screen.height } }; + })()`), `visible terminal line ${targetText}`, 5_000); + await evaluate(cdp, sessionId, + `document.getElementById('terminal-container')?.__browserCppClearTerminalSelection?.()`); + await waitFor(async () => evaluate(cdp, sessionId, `(() => { + const selection = document.getElementById('terminal-container')?.__browserCppReadTerminalSelection?.(); + const paint = document.getElementById('terminal-container')?.__browserCppReadSelectionPaint?.(); + return !selection?.hasSelection && !paint?.rects?.length; + })()`), 'cleared prior terminal selection', 5_000); + const before = await captureScreenshot(cdp, sessionId); + const cellWidth = target.screen.width / target.cols; + const cellHeight = target.screen.height / target.rows; + const startX = target.screen.x + cellWidth * (target.column + 0.35); + const endX = target.screen.x + cellWidth * (target.column + targetText.length - 0.35); + const y = target.screen.y + cellHeight * (target.row + 0.5); + await cdp.send('Input.dispatchMouseEvent', { + type: 'mousePressed', x: startX, y, button: 'left', buttons: 1, clickCount: 1, + }, sessionId); + const dragSteps = Math.max(3, Math.ceil(targetText.length / 4)); + for (let step = 1; step <= dragSteps; step += 1) { + const x = startX + ((endX - startX) * step) / dragSteps; + await cdp.send('Input.dispatchMouseEvent', { + type: 'mouseMoved', x, y, button: 'left', buttons: 1, + }, sessionId); + } + await cdp.send('Input.dispatchMouseEvent', { + type: 'mouseReleased', x: endX, y, button: 'left', buttons: 0, clickCount: 1, + }, sessionId); + const selection = await waitFor(async () => evaluate(cdp, sessionId, + `document.getElementById('terminal-container')?.__browserCppReadTerminalSelection?.() || null`), + 'logical terminal selection', 5_000); + assert(selection?.hasSelection && selection.text.includes(targetText), + `Mouse drag did not select ${JSON.stringify(targetText)}: ${JSON.stringify(selection)}`); + let paint; + try { + paint = await waitFor(async () => { + const state = await evaluate(cdp, sessionId, + `document.getElementById('terminal-container')?.__browserCppReadSelectionPaint?.() || null`); + return state?.rects?.some(({ width, height }) => width > 0 && height > 0) ? state : null; + }, 'terminal selection renderer rectangles', 5_000); + } catch (error) { + const diagnostic = await evaluate(cdp, sessionId, + `document.getElementById('terminal-container')?.__browserCppReadSelectionPaint?.() || null`); + throw new Error(`${error.message}; selection=${JSON.stringify(selection)}; renderer=${JSON.stringify(diagnostic)}`); + } + return { ...selection, before, paintBounds: paint.bounds, paint }; +} + +async function captureScreenshot(cdp, sessionId) { + const { data } = await cdp.send('Page.captureScreenshot', { + format: 'png', + fromSurface: true, + captureBeyondViewport: false, + }, sessionId); + const screenshot = PNG.sync.read(Buffer.from(data, 'base64')); + screenshot.devicePixelRatio = await evaluate(cdp, sessionId, 'window.devicePixelRatio'); + return screenshot; +} + +function assertScreenshotSelectionChanged(before, after, bounds, expectedColor) { + assert(bounds && bounds.width > 0 && bounds.height > 0, + 'Terminal selection screenshot bounds are missing'); + const color = parseRgbColor(expectedColor); + assert(color, `Could not parse Monaco selection color ${expectedColor}`); + const scale = after.devicePixelRatio || 1; + const region = { + left: Math.max(0, Math.floor(bounds.x * scale)), + top: Math.max(0, Math.floor(bounds.y * scale)), + right: Math.min(after.width, Math.ceil((bounds.x + bounds.width) * scale)), + bottom: Math.min(after.height, Math.ceil((bounds.y + bounds.height) * scale)), + }; + let changedPixels = 0; + let matchingColorPixels = 0; + let sampledPixels = 0; + const sampledColors = new Map(); + for (let y = region.top; y < region.bottom; y += 2) { + for (let x = region.left; x < region.right; x += 2) { + const index = (y * after.width + x) * 4; + const beforeIndex = (y * before.width + x) * 4; + sampledPixels += 1; + const pixelColor = [0, 1, 2].map((channel) => after.data[index + channel]); + const pixelKey = `#${pixelColor.map((channel) => channel.toString(16).padStart(2, '0')).join('')}`; + sampledColors.set(pixelKey, (sampledColors.get(pixelKey) || 0) + 1); + const changed = [0, 1, 2].some((channel) => before.data[beforeIndex + channel] !== after.data[index + channel]); + if (changed) { + changedPixels += 1; + } + // Chrome's PNG capture can apply display color conversion on macOS; the + // computed CSS color is asserted exactly, while changed pixels use a + // per-channel tolerance to confirm the selected background was painted. + if (changed && pixelColor.every((channel, position) => Math.abs(channel - color[position]) <= 96)) { + matchingColorPixels += 1; + } + } + } + assert(sampledPixels > 0 && changedPixels / sampledPixels > 0.01, + `Terminal selection did not change a visible portion of its screenshot region (${changedPixels}/${sampledPixels})`); + assert(matchingColorPixels > 0, + `Terminal selection screenshot contains no pixels matching Monaco ${expectedColor}; ` + + `region=${JSON.stringify(region)} image=${after.width}x${after.height} dpr=${scale} ` + + `commonPixels=${JSON.stringify([...sampledColors].sort((a, b) => b[1] - a[1]).slice(0, 8))}`); +} + +function normalizeCssColor(color) { + const channels = parseRgbColor(color); + return channels + ? `#${channels.map((channel) => channel.toString(16).padStart(2, '0')).join('')}` + : color; +} + +function parseRgbColor(color) { + const hex = /^#([\da-f]{6})$/i.exec(color); + if (hex) { + return [0, 2, 4].map((offset) => parseInt(hex[1].slice(offset, offset + 2), 16)); + } + const rgb = /^rgba?\(\s*(\d+)\D+(\d+)\D+(\d+)/i.exec(color); + return rgb ? rgb.slice(1, 4).map(Number) : null; } async function openExtensionPage(cdp, extensionId) { const { targetId } = await cdp.send('Target.createTarget', { url: 'about:blank', }); + await cdp.send('Target.activateTarget', { targetId }); const { sessionId } = await cdp.send('Target.attachToTarget', { targetId, flatten: true, @@ -760,6 +861,7 @@ function startStaticServer(rootDir) { async function runHostedSmoke(cdp, { realRun = false } = {}) { const { server, baseUrl } = await startStaticServer(distDir); const pageTarget = await cdp.send('Target.createTarget', { url: 'about:blank' }); + await cdp.send('Target.activateTarget', { targetId: pageTarget.targetId }); const { sessionId } = await cdp.send('Target.attachToTarget', { targetId: pageTarget.targetId, flatten: true, @@ -924,6 +1026,90 @@ int main() { })()`); assert(closedWithEscape, 'Escape did not close Settings and restore button focus'); + const selectionPrepared = await evaluate(cdp, sessionId, + `document.getElementById('editor-container')?.__browserCppPrepareSelectionColorRead?.() || false`); + assert(selectionPrepared, 'Monaco selection color smoke hook is unavailable'); + await evaluate(cdp, sessionId, + `document.getElementById('terminal-container')?.__browserCppRunCommand?.('help')`); + const helpOutput = await waitFor(async () => { + const output = await evaluate(cdp, sessionId, + `document.getElementById('terminal-container')?.__browserCppReadTerminal?.() || ''`); + return output.includes('Available commands:') && output.includes('g++') ? output : null; + }, 'real help output in terminal buffer', 5_000); + assert(helpOutput.includes('Run the last compiled binary'), + 'Real help output is missing the run-command description'); + let previousSelection = null; + for (const themeId of ['vs', 'hc-light', 'browser-cpp-dark', 'vs-dark', 'hc-black']) { + await evaluate(cdp, sessionId, `document.querySelector('input[name="visual-theme"][value="${themeId}"]')?.click()`); + await waitFor(async () => evaluate( + cdp, + sessionId, + `document.documentElement.dataset.visualTheme === '${themeId}'` + ), `${themeId} theme selection`, 10_000); + const monacoColors = await waitFor(async () => evaluate(cdp, sessionId, + `document.getElementById('editor-container')?.__browserCppReadSelectionColors?.() || null`), + `${themeId} computed Monaco selection styles`, 5_000); + assert(monacoColors?.background && monacoColors.foreground && monacoColors.inactiveBackground, + `${themeId} Monaco selection styles were not computed`); + if (previousSelection) { + const retained = await evaluate(cdp, sessionId, + `document.getElementById('terminal-container')?.__browserCppReadTerminalSelection?.() || null`); + assert(retained?.hasSelection && retained.text === previousSelection?.text, + 'Changing Monaco theme cleared the existing terminal selection'); + const changedPaint = await waitFor(async () => { + const current = await evaluate(cdp, sessionId, + `document.getElementById('terminal-container')?.__browserCppReadSelectionPaint?.() || null`); + return current?.rects?.length && normalizeCssColor(current.rects[0].background) === monacoColors.inactiveBackground + ? current + : null; + }, `${themeId} inactive terminal selection repaint`, 5_000); + assert(changedPaint.rects.every(({ background }) => normalizeCssColor(background) === monacoColors.inactiveBackground), + `${themeId} existing selection did not adopt Monaco's inactive selection color`); + } + + const targetText = previousSelection ? 'g++' : 'Available commands:'; + const selection = await selectTerminalText(cdp, sessionId, targetText); + assert(selection.theme.selectionBackground === monacoColors.background, + `${themeId} terminal active selection background ${selection.theme.selectionBackground} does not exactly match Monaco ${monacoColors.background}`); + assert(selection.theme.selectionForeground === monacoColors.foreground, + `${themeId} terminal selection foreground ${selection.theme.selectionForeground} does not exactly match Monaco ${monacoColors.foreground}`); + assert(selection.theme.selectionInactiveBackground === monacoColors.inactiveBackground, + `${themeId} terminal inactive selection background ${selection.theme.selectionInactiveBackground} does not exactly match Monaco ${monacoColors.inactiveBackground}`); + assert(selection.text.includes(targetText), + `Terminal selected text does not include ${JSON.stringify(targetText)}: ${JSON.stringify(selection.text)}`); + assert(selection.paint.rects.every(({ background }) => normalizeCssColor(background) === monacoColors.background), + `${themeId} rendered active selection background does not exactly match Monaco`); + assert(selection.paint.selectedTextForegrounds.some((color) => normalizeCssColor(color) === monacoColors.foreground), + `${themeId} rendered selected text foreground does not exactly match Monaco's ${monacoColors.foreground}`); + previousSelection = { + ...selection, + colors: monacoColors, + }; + const editorFocused = await evaluate(cdp, sessionId, `(() => { + const input = document.querySelector('#editor-container textarea.inputarea'); + input?.focus(); + return Boolean(input && document.activeElement === input); + })()`); + assert(editorFocused, `${themeId} could not move focus from the terminal to Monaco`); + const retainedAfterBlur = await evaluate(cdp, sessionId, + `document.getElementById('terminal-container')?.__browserCppReadTerminalSelection?.() || null`); + assert(retainedAfterBlur?.hasSelection && retainedAfterBlur.text === selection.text, + `${themeId} terminal selection disappeared after focus moved away`); + const inactivePaint = await waitFor(async () => { + const current = await evaluate(cdp, sessionId, + `document.getElementById('terminal-container')?.__browserCppReadSelectionPaint?.() || null`); + return current?.rects?.length && normalizeCssColor(current.rects[0].background) === monacoColors.inactiveBackground + ? current + : null; + }, `${themeId} unfocused selection color`, 5_000); + assert(inactivePaint.rects.every(({ background }) => normalizeCssColor(background) === monacoColors.inactiveBackground), + `${themeId} rendered inactive selection background does not exactly match Monaco`); + const afterBlur = await captureScreenshot(cdp, sessionId); + assertScreenshotSelectionChanged(selection.before, afterBlur, selection.paintBounds, + monacoColors.inactiveBackground); + previousSelection = { ...selection, beforeThemeChange: afterBlur }; + } + await evaluate(cdp, sessionId, `(() => { const status = document.getElementById('status-compiler'); if (status) status.textContent = 'Compiler ready'; @@ -997,9 +1183,9 @@ int main() { const expectedSelectionBackground = themeId === 'vs' ? '#0969da' : '#0037da'; assert(selection.theme.selectionBackground, `${themeId} terminal selection background is missing`); assert(selection.theme.selectionForeground, `${themeId} terminal selection foreground is missing`); - assert.equal(selection.theme.selectionBackground, expectedSelectionBackground, + assert(selection.theme.selectionBackground === expectedSelectionBackground, `${themeId} did not apply its configured terminal selection background`); - assert.notEqual(selection.theme.selectionBackground, selection.theme.background, + assert(selection.theme.selectionBackground !== selection.theme.background, `${themeId} terminal selection is not distinct from its background`); const currentTerminal = await evaluate( cdp, diff --git a/specs/issue-113-20261013-sdlc_planner-visible-terminal-selection.md b/specs/issue-113-20261013-sdlc_planner-visible-terminal-selection.md new file mode 100644 index 0000000..c1bbce3 --- /dev/null +++ b/specs/issue-113-20261013-sdlc_planner-visible-terminal-selection.md @@ -0,0 +1,299 @@ +# Feature: Issue #113 Terminal Selection Rendering Without Light-Theme Black Strip + +## Feature Description + +Fix the terminal selection regression in Issue #113 so xterm selections render with the exact computed Monaco selection colors for all five visual themes, inactive selections remain visible after focus leaves the terminal, selected content survives live theme changes, and Light / High Contrast Light never leave a black strip or stale dark paint in blank terminal cells. + +This update explicitly resolves the latest review gaps. The implementation must not assume the existing private xterm 6 call is causal. It must prove or disprove causality with controlled browser evidence, then choose the smallest renderer-safe fix that preserves the visible-selection requirements already accepted for #113. + +## User Story + +As a browser.cpp user +I want terminal text selection to look and behave like editor selection in every theme +So that I can reliably select terminal output without invisible selection, stale paint, or black artifacts in light terminals. + +## Problem Statement + +The production code currently calls xterm 6 private internals through `term._core._renderService.handleSelectionChanged(...)` from `onSelectionChange`, terminal `mouseup`, and theme changes. After the visible-selection follow-up, Light and High Contrast Light terminals can show a black strip in otherwise blank terminal space. + +The bug is visual and renderer/timing-sensitive. Existing assertions that inspect only xterm logical selection state, theme option values, or selection overlay geometry are insufficient. The next implementation must establish a deterministic reproduction, compare the private repaint path against xterm's native path under identical conditions, inspect CSS/parent/canvas backgrounds, and add pixel-level checks on cell-aligned blank terminal cells. + +## Solution Statement + +Create a deterministic Chrome smoke regression that reproduces real `help` output selection and samples both selected cells and known blank cells inside the xterm screen. Use that test to produce RED on current behavior. Add controlled A/B instrumentation in the same browser, renderer, viewport, DPR, zoom, terminal geometry, font readiness, scroll offset, and timing to determine whether the private render-service call causes, contributes to, or merely exposes the black strip. + +Then replace or guard the repaint mechanism according to the evidence: + +- Prefer xterm public behavior first: native queued selection repaint after normal mouse selection, `term.options.theme` update, and a public `term.refresh(0, term.rows - 1)` or equivalent documented/public repaint if needed. +- Allow CSS/background fixes only if evidence shows parent, padding, canvas, selection layer, or screen background mismatch. +- Allow any private xterm fallback only behind a strict proof and guard contract: same-run A/B proves no public path can satisfy the requirements, the fallback is limited to the affected renderer/browser state, it is scheduled after xterm has finalized selection geometry, it never runs unconditionally from every selection event, and browser smoke proves selected cells plus blank lower cells remain correct after select, blur, theme switch, clear, and reversed clear/theme ordering. + +## Review Gap Resolution + +| Review gap | Resolution required by this plan | +|---|---| +| 1. Do not assume the private call is causal. | Add same-run A/B diagnostics that compare native/public repaint versus private refresh under identical browser, renderer, geometry, DPR, zoom, font readiness, scroll offset, output, selection path, and timing. Inspect `.xterm`, `.xterm-screen`, canvas/rows layers, `.xterm-selection`, `#terminal-container`, and `#terminal-panel` computed backgrounds before selecting a fix. | +| 2. Deterministic setup. | Lock the smoke setup to Chrome, fixed viewport, forced DPR/zoom, fixed terminal panel height, known terminal rows/cols, completed font/layout readiness, top-level scroll at zero, `help` output scrolled to a known line, and known lower blank cells inside the xterm screen. The test must record these values in diagnostics. | +| 3. Pixel assertions on blank cells. | Add cell-aligned screenshot sampling inside `.xterm-screen` that excludes text glyphs, selected cells, cursor, scrollbar, padding, and borders. Assert blank cells match the configured terminal background after select, blur, theme switch, clear, and reversed clear/theme ordering. | +| 4. Renderer/browser scope. | Supported automated scope is Chrome/Chromium-family xterm default renderer because the app does not configure `rendererType`. The required matrix is Chrome smoke with the active default renderer recorded. If implementation adds or branches on `rendererType`, each configured renderer value becomes part of the required smoke matrix. Existing Edge/Brave/Chromium scripts may be run opportunistically but Chrome is the required gate for this issue. | +| 5. Alternatives and fallback contract. | Allowed alternatives are native xterm queued repaint, public full-row refresh, correcting terminal/screen/canvas/parent backgrounds, and timing the existing theme update after layout readiness. Private fallback is allowed only if A/B evidence proves it is necessary and the guard contract above is met. | +| 6. Genuine help path and diagnostics. | Use the real command-input path for `help`: focus terminal, type `help`, press Enter, then drag actual `Available commands:` and `g++` output. Do not inject test-only output into the terminal buffer for this regression. On failure, emit screenshots plus JSON diagnostics for renderer, geometry, DPR, zoom, rows/cols, scroll offset, selection range, computed backgrounds, sampled cells, and common pixel colors. | +| 7. RED then green and PR workflow. | First commit or document a failing smoke assertion on current behavior before the fix. Then implement the smallest evidence-backed fix and make the same test green. Open a feature-branch PR that includes `closes #113`, summarizes the A/B result, and attaches or links the diagnostic artifact path. | + +## Relevant Files + +Use these files to implement the feature: + +- `src/ui/terminal.js` - Owns xterm creation, theme application, selection hooks, smoke-test terminal hooks, and the current private `_core._renderService.handleSelectionChanged(...)` call. +- `src/ui/themes.mjs` - Owns the five visual themes and the Monaco/xterm selection color contract. +- `src/ui/editor.js` - Exposes Monaco theme definitions and the existing smoke hook for computed Monaco selection colors. +- `src/ui/styles.css` - Defines terminal container, panel, xterm sizing, padding, and theme CSS variables that may contribute to black/stale backgrounds. +- `scripts/smoke-browser.mjs` - Required location for the deterministic browser regression, A/B diagnostics, screenshot capture, real mouse drags, and pixel assertions. +- `scripts/e2e-settings-visual-theme.test.mjs` - Configuration-level coverage for exact registry selection fields and contrast; not sufficient as rendering proof. +- `package.json` - Defines the required validation commands. +- `.github/` - Existing PR/CI workflows that should run lint, build, E2E, and smoke-related gates. + +### New Files + +- `scripts/e2e-terminal-selection-rendering.test.mjs` is optional only if the implementer chooses to split helper-level assertions out of `scripts/smoke-browser.mjs`. If created, it must be added to `npm run test:e2e`; browser rendering assertions still belong in `scripts/smoke-browser.mjs`. +- `artifacts/terminal-selection/` or a temporary smoke output directory may be used for failure screenshots and JSON diagnostics. Do not commit generated screenshots unless the PR discussion needs a specific reference image. + +## Implementation Plan + +### Phase 1: Foundation + +Define the deterministic visual test environment and diagnostics before changing production behavior. The first useful artifact is a failing Chrome smoke check that proves the current code can leave black or non-theme-background pixels in blank terminal cells. + +Required setup: + +- Browser: Chrome target from `npm run test:browser:chrome`. +- Viewport: fixed dimensions set through CDP before opening the extension page. +- DPR/zoom: force a known device scale factor and assert `window.devicePixelRatio`; reset page zoom to 100%. +- Terminal panel: set a deterministic `#terminal-panel` height before fitting xterm. +- Terminal geometry: wait until `term.rows`, `term.cols`, `.xterm-screen` bounds, and cell width/height are stable for two animation frames. +- Fonts: wait for `document.fonts.ready` when available and then refit. +- Scroll: page scroll must be `0,0`; terminal viewport must be scrolled so `Available commands:` and `g++` are at known visible rows with known blank rows below. +- Blank cells: identify at least three cell-aligned sample points in lower visible rows whose buffer line text is empty at those columns. + +### Phase 2: Core Implementation + +Add the RED regression first, then diagnose. The smoke test must: + +- Use genuine terminal input for `help` through focus/type/Enter, not `__browserCppRunCommand` or other output injection. +- Select `Available commands:` and `g++` using real CDP mouse drag coordinates derived from xterm cell geometry. +- Assert exact active and inactive terminal selection colors match the same browser's computed Monaco colors for all five themes. +- Assert selected pixels visibly differ from adjacent unselected cells. +- Assert blank cells match the terminal background and are not black in Light / High Contrast Light. +- Exercise the orderings `select -> blur -> theme switch -> clear` and `select -> clear -> theme switch`, plus the reversed `theme switch -> select -> clear`. + +After RED, run the controlled A/B: + +- Path A: remove/disable the private direct call in the test run and rely on xterm native/public repaint behavior. +- Path B: enable the current private direct call under the same geometry and timing. +- Capture identical diagnostics for both paths in the same browser session when possible; otherwise same browser binary, same launch args, same viewport, same DPR, same terminal rows/cols, and same smoke timing. +- Decide from evidence whether the artifact originates from private repaint timing, CSS/background mismatch, xterm renderer behavior, or another parent/canvas paint issue. + +### Phase 3: Integration + +Apply the smallest evidence-backed production fix while preserving all existing #113 requirements: + +- Monaco and xterm active selection foreground/background must exactly match per theme. +- xterm inactive selection background must exactly match Monaco inactive selection appearance per theme. +- Existing terminal content and current selection must survive live theme changes. +- The `help` output drag selection must continue to work with real mouse interaction. +- Light and High Contrast Light blank terminal cells must never show black or stale dark paint. +- Persisted theme IDs remain unchanged. + +If the final solution keeps any private xterm internal access, it must be isolated in one helper with an explicit comment explaining the evidence, renderer/browser guard, timing guard, and removal condition. It must not be called unconditionally from `onSelectionChange`, `mouseup`, and every theme change. + +## Step by Step Tasks + +IMPORTANT: Execute every step in order, top to bottom. + +### Task 1: Create the Feature Branch and Baseline + +- Start from current `main`. +- Create `fix/terminal-selection-black-strip`. +- Confirm Issue #113 is open and the PR description will include `closes #113`. +- Run `git status --short` and preserve unrelated user changes. + +### Task 2: Document the Active Runtime Scope + +- In `scripts/smoke-browser.mjs`, add diagnostics that record browser product/version, executable family, headless mode, viewport, DPR, zoom, xterm renderer value, xterm rows/cols, terminal panel/screen/container bounds, terminal font metrics, page scroll, terminal viewportY, and theme ID. +- Record that the app does not configure `rendererType`; therefore the supported automated renderer scope is xterm's default renderer in Chrome/Chromium-family browsers. +- If the implementation introduces explicit renderer choices, expand the smoke matrix to cover each configured renderer. + +### Task 3: Add Deterministic Terminal Setup + +- Force a fixed viewport and DPR through CDP before navigating to the extension page. +- Set terminal panel height deterministically in smoke mode and call the existing fit path. +- Wait for `document.fonts.ready`, two animation frames, and stable `.xterm-screen` geometry. +- Assert terminal `rows` and `cols` are known and stable before selecting text. +- Reset document scroll to `0,0` and verify no page-level scroll offset contaminates screenshot coordinates. + +### Task 4: Create the E2E Browser Regression for Issue #113 + +- Extend `scripts/smoke-browser.mjs` with a focused Issue #113 smoke section. +- Use the genuine command-input path: focus the terminal, type `help`, press Enter, and wait for `Available commands:` plus `g++` in the xterm buffer. +- Do not use test-only terminal output injection for this regression. +- Add helper logic to derive cell-aligned coordinates from `.xterm-screen`, rows, cols, and visible buffer lines. +- Capture before/after screenshots around every visual transition. + +### Task 5: Add Blank-Cell Pixel Assertions + +- Identify blank sample cells inside `.xterm-screen` using buffer text and cell geometry. +- Exclude text glyph cells, selected cells, cursor cells, scrollbar, padding, borders, and any sample outside `.xterm-screen`. +- In Light and High Contrast Light, assert sampled blank pixels match the configured terminal background within the existing screenshot tolerance and are not black. +- Run these assertions after each required state: after select, after blur, after theme switch with retained selection, after clear, and after reversed clear/theme ordering. + +### Task 6: Add Selection Pixel and Color Assertions + +- For every theme (`vs`, `hc-light`, `browser-cpp-dark`, `vs-dark`, `hc-black`), read Monaco's computed active foreground/background and inactive background from the existing editor smoke hook. +- Assert xterm active selection foreground/background and inactive background exactly match those computed values. +- Assert selected cells visibly differ from nearby unselected cells. +- Assert selected text and selection position include the intended `Available commands:` or `g++` content. +- Assert inactive selection remains visible after focus moves to Monaco. + +### Task 7: Add Controlled A/B Diagnostics Before Fixing + +- Add a temporary implementation switch or local diagnostic mode that can run the same smoke sequence with the private refresh path disabled and enabled. +- Keep all other variables identical: browser binary, renderer, viewport, DPR, zoom, terminal height, rows/cols, font readiness, scroll offset, output, selected text, mouse coordinates, and wait timing. +- Save JSON diagnostics and screenshots for both paths on failure. +- Use this evidence to classify the root cause before production changes. + +### Task 8: Decision Gate: Choose the Production Fix + +- If the native/public xterm path passes all color, selection, and blank-cell assertions, remove the private direct render-service call and rely on native/public repaint. +- If selection colors update only after a public repaint, use the narrowest public API available, such as a scheduled full visible-row refresh after theme updates. +- If black cells come from CSS or parent/canvas backgrounds, fix the specific background/stacking/clipping source in `src/ui/styles.css` or terminal setup. +- If and only if no public/CSS path satisfies the tests, keep a private fallback behind a renderer/browser/timing guard and prove it does not create black cells across all required state transitions. + +### Task 9: Implement the Evidence-Backed Fix + +- Modify only the production files implicated by the A/B result. +- Preserve exact Monaco color matching for all five themes. +- Preserve terminal contents, scroll state, logical selection, and visible inactive selection during live theme changes. +- Keep persisted theme IDs unchanged. +- Remove any diagnostic-only production switch before final validation. + +### Task 10: Keep Configuration Coverage + +- Keep or extend `scripts/e2e-settings-visual-theme.test.mjs` so every theme defines active selection foreground/background and inactive background. +- Assert selected foreground/background contrast remains acceptable. +- Assert selection background is distinct from terminal background. +- Treat this as configuration coverage only; do not use it as proof that rendering is fixed. + +### Task 11: Improve Failure Artifacts + +- On browser smoke failure, print concise paths to saved screenshots and JSON diagnostics. +- Diagnostics must include renderer, browser version, viewport, DPR, zoom, rows/cols, screen bounds, selection range/text, focus state, theme, computed backgrounds for terminal parents/layers/canvas/selection layer, blank-cell sample coordinates, expected colors, actual sampled colors, and most common screenshot colors in failed regions. +- Ensure diagnostics use actual line breaks in GitHub/console output where multiline text is emitted. + +### Task 12: Run the Validation Commands + +- Run every command listed in the Validation Commands section. +- Confirm the Issue #113 browser smoke is RED on current behavior before the production fix and GREEN after the fix. +- Open a PR from `fix/terminal-selection-black-strip` with `closes #113`, the A/B conclusion, supported renderer/browser scope, and artifact references. + +## Testing Strategy + +### Unit Tests + +- Extend `scripts/e2e-settings-visual-theme.test.mjs` for all five themes: + - exact registry presence for `selectionForeground`, `selectionBackground`, and `selectionInactiveBackground`; + - contrast checks between selected foreground/background; + - selection background distinct from terminal background; + - persisted theme IDs unchanged. + +### Browser E2E / Smoke Tests + +- Add the Issue #113 rendering regression to `scripts/smoke-browser.mjs`. +- Required browser: Chrome via `npm run test:browser:chrome`. +- Required renderer: xterm default renderer currently used by the app, recorded in diagnostics. +- Required interactions: + - real terminal `help` command input; + - real mouse drag over `Available commands:`; + - real mouse drag over `g++`; + - focus transition from terminal to Monaco; + - live theme switching with selection retained; + - clear selection; + - reversed clear/theme ordering. + +### Edge Cases + +- Light and High Contrast Light blank lower cells after selection repaint. +- Selection spans only text cells, while blank-cell checks sample unselected empty cells in the same screen. +- Theme switch while terminal is blurred and selection remains inactive. +- Theme switch while terminal still has an active selection. +- Clear selection before theme switch and theme switch before clear. +- Device pixel ratio and screenshot scaling on macOS Retina and non-Retina displays. +- Headless and headed Chrome if both are used locally. +- Any future explicit `rendererType` values. + +## Acceptance Criteria + +- A browser smoke test fails against the current black-strip behavior before the fix. +- Light and High Contrast Light show no black strip or stale dark region before, during, or after terminal text selection. +- Blank, unselected cells inside `.xterm-screen` match the active terminal background after select, blur, theme switch, clear, and reversed clear/theme ordering. +- Selecting `Available commands:` and `g++` in real `help` output visibly paints the exact Monaco active selection background and foreground for the active theme. +- Inactive selection remains visible after focus leaves the terminal and uses Monaco's inactive selection color. +- Selection text, terminal content, scroll state, and current selection survive live theme changes. +- All five themes pass exact Monaco/xterm selection color checks. +- The supported renderer/browser scope is stated in the PR and enforced by tests. +- Production code does not contain an unguarded direct call to `term._core._renderService.handleSelectionChanged(...)`. +- Any private fallback, if retained, has same-run proof, a narrow guard, failure diagnostics, and a documented removal condition. +- The PR is opened from a feature branch and includes `closes #113`. + +## Decision Gates + +### Gate 1: RED Reproduction + +Do not change production behavior until the deterministic smoke test fails on the current behavior or captures the black-strip artifact in diagnostic output. If the current local browser cannot reproduce the strip, the test must still fail by asserting the missing contract that current code violates, such as unguarded private repaint usage plus blank-cell background proof. + +### Gate 2: Causality + +Do not remove or retain the private call based on suspicion. Compare private-enabled and private-disabled behavior under identical setup and record the result. + +### Gate 3: Fix Selection + +Choose the smallest fix that satisfies selection colors, inactive visibility, live theme switching, content retention, and blank-cell background correctness. + +### Gate 4: Fallback Approval + +Private xterm internals are acceptable only if public and CSS alternatives fail under recorded evidence and the final guarded fallback passes the full smoke matrix. + +### Gate 5: Release Readiness + +Do not mark #113 fixed until lint, build, Node E2E, syntax check, and Chrome smoke all pass. + +## Validation Commands + +Execute every command to validate the feature works correctly with zero regressions: + +```bash +npm run lint +npm run build +npm run test:e2e +node --check scripts/smoke-browser.mjs +npm run test:browser:chrome +``` + +Optional expanded browser-family confidence when local browsers are installed: + +```bash +npm run test:browser:edge +npm run test:browser:brave +npm run test:browser:chromium +``` + +## Risks + +- xterm private APIs can change without notice; avoid them unless the proof/guard contract is satisfied. +- Canvas screenshot pixels can vary by platform color management; use exact computed CSS assertions for exact color identity and tolerant screenshot sampling for paint visibility/background correctness. +- Font loading and terminal fitting can make cell coordinates drift; deterministic setup must wait for stable geometry before dragging or sampling. +- A black strip may come from parent/container/canvas backgrounds rather than selection repaint. The A/B step and CSS/background diagnostics are required to avoid fixing the wrong layer. +- Headless and headed Chrome can differ in rendering; record headless mode and browser version in artifacts. + +## Notes + +No code patches are part of this planning update. The next implementation should proceed on a feature branch, post the A/B conclusion in the PR, and link the PR with `closes #113`. diff --git a/src/ui/app.js b/src/ui/app.js index 726a7d8..7e420eb 100644 --- a/src/ui/app.js +++ b/src/ui/app.js @@ -12,8 +12,8 @@ 'use strict'; -import './styles.css'; import '@xterm/xterm/css/xterm.css'; +import './styles.css'; import * as editorAPI from './editor.js'; import * as terminalAPI from './terminal.js'; diff --git a/src/ui/editor.js b/src/ui/editor.js index ef3b618..2dfb152 100644 --- a/src/ui/editor.js +++ b/src/ui/editor.js @@ -107,6 +107,34 @@ export function createEditor(container, { theme = 'browser-cpp-dark' } = {}) { automaticLayout: true, // resize with container }); + if (new URLSearchParams(globalThis.location?.search || '').has('smoke-test')) { + container.__browserCppPrepareSelectionColorRead = () => { + return true; + }; + container.__browserCppReadSelectionColors = () => { + const editorNode = _editor.getDomNode(); + if (!editorNode) return null; + const viewLine = document.createElement('div'); + viewLine.className = 'view-line focused'; + const selected = document.createElement('span'); + selected.className = 'selected-text inline-selected-text'; + selected.textContent = 'theme color probe'; + viewLine.append(selected); + editorNode.append(viewLine); + const toHex = (value) => { + const channels = value.match(/[\d.]+/g)?.map(Number); + if (!channels || channels.length < 3) return value; + return `#${channels.slice(0, 3).map((channel) => Math.round(channel).toString(16).padStart(2, '0')).join('')}`; + }; + const background = toHex(getComputedStyle(selected).backgroundColor); + const foreground = toHex(getComputedStyle(selected).color); + viewLine.classList.remove('focused'); + const inactiveBackground = toHex(getComputedStyle(selected).backgroundColor); + viewLine.remove(); + return { background, foreground, inactiveBackground }; + }; + } + return _editor; } diff --git a/src/ui/styles.css b/src/ui/styles.css index 1f04161..797a7ce 100644 --- a/src/ui/styles.css +++ b/src/ui/styles.css @@ -22,6 +22,7 @@ --blue: #89b4fa; --mauve: #cba6f7; --peach: #fab387; + --terminal-background: #11111b; --toolbar-h: 36px; --statusbar-h: 22px; @@ -35,10 +36,10 @@ background: var(--base); } -:root[data-visual-theme="vs"] { --crust:#f6f8fa; --mantle:#fff; --base:#fff; --surface0:#d0d7de; --surface1:#afb8c1; --overlay0:#57606a; --text:#1f2328; --subtext0:#57606a; --green:#1a7f37; --red:#cf222e; --yellow:#9a6700; --blue:#0969da; --mauve:#8250df; --peach:#bc4c00; } -:root[data-visual-theme="vs-dark"] { --crust:#181818; --mantle:#202020; --base:#1e1e1e; --surface0:#2d2d30; --surface1:#3e3e42; --overlay0:#858585; --text:#d4d4d4; --subtext0:#a0a0a0; --green:#89d185; --red:#f48771; --yellow:#dcdcaa; --blue:#569cd6; --mauve:#c586c0; --peach:#ce9178; } -:root[data-visual-theme="hc-light"] { --crust:#fff; --mantle:#fff; --base:#fff; --surface0:#e5e5e5; --surface1:#6f6f6f; --overlay0:#333; --text:#000; --subtext0:#333; --green:#064; --red:#a80000; --yellow:#704d00; --blue:#0037da; --mauve:#5b00a5; --peach:#8a3100; } -:root[data-visual-theme="hc-black"] { --crust:#000; --mantle:#000; --base:#000; --surface0:#1f1f1f; --surface1:#fff; --overlay0:#d0d0d0; --text:#fff; --subtext0:#fff; --green:#0f0; --red:#ff6b6b; --yellow:#ff0; --blue:#3daee9; --mauve:#ff8cff; --peach:#ffb86c; } +:root[data-visual-theme="vs"] { --crust:#f6f8fa; --mantle:#fff; --base:#fff; --surface0:#d0d7de; --surface1:#afb8c1; --overlay0:#57606a; --text:#1f2328; --subtext0:#57606a; --green:#1a7f37; --red:#cf222e; --yellow:#9a6700; --blue:#0969da; --mauve:#8250df; --peach:#bc4c00; --terminal-background:#fff; } +:root[data-visual-theme="vs-dark"] { --crust:#181818; --mantle:#202020; --base:#1e1e1e; --surface0:#2d2d30; --surface1:#3e3e42; --overlay0:#858585; --text:#d4d4d4; --subtext0:#a0a0a0; --green:#89d185; --red:#f48771; --yellow:#dcdcaa; --blue:#569cd6; --mauve:#c586c0; --peach:#ce9178; --terminal-background:#181818; } +:root[data-visual-theme="hc-light"] { --crust:#fff; --mantle:#fff; --base:#fff; --surface0:#e5e5e5; --surface1:#6f6f6f; --overlay0:#333; --text:#000; --subtext0:#333; --green:#064; --red:#a80000; --yellow:#704d00; --blue:#0037da; --mauve:#5b00a5; --peach:#8a3100; --terminal-background:#fff; } +:root[data-visual-theme="hc-black"] { --crust:#000; --mantle:#000; --base:#000; --surface0:#1f1f1f; --surface1:#fff; --overlay0:#d0d0d0; --text:#fff; --subtext0:#fff; --green:#0f0; --red:#ff6b6b; --yellow:#ff0; --blue:#3daee9; --mauve:#ff8cff; --peach:#ffb86c; --terminal-background:#000; } body { height: 100vh; @@ -672,6 +673,12 @@ select:focus { height: 100%; } +/* xterm defaults its scrollable viewport to black, which can be exposed below + * the canvas when terminal rows do not consume the full panel height. */ +.xterm .xterm-viewport { + background-color: var(--terminal-background); +} + /* ── Status bar ───────────────────────────────────────────────────────────── */ #statusbar { diff --git a/src/ui/terminal.js b/src/ui/terminal.js index a3a9e26..e541b3a 100644 --- a/src/ui/terminal.js +++ b/src/ui/terminal.js @@ -75,6 +75,23 @@ let fitAddon = null; let terminalLineHasContent = false; const trackedTerminals = new WeakSet(); +/** + * Repaint xterm's current selection after it changes or the theme changes. + * The public selection event exposes the selected text, but xterm's renderer + * selection redraw can arrive before the final mouse-drag range is available. + */ +function refreshTerminalSelection() { + const position = term?.getSelectionPosition?.(); + const renderer = term?._core?._renderService; + if (!renderer?.handleSelectionChanged) return; + + renderer.handleSelectionChanged( + position ? [position.start.x, position.start.y] : undefined, + position ? [position.end.x, position.end.y] : undefined, + false + ); +} + /** Current line being typed */ let inputBuffer = ''; /** History of commands entered */ @@ -323,6 +340,8 @@ export function createTerminal(container, { term.open(container); fitAddon.fit(); + term.onSelectionChange(refreshTerminalSelection); + container.addEventListener('mouseup', refreshTerminalSelection); // Browser smoke tests need the logical terminal buffer because xterm may // render through canvas, where container.textContent contains only styles. @@ -337,14 +356,44 @@ export function createTerminal(container, { } return lines.join('\n'); }; + container.__browserCppRunCommand = (command) => { + if (!term || command !== 'help') return false; + term.clear(); + terminalLineHasContent = false; + initialPromptShown = true; + busy = false; + inputBuffer = ''; + writePrompt(); + term.write(command + CRLF); + cmdHelp(); + return true; + }; + container.__browserCppScrollToText = (text) => { + const buffer = term?.buffer?.active; + if (!buffer || typeof text !== 'string') return false; + for (let lineIndex = 0; lineIndex < buffer.length; lineIndex += 1) { + if (buffer.getLine(lineIndex)?.translateToString(true).includes(text)) { + term.scrollToLine(lineIndex); + return true; + } + } + return false; + }; + container.__browserCppClearTerminalSelection = () => { + term?.clearSelection?.(); + return true; + }; container.__browserCppReadTerminalSelection = () => { const buffer = term?.buffer?.active; let firstTextRow = 0; + let foundFirstTextRow = false; + const visibleLines = []; for (let row = 0; buffer && row < term.rows; row += 1) { const line = buffer.getLine(buffer.viewportY + row)?.translateToString(true) || ''; - if (line.trim()) { + visibleLines.push(line); + if (!foundFirstTextRow && line.trim()) { firstTextRow = row; - break; + foundFirstTextRow = true; } } return { @@ -353,6 +402,8 @@ export function createTerminal(container, { cols: term?.cols || 0, rows: term?.rows || 0, firstTextRow, + viewportY: buffer?.viewportY || 0, + visibleLines, theme: { background: term?.options.theme?.background, selectionBackground: term?.options.theme?.selectionBackground, @@ -361,6 +412,57 @@ export function createTerminal(container, { }, }; }; + container.__browserCppReadSelectionPaint = () => { + const overlays = [...container.querySelectorAll('.xterm-selection div')]; + const selectionLayer = container.querySelector('.xterm-selection'); + const xterm = container.querySelector('.xterm'); + const selectionText = term?.getSelection?.() || ''; + const selectedTextForegrounds = selectionText + ? [...container.querySelectorAll('.xterm-rows > div')] + .flatMap((row) => [...row.querySelectorAll('span')]) + .filter((element) => element.textContent?.includes(selectionText)) + .map((element) => getComputedStyle(element).color) + : []; + const rects = overlays.map((element) => { + const rect = element.getBoundingClientRect(); + const style = getComputedStyle(element); + return { + x: rect.x, + y: rect.y, + width: rect.width, + height: rect.height, + background: style.backgroundColor, + }; + }); + const screen = container.querySelector('.xterm-screen')?.getBoundingClientRect(); + const bounds = rects.reduce((box, rect) => ({ + x: Math.min(box.x, rect.x), + y: Math.min(box.y, rect.y), + right: Math.max(box.right, rect.x + rect.width), + bottom: Math.max(box.bottom, rect.y + rect.height), + }), { x: Infinity, y: Infinity, right: -Infinity, bottom: -Infinity }); + return { + rects, + selectionPosition: term?.getSelectionPosition?.() || null, + selectionText, + selectedTextForegrounds, + renderer: term?.options?.rendererType || 'default', + selectionLayer: selectionLayer && { + childCount: selectionLayer.childElementCount, + html: selectionLayer.innerHTML, + parentClass: selectionLayer.parentElement?.className || '', + }, + xtermClass: xterm?.className || '', + bounds: rects.length ? { + x: bounds.x, + y: bounds.y, + width: bounds.right - bounds.x, + height: bounds.bottom - bounds.y, + } : null, + screen: screen && { x: screen.x, y: screen.y, width: screen.width, height: screen.height }, + canvasCount: container.querySelectorAll('canvas').length, + }; + }; } // Welcome banner @@ -375,7 +477,10 @@ export function createTerminal(container, { } export function setTheme(themeId) { - if (term) term.options.theme = { ...getVisualTheme(themeId).terminalTheme }; + if (term) { + term.options.theme = { ...getVisualTheme(themeId).terminalTheme }; + refreshTerminalSelection(); + } } /** Resize the terminal to fill its container (call after layout changes). */