From 906f7c45bd8b7f295698d2c01ef97837d54ca114 Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 2 Sep 2026 07:31:02 -0600 Subject: [PATCH 01/10] Run the e2e suite off-screen, and pay down the automation debt behind it (BL-16799) Every run of the src/BloomE2E suite took over the developer's desktop, and a test could pass while Bloom drove a document the test never looked at. A new --headless flag puts the shell and the splash screen far left of every monitor, out of the taskbar, without touching the saved window placement. The window is off-screen rather than minimized because WebView2 stops painting a minimized window and screenshots come back blank; BLOOM_E2E_HEADED=1 or Playwright's --debug shows it. Browsers built on the UI thread share one WebView2 environment under --e2e, so a run has one browser process and one remote-debugging listener, and a new e2e/shellUrl hook names the document Bloom drives for the fixture to confirm. That sharing is limited to the UI thread: an environment belongs to the thread that created it, and publishing a BloomPUB builds its browsers on the thread serving the API call. editView/jumpToPage now queues a jump that arrives while the Edit tab is not showing, is navigating, or is saving, and reports a failure when it can do neither, instead of replying success to a jump it dropped. A new e2e/setCollectionLanguages hook does the work of the Collection Settings dialog's OK button, so no test composes .bloomCollection XML. The workspace tabs carry data-testid attributes, so the suite no longer matches on localized labels. BLOOM_E2E_VITE_PORT points a launched Bloom at a dev server, so a front-end edit reaches the suite with no build. Also: a visual-regression case collects every failed image comparison and fails once at the end; toolbox tool registration is a side-effect-free registerAllToolboxTools() that both the bootstrap and the test harness call; the component-tester Playwright suites have a nightly job; the bloom-automation scripts answer --help without killing anything and reject an unknown flag; a screenshot helper captures a whole book page the safe way; and the add-e2e-test skill states that every step of a test is a helper call. Co-Authored-By: Claude Fable 5.1 --- .github/skills/add-e2e-test/SKILL.md | 22 +- .../bloom-automation/bloomProcessStatus.mjs | 21 ++ .../bloom-automation/killBloomProcess.mjs | 34 +++ .../bloom-automation/launcherControl.mjs | 18 ++ .github/workflows/nightly.yml | 71 ++++- .../bookAndPageSettings/StyleAndFontTable.tsx | 4 +- .../toolbox/canvas/customXmatterPage.tsx | 4 + .../toolbox/registerAllToolboxTools.ts | 47 ++++ .../bookEdit/toolbox/toolboxBootstrap.ts | 30 +- .../component-tests/error-handling.uitest.ts | 26 +- .../ToolboxRootTestHarness.tsx | 48 +--- .../bloom-exe-collection-topbar.uitest.ts | 2 +- .../react_components/TopBar/TopBar.tsx | 8 + .../component-tests/bloom-exe-tabs.uitest.ts | 10 +- .../component-tester/bloomExeCdp.ts | 25 +- .../component-tests/test-helpers.ts | 7 +- src/BloomBrowserUI/utils/bloomApi.ts | 10 +- src/BloomE2E/AUTOMATION-DEBT.md | 186 +++++++------ src/BloomE2E/README.md | 51 +++- src/BloomE2E/fixtures/bloomTest.ts | 96 ++++++- src/BloomE2E/fixtures/launchBloom.ts | 47 +++- src/BloomE2E/helpers/bookMaking.ts | 47 ++-- src/BloomE2E/helpers/collection.ts | 33 +++ src/BloomE2E/helpers/publish.ts | 48 ++-- src/BloomE2E/helpers/screenshot.ts | 259 ++++++++++++++++++ src/BloomE2E/helpers/workspace.ts | 17 +- src/BloomE2E/tests/capture-book-page.spec.ts | 53 ++++ .../tests/publish-text-languages.spec.ts | 71 ++--- src/BloomE2E/tests/workspace-tabs.spec.ts | 4 +- src/BloomExe/Edit/EditingModel.cs | 87 +++++- src/BloomExe/Edit/EditingStateMachine.cs | 64 ++++- src/BloomExe/Program.cs | 15 + src/BloomExe/Shell.cs | 55 +++- src/BloomExe/SplashScreen.cs | 23 +- src/BloomExe/WebView2Browser.cs | 31 +++ src/BloomExe/Workspace/WorkspaceView.cs | 10 + src/BloomExe/web/controllers/E2eTestingApi.cs | 104 +++++++ .../web/controllers/EditingViewApi.cs | 15 +- src/BloomTests/ProgramTests.cs | 27 ++ src/BloomVisualRegressionTests/index.spec.ts | 39 ++- 40 files changed, 1446 insertions(+), 323 deletions(-) create mode 100644 src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts create mode 100644 src/BloomE2E/helpers/screenshot.ts create mode 100644 src/BloomE2E/tests/capture-book-page.spec.ts diff --git a/.github/skills/add-e2e-test/SKILL.md b/.github/skills/add-e2e-test/SKILL.md index b332ab94b7ba..5c39750daf2e 100644 --- a/.github/skills/add-e2e-test/SKILL.md +++ b/.github/skills/add-e2e-test/SKILL.md @@ -261,11 +261,31 @@ pnpm exec playwright test tests/workspace-tabs.spec.ts # one file pnpm exec playwright test -g "switching workspace tabs" # one test by title ``` -A run opens a real Bloom window; that is expected. It needs a built `Bloom.exe` under +A run launches a real Bloom, but no window appears: the fixture passes `--headless`, which puts +Bloom's window far outside every monitor, so a run does not take your desktop over. Set +`BLOOM_E2E_HEADED=1` to watch it (`--debug` sets it for you). The window goes off-screen rather +than minimized or hidden because WebView2 stops painting a minimized window, which makes every +screenshot blank. + +A run needs a built `Bloom.exe` under `output/{Debug,Release}/{x64,AnyCPU,}/` and the inputs at `output/testing-inputs`. Point `BLOOM_TESTING_INPUTS_DIR` at a bloom-testing-inputs checkout to use your own in-progress collections instead of the pinned ones. +The launched Bloom serves its React UI from the built `output/browser`, so **an edit to a `.tsx` +file does not reach a run until that bundle is rebuilt.** To test the working tree instead, start +a dev server and name its port in `BLOOM_E2E_VITE_PORT`; the fixture passes `--vite-port` and +Bloom loads every React control from it. Set `PORT` as well as `--port`, or the dev server's +HMR and React-Refresh URLs still point at 5173 and the page fails to load its entry module. + +```bash +PORT=5199 pnpm exec vite --port 5199 --strictPort # in src/BloomBrowserUI +BLOOM_E2E_VITE_PORT=5199 pnpm test # in src/BloomE2E +``` + +Leaving the variable unset does not mean "no dev server": a dev build of Bloom looks for one on +5173 by itself, so what a run tests can depend on what else is running. + `.github/workflows/nightly.yml` does not run this suite yet. The step it will need is the same `pnpm test` in that folder, after the Release build and the testing-inputs fetch that the visual-regression job already does. diff --git a/.github/skills/bloom-automation/bloomProcessStatus.mjs b/.github/skills/bloom-automation/bloomProcessStatus.mjs index cc3df6e285e3..8c8652557fab 100644 --- a/.github/skills/bloom-automation/bloomProcessStatus.mjs +++ b/.github/skills/bloom-automation/bloomProcessStatus.mjs @@ -13,6 +13,18 @@ import { toWorkspaceTabsEndpoint, } from "./bloomProcessCommon.mjs"; +const usage = `Report the Bloom and launcher processes this machine is running. + + node bloomProcessStatus.mjs [options] + + --help, -h Print this and exit. + --json Report as JSON. + --running-bloom Also probe each running Bloom's own HTTP server. + --repo-root The worktree to judge instances against (default: this checkout). + --http-port Report the instance whose server answers on this port. + +This script only reads; it never kills anything.`; + const parseArgs = () => { const args = process.argv.slice(2); const options = { @@ -25,6 +37,11 @@ const parseArgs = () => { for (let i = 0; i < args.length; i++) { const arg = args[i]; + if (arg === "--help" || arg === "-h") { + console.log(usage); + process.exit(0); + } + if (arg === "--json") { options.json = true; continue; @@ -57,6 +74,10 @@ const parseArgs = () => { ); continue; } + + // An unknown flag is a mistake, and silently ignoring it hides it. + console.error(`Unknown option ${arg}.\n\n${usage}`); + process.exit(2); } return options; diff --git a/.github/skills/bloom-automation/killBloomProcess.mjs b/.github/skills/bloom-automation/killBloomProcess.mjs index 7c97dde452bc..f68a8fc35873 100644 --- a/.github/skills/bloom-automation/killBloomProcess.mjs +++ b/.github/skills/bloom-automation/killBloomProcess.mjs @@ -10,6 +10,33 @@ import { requireTcpPortOption, } from "./bloomProcessCommon.mjs"; +const usage = `Kill the Bloom.exe (and dotnet.exe BloomExe.csproj) processes of this worktree. + + node killBloomProcess.mjs [options] + + --help, -h Print this and exit without killing anything. + --json Report what was killed as JSON. + --only-mismatched Kill only instances whose repo root is not this worktree. + --repo-root The worktree to judge instances against (default: this checkout). + --http-port Kill the instance whose server answers on this port. + --pid Kill this process and the Bloom processes in its chain. + --watch-pid Kill this launcher/watch process and its Bloom processes. + +With no --http-port, --pid or --watch-pid, this kills EVERY Bloom this worktree owns.`; + +// Print the usage and exit 0 without killing anything, or reject an unknown flag with a non-zero +// exit. Reading the usage first must never be the dangerous move: this script used to ignore +// --help and go straight to its destructive default (AUTOMATION-DEBT.md, "Automation helper +// scripts run destructive defaults on unknown flags"). +const exitWithUsage = (unknownArgument) => { + if (unknownArgument) { + console.error(`Unknown option ${unknownArgument}.\n\n${usage}`); + process.exit(2); + } + console.log(usage); + process.exit(0); +}; + const parseArgs = () => { const args = process.argv.slice(2); const options = { @@ -24,6 +51,10 @@ const parseArgs = () => { for (let i = 0; i < args.length; i++) { const arg = args[i]; + if (arg === "--help" || arg === "-h") { + exitWithUsage(); + } + if (arg === "--json") { options.json = true; continue; @@ -76,7 +107,10 @@ const parseArgs = () => { if (arg.startsWith("--watch-pid=")) { options.watchPid = Number(arg.slice("--watch-pid=".length)); + continue; } + + exitWithUsage(arg); } return options; diff --git a/.github/skills/bloom-automation/launcherControl.mjs b/.github/skills/bloom-automation/launcherControl.mjs index c614f481079a..fcfa614ccba1 100644 --- a/.github/skills/bloom-automation/launcherControl.mjs +++ b/.github/skills/bloom-automation/launcherControl.mjs @@ -43,6 +43,17 @@ const actionNames = [ "--ensure-running", ]; +const usage = `Command the go.sh launcher for this worktree. + + node launcherControl.mjs [options] + + actions: ${actionNames.join(", ")} + options: --json, --wait-ready, --repo-root , --timeout-ms + --help, -h Print this and exit without commanding anything. + + --restart rebuilds and relaunches; --quit-bloom stops Bloom and leaves the launcher; + --shutdown stops Bloom, the launcher, and Vite.`; + const parseArgs = () => { const args = process.argv.slice(2); const options = { @@ -56,6 +67,13 @@ const parseArgs = () => { for (let i = 0; i < args.length; i++) { const arg = args[i]; + // Print the usage and stop, before any action can run. Asking a destructive script how to + // use it must never be the destructive move. + if (arg === "--help" || arg === "-h") { + console.log(usage); + process.exit(0); + } + if (actionNames.includes(arg)) { if (options.action) { throw new Error( diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 0f922401ce73..1007ae744e6b 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -1,19 +1,20 @@ # Nightly build + full test run of everything on master. # # This is a health check, not a release: it builds the front-end and the C# solution and -# runs all three test suites — front-end vitest, C# NUnit, and the visual regression suite — -# but produces no installer, does no signing, and publishes nothing. It exists to catch +# runs all four test suites — front-end vitest, C# NUnit, the visual regression suite, and the +# component-tester Playwright suite — but produces no installer, does no signing, and +# publishes nothing. It exists to catch # breakage that the PR checks miss — e.g. tests excluded from PR runs, or rot from # dependency/runner drift — on a predictable cadence. # -# Each suite publishes its own check run / job-summary section, so the commit shows three +# Each suite publishes its own check run / job-summary section, so the commit shows four # independent results rather than one merged total. See the "Test reports" steps at the end. # # Schedule: 04:00 UTC daily. GitHub cron is always UTC (== GMT, no DST), so this is a # literal 4am GMT. Note GitHub may delay scheduled runs during peak load; exact timing is # best-effort. Can also be run on demand via the Actions "Run workflow" button. # -# A manual run can pick which of the three suites to run; all three are ticked by default, so the +# A manual run can pick which of the four suites to run; all four are ticked by default, so the # default manual run matches the scheduled one. Unticking the ones you don't need is how to iterate # quickly when chasing a failure in a single suite: the C# suite alone is ~9 minutes of a ~22-minute # run, so dropping it takes an attempt to ~12. That was added while hunting the intermittent @@ -50,6 +51,10 @@ on: description: "Visual-regression suite (drives a real Bloom)" type: boolean default: true + run_component_tests: + description: "Component-tester suite (Playwright against the Vite harness)" + type: boolean + default: true # Don't stack nightlies: if a manual run overlaps the scheduled one, let the first finish. concurrency: @@ -70,12 +75,13 @@ jobs: # VersionNumbers requires a 4-part BUILD_NUMBER; nightlies never publish, so a # throwaway value that just identifies the run is fine. BUILD_NUMBER: 0.0.${{ github.run_number }}.0 - # Which suites this run should do. A scheduled run always does all three; a manual run does + # Which suites this run should do. A scheduled run always does all of them; a manual run does # whichever boxes were ticked. Resolved once here so the step conditions stay readable, and # compared as strings at the point of use because env values are always strings. RUN_FRONTEND_TESTS: ${{ github.event_name != 'workflow_dispatch' || inputs.run_frontend_tests }} RUN_CSHARP_TESTS: ${{ github.event_name != 'workflow_dispatch' || inputs.run_csharp_tests }} RUN_VISUAL_REGRESSION: ${{ github.event_name != 'workflow_dispatch' || inputs.run_visual_regression }} + RUN_COMPONENT_TESTS: ${{ github.event_name != 'workflow_dispatch' || inputs.run_component_tests }} steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 @@ -104,6 +110,7 @@ jobs: src/BloomBrowserUI/pnpm-lock.yaml src/content/pnpm-lock.yaml src/BloomVisualRegressionTests/pnpm-lock.yaml + src/BloomBrowserUI/react_components/component-tester/pnpm-lock.yaml # ----- Dependencies (mirrors init.sh) ----- @@ -370,6 +377,50 @@ jobs: retention-days: 4 if-no-files-found: ignore + # ----- Component-tester tests ----- + # src/BloomBrowserUI/react_components/component-tester renders one React component at a + # time in a Vite dev server and drives it with Playwright (the *.uitest.ts files beside + # each component). Nothing ran these until now, which is how the harness sat broken for + # weeks — a React 17 pin plus a config bug — and it would rot again silently + # (AUTOMATION-DEBT.md, "The component-tester Playwright suites are not in CI"). + # + # This is the component config only (playwright.config.ts, which the package's own + # `pnpm test` uses). The sibling playwright.bloom-exe.config.ts attaches over CDP to a + # Bloom the developer already has running, so it needs the src/BloomE2E launch fixture + # before it can run unattended; its specs are excluded by that config's testIgnore. + # + # It needs neither build: the harness serves the components from its own Vite dev + # server, which playwright.config.ts starts as its webServer. So this group depends on + # nothing above it, and runs even when the builds failed. + - name: Set up component-tester tests + id: setup_component_tests + if: ${{ !cancelled() && env.RUN_COMPONENT_TESTS == 'true' }} + working-directory: src/BloomBrowserUI/react_components/component-tester + shell: bash + run: | + pnpm install --frozen-lockfile + pnpm exec playwright install chromium + + # Playwright takes its junit path from PLAYWRIGHT_JUNIT_OUTPUT_NAME, not from a CLI + # flag: it has no --outputFile (that is vitest's). The path is relative to the working + # directory, so it climbs back to the repo root's output/Tests like the other suites. + # + # --timeout raises the per-test 30s of playwright.config.ts, which is a developer's + # number: it assumes a dev server that has already transformed the module graph. Every + # CI run starts cold, and the first request for a component pays for transforming that + # whole graph. Locally, a cold first run failed 12 tests on `page.goto` timing out + # while the warm re-run of the same suite passed 142 in 58 seconds. So give the runner + # room rather than reporting a cold start as a broken component. A passing test still + # returns as soon as it passes, so this costs a green run nothing. + - name: Run component-tester tests + id: component_tests + if: ${{ !cancelled() && steps.setup_component_tests.outcome == 'success' }} + working-directory: src/BloomBrowserUI/react_components/component-tester + shell: bash + env: + PLAYWRIGHT_JUNIT_OUTPUT_NAME: ../../../../output/Tests/component-tester-junit.xml + run: pnpm test --timeout=120000 --reporter=list,junit + # ----- Test reports, one per suite ----- # Each suite gets its OWN invocation of the publish action, and therefore its own # check run on the commit and its own section in the job summary: pass/fail/skip @@ -426,6 +477,15 @@ jobs: action_fail_on_inconclusive: true files: output/Tests/visual-regression-junit.xml + - name: Publish component-tester test results + if: ${{ !cancelled() && steps.component_tests.outcome != 'skipped' }} + uses: EnricoMi/publish-unit-test-result-action/windows@d0a4676d0e0b938bc201470d88276b7c74c712b3 # v2.24.0 + with: + check_name: "Nightly tests: component-tester (Playwright)" + comment_mode: "off" + action_fail_on_inconclusive: true + files: output/Tests/component-tester-junit.xml + - name: Upload test results if: ${{ always() }} uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 @@ -438,4 +498,5 @@ jobs: output/Tests/Release/x64/TestResults.xml output/Tests/vitest-junit.xml output/Tests/visual-regression-junit.xml + output/Tests/component-tester-junit.xml retention-days: 14 diff --git a/src/BloomBrowserUI/bookEdit/bookAndPageSettings/StyleAndFontTable.tsx b/src/BloomBrowserUI/bookEdit/bookAndPageSettings/StyleAndFontTable.tsx index b4bc47669e94..7b7f317dc699 100644 --- a/src/BloomBrowserUI/bookEdit/bookAndPageSettings/StyleAndFontTable.tsx +++ b/src/BloomBrowserUI/bookEdit/bookAndPageSettings/StyleAndFontTable.tsx @@ -46,7 +46,9 @@ export const StyleAndFontTable: React.FunctionComponent<{ function closeDialogAndJumpToPage(pageId: string) { props.closeDialog(); - postString("editView/jumpToPage", pageId); + // report: false — the edit tab declines a jump it cannot do (it is mid-save), and that + // is not something to raise a problem report about. See EditingModel.JumpToPage. + postString("editView/jumpToPage", pageId, false); } return ( diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx b/src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx index 23a110a078eb..37004f382a44 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/customXmatterPage.tsx @@ -567,9 +567,13 @@ function renderPageLayoutMenu(page: HTMLElement): void { ); // Persist the newly created custom layout state so a later switch back // to standard has matching server-side state to work from. + // report: false — the edit tab declines a jump it cannot do (it is mid-save), and + // that is not something to raise a problem report about. See + // EditingModel.JumpToPage. await postString( "editView/jumpToPage", page.getAttribute("id")!, + false, ); renderPageLayoutMenu(page); } else if (selection === "custom" && response) { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts b/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts new file mode 100644 index 000000000000..9e0135e465a0 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts @@ -0,0 +1,47 @@ +// The one list of the tools that belong in the Edit tab's toolbox. +// +// ToolboxRoot renders a section only for a tool that is in the master tool list, and a tool gets +// there by being registered. In the running app that used to happen as a side effect of loading +// toolboxBootstrap.ts, which also renders its own toolbox root and assigns window.toolboxBundle. +// A test harness cannot afford those side effects, so it duplicated the list with a "keep in +// sync" comment. This module is the list, and nothing else: importing it registers nothing, and +// both toolboxBootstrap.ts and react_components/ToolboxRootTestHarness call the function below. +// (AUTOMATION-DEBT.md: "Toolbox tool registration is a side effect of toolboxBootstrap".) + +import { ToolBox, getMasterToolList } from "./toolbox"; +import { DecodableReaderTool } from "./readers/decodableReader/decodableReaderTool"; +import { LeveledReaderTool } from "./readers/leveledReader/leveledReaderTool"; +import { MusicToolAdaptor } from "./music/musicToolControls"; +import { ImpairmentVisualizerAdaptor } from "./impairmentVisualizer/impairmentVisualizer"; +import { MotionTool } from "./motion/motionTool"; +import TalkingBookTool from "./talkingBook/talkingBookTool"; +import { SignLanguageTool } from "./signLanguage/signLanguageTool"; +import { ImageDescriptionAdapter } from "./imageDescription/imageDescription"; +import { CanvasTool } from "./canvas/canvasTool"; +import { GameTool } from "./games/GameTool"; +import { SettingsTool } from "./settings/settingsTool"; + +/** + * Make the one instance of each toolbox class and register it with the master toolbox. The + * imports above also serve to ensure that each tool's code is part of the bundle. + * + * Calling this twice registers nothing the second time. ToolBox.registerTool is a bare push with + * no check for duplicates, and the guard keys off the shared master list rather than a flag in + * this module: a caller that is a React-Refresh boundary re-executes its own module during + * `pnpm dev` while masterToolList, which lives in toolbox.ts, keeps its entries. A flag here + * would reset and we would get eleven duplicate tools and duplicate accordion sections. + */ +export function registerAllToolboxTools(): void { + if (getMasterToolList().length > 0) return; + ToolBox.registerTool(new DecodableReaderTool()); + ToolBox.registerTool(new LeveledReaderTool()); + ToolBox.registerTool(new MusicToolAdaptor()); + ToolBox.registerTool(new ImpairmentVisualizerAdaptor()); + ToolBox.registerTool(new MotionTool()); + ToolBox.registerTool(new TalkingBookTool()); + ToolBox.registerTool(new SignLanguageTool()); + ToolBox.registerTool(new ImageDescriptionAdapter()); + ToolBox.registerTool(new CanvasTool()); + ToolBox.registerTool(new GameTool()); + ToolBox.registerTool(new SettingsTool()); +} diff --git a/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts b/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts index d7db8cc854c8..71d36a22eaed 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/toolboxBootstrap.ts @@ -10,18 +10,10 @@ import { import { simulateBlurOnPageFrameMouseDown } from "../../utils/menuCloseOnBlur"; import { getTheOneReaderToolsModel } from "./readers/readerToolsModel"; import { ToolBox } from "./toolbox"; -import { DecodableReaderTool } from "./readers/decodableReader/decodableReaderTool"; -import { LeveledReaderTool } from "./readers/leveledReader/leveledReaderTool"; -import { MusicToolAdaptor } from "./music/musicToolControls"; -import { ImpairmentVisualizerAdaptor } from "./impairmentVisualizer/impairmentVisualizer"; -import { MotionTool } from "./motion/motionTool"; import TalkingBookTool from "./talkingBook/talkingBookTool"; -import { SignLanguageTool } from "./signLanguage/signLanguageTool"; -import { ImageDescriptionAdapter } from "./imageDescription/imageDescription"; import "errorHandler"; -import { CanvasTool } from "./canvas/canvasTool"; -import { GameTool, setActiveDragActivityTab } from "./games/GameTool"; -import { SettingsTool } from "./settings/settingsTool"; +import { setActiveDragActivityTab } from "./games/GameTool"; +import { registerAllToolboxTools } from "./registerAllToolboxTools"; // Explicit imports needed so that these symbols are in local scope for the window.toolboxBundle object import { addWordListChangedListener, @@ -120,20 +112,10 @@ $(document).ready(() => { getTheOneToolbox().initialize(); }); -// Make the one instance of each Toolbox class and register it with the master toolbox. -// The imports we need to make these calls possible also serve to ensure that each -// toolbox's code is made part of the bundle. -ToolBox.registerTool(new DecodableReaderTool()); -ToolBox.registerTool(new LeveledReaderTool()); -ToolBox.registerTool(new MusicToolAdaptor()); -ToolBox.registerTool(new ImpairmentVisualizerAdaptor()); -ToolBox.registerTool(new MotionTool()); -ToolBox.registerTool(new TalkingBookTool()); -ToolBox.registerTool(new SignLanguageTool()); -ToolBox.registerTool(new ImageDescriptionAdapter()); -ToolBox.registerTool(new CanvasTool()); -ToolBox.registerTool(new GameTool()); -ToolBox.registerTool(new SettingsTool()); +// Make the one instance of each Toolbox class and register it with the master toolbox. The list +// lives in registerAllToolboxTools.ts, which the test harness imports as well, so there is only +// one list to keep right. +registerAllToolboxTools(); const toolboxBundle: ToolboxBundleApi = { getTheOneToolbox, diff --git a/src/BloomBrowserUI/react_components/LinkTargetChooser/component-tests/error-handling.uitest.ts b/src/BloomBrowserUI/react_components/LinkTargetChooser/component-tests/error-handling.uitest.ts index 0661561e390f..be7bea07c53d 100644 --- a/src/BloomBrowserUI/react_components/LinkTargetChooser/component-tests/error-handling.uitest.ts +++ b/src/BloomBrowserUI/react_components/LinkTargetChooser/component-tests/error-handling.uitest.ts @@ -7,6 +7,12 @@ import { test, expect } from "../../component-tester/playwrightTest"; import { setupLinkTargetChooser } from "./test-helpers"; +// How long to let the error message appear or clear. It was 1000ms, which was not enough on a +// loaded machine: two of these tests failed about once per full-suite run and passed when run on +// their own. These are waits for a state rather than sleeps, so the larger number costs a passing +// run nothing. +const kErrorAppearsTimeoutMs = 10000; + test.describe("LinkTargetChooser - Error Handling for Missing Books/Pages", () => { test("Shows error when URL points to missing book", async ({ page }) => { const context = await setupLinkTargetChooser(page, { @@ -17,7 +23,10 @@ test.describe("LinkTargetChooser - Error Handling for Missing Books/Pages", () = // Wait for the error message to appear const errorMsgElement = await context.errorDisplay.getErrorMessage(); - await errorMsgElement.waitFor({ state: "visible", timeout: 1000 }); + await errorMsgElement.waitFor({ + state: "visible", + timeout: kErrorAppearsTimeoutMs, + }); // Error message should be visible const isErrorVisible = await context.errorDisplay.isVisible(); @@ -42,7 +51,10 @@ test.describe("LinkTargetChooser - Error Handling for Missing Books/Pages", () = // Wait for the error message to appear const errorMsgElement = await context.errorDisplay.getErrorMessage(); - await errorMsgElement.waitFor({ state: "visible", timeout: 1000 }); + await errorMsgElement.waitFor({ + state: "visible", + timeout: kErrorAppearsTimeoutMs, + }); // Error message should be visible const isErrorVisible = await context.errorDisplay.isVisible(); @@ -114,7 +126,10 @@ test.describe("LinkTargetChooser - Error Handling for Missing Books/Pages", () = // Wait for the error message to appear const errorMsgElement = await context.errorDisplay.getErrorMessage(); - await errorMsgElement.waitFor({ state: "visible", timeout: 1000 }); + await errorMsgElement.waitFor({ + state: "visible", + timeout: kErrorAppearsTimeoutMs, + }); // Verify error appears let isErrorVisible = await context.errorDisplay.isVisible(); @@ -124,7 +139,10 @@ test.describe("LinkTargetChooser - Error Handling for Missing Books/Pages", () = await context.urlEditor.setValue("/book/book1"); // Wait for error to disappear - await errorMsgElement.waitFor({ state: "hidden", timeout: 1000 }); + await errorMsgElement.waitFor({ + state: "hidden", + timeout: kErrorAppearsTimeoutMs, + }); isErrorVisible = await context.errorDisplay.isVisible(); expect(isErrorVisible).toBe(false); diff --git a/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/ToolboxRootTestHarness.tsx b/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/ToolboxRootTestHarness.tsx index a9ce390547f8..53347d60f2f5 100644 --- a/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/ToolboxRootTestHarness.tsx +++ b/src/BloomBrowserUI/react_components/ToolboxRootTestHarness/ToolboxRootTestHarness.tsx @@ -1,47 +1,15 @@ import * as React from "react"; import { ToolboxRoot } from "../../bookEdit/toolbox/ToolboxRoot"; -import { ToolBox, getMasterToolList } from "../../bookEdit/toolbox/toolbox"; -import { DecodableReaderTool } from "../../bookEdit/toolbox/readers/decodableReader/decodableReaderTool"; -import { LeveledReaderTool } from "../../bookEdit/toolbox/readers/leveledReader/leveledReaderTool"; -import { MusicToolAdaptor } from "../../bookEdit/toolbox/music/musicToolControls"; -import { ImpairmentVisualizerAdaptor } from "../../bookEdit/toolbox/impairmentVisualizer/impairmentVisualizer"; -import { MotionTool } from "../../bookEdit/toolbox/motion/motionTool"; -import TalkingBookTool from "../../bookEdit/toolbox/talkingBook/talkingBookTool"; -import { SignLanguageTool } from "../../bookEdit/toolbox/signLanguage/signLanguageTool"; -import { ImageDescriptionAdapter } from "../../bookEdit/toolbox/imageDescription/imageDescription"; -import { CanvasTool } from "../../bookEdit/toolbox/canvas/canvasTool"; -import { GameTool } from "../../bookEdit/toolbox/games/GameTool"; -import { SettingsTool } from "../../bookEdit/toolbox/settings/settingsTool"; +import { registerAllToolboxTools } from "../../bookEdit/toolbox/registerAllToolboxTools"; // ToolboxRoot only renders a section for a tool that is in the master tool list, and tools -// put themselves there by being registered. In the running app that happens as a side effect -// of loading toolboxBootstrap. We deliberately do NOT import that module here: besides -// registering tools it also renders its own toolbox root on $(document).ready and assigns -// window.toolboxBundle, which would both duplicate the root this harness renders and -// overwrite the toolboxBundle stub some tests install. So we register the same set of tools -// ourselves. Keep this list in sync with toolboxBootstrap.ts. -// The guard keys off the shared master list rather than a module-local flag on purpose. -// This file is a valid React-Refresh boundary (its only export is a component), so editing -// it during `pnpm dev` re-executes this module without reloading the page. A module-local -// flag would reset to false while masterToolList — which lives in toolbox.ts and is not -// invalidated — kept its entries, and ToolBox.registerTool is a bare push with no dedupe, -// so we would end up with 11 duplicate tools and duplicate accordion sections. -function registerToolsOnce() { - if (getMasterToolList().length > 0) return; - ToolBox.registerTool(new DecodableReaderTool()); - ToolBox.registerTool(new LeveledReaderTool()); - ToolBox.registerTool(new MusicToolAdaptor()); - ToolBox.registerTool(new ImpairmentVisualizerAdaptor()); - ToolBox.registerTool(new MotionTool()); - ToolBox.registerTool(new TalkingBookTool()); - ToolBox.registerTool(new SignLanguageTool()); - ToolBox.registerTool(new ImageDescriptionAdapter()); - ToolBox.registerTool(new CanvasTool()); - ToolBox.registerTool(new GameTool()); - ToolBox.registerTool(new SettingsTool()); -} - -registerToolsOnce(); +// put themselves there by being registered. In the running app that happens when +// toolboxBootstrap.ts calls registerAllToolboxTools. We deliberately do NOT import +// toolboxBootstrap here: besides registering tools it also renders its own toolbox root on +// $(document).ready and assigns window.toolboxBundle, which would both duplicate the root this +// harness renders and overwrite the toolboxBundle stub some tests install. So we call the shared +// registration function, which is side-effect-free to import and safe to call twice. +registerAllToolboxTools(); export const ToolboxRootTestHarness: React.FunctionComponent = () => { return ; diff --git a/src/BloomBrowserUI/react_components/TopBar/CollectionTopBarControls/component-tests/bloom-exe-collection-topbar.uitest.ts b/src/BloomBrowserUI/react_components/TopBar/CollectionTopBarControls/component-tests/bloom-exe-collection-topbar.uitest.ts index 3d955de146cf..bf34d6f1ce96 100644 --- a/src/BloomBrowserUI/react_components/TopBar/CollectionTopBarControls/component-tests/bloom-exe-collection-topbar.uitest.ts +++ b/src/BloomBrowserUI/react_components/TopBar/CollectionTopBarControls/component-tests/bloom-exe-collection-topbar.uitest.ts @@ -10,7 +10,7 @@ test.describe("CollectionTopBarControls on Bloom.exe", () => { const connection = await connectToBloomExe(); try { - await clickWorkspaceTab(connection.page, "Collections"); + await clickWorkspaceTab(connection.page, "collection"); await waitForActiveWorkspaceTab("collection"); await expect( diff --git a/src/BloomBrowserUI/react_components/TopBar/TopBar.tsx b/src/BloomBrowserUI/react_components/TopBar/TopBar.tsx index 6fea416fba82..6ca83547343e 100644 --- a/src/BloomBrowserUI/react_components/TopBar/TopBar.tsx +++ b/src/BloomBrowserUI/react_components/TopBar/TopBar.tsx @@ -130,6 +130,10 @@ export const TopBar: React.FunctionComponent = () => {
{ const connection = await connectToBloomExe(); try { - await clickWorkspaceTab(connection.page, "Collections"); + await clickWorkspaceTab(connection.page, "collection"); await waitForActiveWorkspaceTab("collection"); await expect(connection.page.locator("body")).toHaveClass( /collection-mode/, ); - await clickWorkspaceTab(connection.page, "Publish"); + await clickWorkspaceTab(connection.page, "publish"); await waitForActiveWorkspaceTab("publish"); await expect(connection.page.locator("body")).toHaveClass( /publish-mode/, ); - await clickWorkspaceTab(connection.page, "Edit"); + await clickWorkspaceTab(connection.page, "edit"); await waitForActiveWorkspaceTab("edit"); await expect(connection.page.locator("body")).toHaveClass( /edit-mode/, @@ -56,7 +56,7 @@ test.describe("Bloom exe CDP top bar", () => { ) .toBe(true); - await clickWorkspaceTab(connection.page, "Publish"); + await clickWorkspaceTab(connection.page, "publish"); await waitForActiveWorkspaceTab("publish"); await expect @@ -67,7 +67,7 @@ test.describe("Bloom exe CDP top bar", () => { ) .toBe(true); - await clickWorkspaceTab(connection.page, "Edit"); + await clickWorkspaceTab(connection.page, "edit"); await waitForActiveWorkspaceTab("edit"); } finally { await connection.browser.close(); diff --git a/src/BloomBrowserUI/react_components/component-tester/bloomExeCdp.ts b/src/BloomBrowserUI/react_components/component-tester/bloomExeCdp.ts index beac61f2d2f6..3efa65c53db8 100644 --- a/src/BloomBrowserUI/react_components/component-tester/bloomExeCdp.ts +++ b/src/BloomBrowserUI/react_components/component-tester/bloomExeCdp.ts @@ -70,23 +70,20 @@ export const connectToBloomExe = async (): Promise<{ return { browser, page }; }; +/** + * Click a workspace tab in the real top bar. The tab is found by the test id that + * react_components/TopBar/TopBar.tsx puts on it, so this works in any UI language. + * + * Bloom hides the Edit and Publish tabs until a book is selected, so a caller that wants either + * of them must select a book first. + */ export const clickWorkspaceTab = async ( page: Page, - name: WorkspaceTabId extends infer _T - ? "Collections" | "Edit" | "Publish" - : never, + tab: WorkspaceTabId, ): Promise => { - await page.waitForSelector("#main-tabs button", { - timeout: 10000, - }); - - await page.locator("#main-tabs button").filter({ hasText: name }).first(); - - await page - .locator("#main-tabs button") - .filter({ hasText: name }) - .first() - .click(); + const target = page.getByTestId(`workspace-tab-${tab}`); + await target.waitFor({ state: "visible", timeout: 10000 }); + await target.click(); }; export const getWorkspaceTabs = async (): Promise<{ diff --git a/src/BloomBrowserUI/react_components/registration/component-tests/test-helpers.ts b/src/BloomBrowserUI/react_components/registration/component-tests/test-helpers.ts index 70d70241959f..2ca871ca3f03 100644 --- a/src/BloomBrowserUI/react_components/registration/component-tests/test-helpers.ts +++ b/src/BloomBrowserUI/react_components/registration/component-tests/test-helpers.ts @@ -11,7 +11,12 @@ import { // Test timing constants export const kTestOptOutDelaySeconds = 2; -export const kTestOptOutTimeoutMs = kTestOptOutDelaySeconds * 1000 + 2000; // delay + buffer +// The delay the component waits before offering the opt-out button, plus a buffer for the machine +// running the suite. The buffer was 2000ms, which was not enough on a loaded machine: the last +// worker to start would miss the button by a fraction of a second, so one of these tests failed +// about once per full-suite run and passed when run on its own. This is a wait for a state, not a +// sleep, so a longer buffer costs a passing run nothing. +export const kTestOptOutTimeoutMs = kTestOptOutDelaySeconds * 1000 + 8000; // Field helper type for registration form type FieldHelper = { diff --git a/src/BloomBrowserUI/utils/bloomApi.ts b/src/BloomBrowserUI/utils/bloomApi.ts index 82ec01847c26..081ef14c56f4 100644 --- a/src/BloomBrowserUI/utils/bloomApi.ts +++ b/src/BloomBrowserUI/utils/bloomApi.ts @@ -580,7 +580,14 @@ export async function getWithConfigAsync( return wrapAxios(axios.get(getBloomApiPrefix() + urlSuffix, config)); } -export function postString(urlSuffix: string, value: string) { +// Pass report: false for an endpoint whose failure is not worth a problem report, in the same +// spirit as postThatMightNavigate: a request that asks Bloom to do something it may decline for +// reasons of timing, where the user has nothing to fix and nothing to be told. +export function postString( + urlSuffix: string, + value: string, + report: boolean = true, +) { // Match post(): unit tests should not hit Bloom backend endpoints. const isTest = typeof process !== "undefined" && process.env.NODE_ENV === "test"; @@ -594,6 +601,7 @@ export function postString(urlSuffix: string, value: string) { "Content-Type": "text/plain", }, }), + report, ); } diff --git a/src/BloomE2E/AUTOMATION-DEBT.md b/src/BloomE2E/AUTOMATION-DEBT.md index 3ffc24a1189f..95de29bf21f2 100644 --- a/src/BloomE2E/AUTOMATION-DEBT.md +++ b/src/BloomE2E/AUTOMATION-DEBT.md @@ -26,14 +26,13 @@ force-foreground trick. Fix direction: move these surfaces to the web UI (the te direction anyway), or expose each dialog's WebView2 on a discoverable CDP port. (Promoted from PAPERCUTS 2026-07-11.) -seen again 2026-09-01 (Test Case ID 169, `publish-text-languages.spec.ts`): the case -turns on which languages the collection has, and there is no API for that either. -`collectionSettings/changeLanguage` is not one: its only listener is the open WinForms -`CollectionSettingsDialog` (`CollectionSettingsDialog.cs:368`), so a POST to it while -the dialog is closed does nothing. The test therefore changes a collection language by -stopping Bloom, rewriting the `.bloomCollection`, and starting again, which is what the -new `bloomApp.restart(betweenStopAndStart)` fixture method is for. Each restart costs -about six seconds and loses whatever the editor had not yet saved. +seen again 2026-09-01, and the collection-languages half of it is now fixed: tests set +the collection's languages through the `e2e/setCollectionLanguages` hook, which does +the same work as clicking OK in the Collection Settings dialog, and +`helpers/collection.ts setCollectionLanguages` wraps it with the restart Bloom still +needs (about six seconds, and it loses whatever the editor had not yet saved). No test +composes `.bloomCollection` XML any more. What remains is the dialog itself: nothing +can drive or screenshot the Settings UI, so the journey test for it cannot be written. ## Native OS dialogs hang automation @@ -43,50 +42,56 @@ cannot dismiss; a test that triggers one hangs the run. Tests must avoid them (t `E2eTestingApi` for the common cases (choose image file, choose video), so journeys that need them become automatable. -## Visual-regression cases stop at the first failed comparison - -Each case in `src/BloomVisualRegressionTests/index.spec.ts` throws on the first -mismatch, so later comparisons never capture their images; stale baselines surface one -layer per ~3-minute run (BL-16638 took three accept-and-rerun rounds). Fix direction: -accumulate per-comparison failures and fail once at the end — proven during BL-16638 -(~20–30 lines, confined to the spec). Loop at `index.spec.ts:426`, assertion at -`index.spec.ts:486` (pre-rewire line numbers). (Promoted from PAPERCUTS 2026-07-30.) - -## The top bar has no stable test ids, so tests match on localized text - -`TopBar.tsx` renders the workspace tabs as `` with a localized `` -label and no id, class, or `data-testid`. Two costs, both already paid: the -component-tester's `bloomExeCdp.ts` drives `#main-tabs button`, a selector that exists -nowhere in the source, so `bloom-exe-tabs.uitest.ts` cannot have worked for some time -(it needs a developer's Bloom already running, and nothing runs it in CI — see the entry -below); and `src/BloomE2E/helpers/workspace.ts` has to map tab ids to the English labels -"Collections"/"Edit"/"Publish", so the suite silently only works in an English UI — -which rules out automating the UI-language cases. The same gap makes the fixture -identify Bloom's shell document among the CDP page targets by `[role="tablist"]`, the -only stable marker available. Fix direction: `data-testid="workspace-tab-collection"` -(etc.) on each tab and one on the shell root, and drop the label matching. -(Found 2026-09-01 while scaffolding src/BloomE2E.) - -## The component-tester Playwright suites are not in CI - -`nightly.yml` runs vitest, C#, and visual-regression only; nothing runs -`react_components/component-tester`'s suites, which is how the harness sat broken -(React 17 pin + config bug) unnoticed until it was green again at 144 passed. It will -rot again silently. Fix direction: a nightly job mirroring the visual-regression one -(component config only; the bloom-exe config needs the e2e launch fixture first). -(Promoted from PAPERCUTS 2026-07-27.) - -## Toolbox tool registration is a side effect of toolboxBootstrap - -`ToolboxRoot` only renders tools registered via importing `toolboxBootstrap.ts`, which -also renders and clobbers globals, so the test harness duplicates the 11 -`ToolBox.registerTool(...)` calls with a "keep in sync" comment. Fix direction: extract -a side-effect-free `registerAllToolboxTools()` both import — probably folded into the -toolbox React refactor (BL-16608 / PR #8109). Related: one harness test is `test.fixme` -because it asserts on `.subscription-badge` (legacy-toolbox-only) and -`.toolbox-react-header-icon` (never existed); re-enabling it needs a decision on whether -the React header renders badges/icons and what classes to expose. -(Promoted from PAPERCUTS 2026-07-27.) +## Which front end the e2e suite tests depends on what else is running + +A launched Bloom serves its React front end either from the built `output/browser` or from a Vite +dev server, and until the fixture is told which, the answer depends on the machine. Three facts, +established 2026-09-01: + +- **There is no way to point Bloom at another folder.** `BloomFileLocator.BrowserRoot` computes + `output/browser` (or `browser`) from where the app sits, with no environment variable and no + command-line option, so the isolated bundle that `build/agent-vite.ps1` writes under + `output/agent//browser` cannot be used by a launched Bloom. +- **A dev server is the supported route, and the fixture now takes it.** Set + `BLOOM_E2E_VITE_PORT=` and `fixtures/launchBloom.ts` passes `--vite-port `, so the suite + tests the working tree with no build at all. Start the server with `PORT` set as well as + `--port`: the port in `vite.config.mts` comes from `process.env.PORT`, so `--port` alone moves + the server but leaves its HMR and React-Refresh URLs pointing at 5173, and the page then fails + to load its entry module. +- **Leaving the variable unset does not mean "no dev server".** A dev build probes port 5173 by + itself (`ReactControl.TryGetActiveViteDevPort`), so a developer's own dev server silently + decides what the suite tests, and Bloom offers no option that means "ignore any dev server" + (`--vite-port` rejects 0, and `ValidateStartupVitePort` requires the port to answer). + +What remains: the fixture neither starts a dev server of its own nor checks that `output/browser` +is newer than `src/BloomBrowserUI`, so a run with the variable unset can still test a stale +bundle without saying so. Fix direction: have the fixture own the choice, either by starting a +dev server on a port of its own choosing, or by refusing to run against an `output/browser` older +than the source and naming the file that is newer. Bloom needs an explicit "no dev server" +option before the second half of that can be trusted. +(Found 2026-09-01 while fixing the top-bar test ids.) + +## One test's tab is the next test's starting state + +`fixtures/bloomTest.ts` launches one Bloom per worker, and Playwright gives every test with the +same fixture options that same worker. So a test that ends on the Edit tab makes the next one +start there, and `tests/workspace-tabs.spec.ts` fails its opening sanity check with +`collection: "enabled"` rather than for any reason to do with tabs. `tests/capture-book-page.spec.ts` +switches back to the collection tab at its end to avoid exactly this, which is a convention no +helper enforces and nothing reminds a new test about. Fix direction: reset the workspace in the +fixture's per-test setup, so the tab a test starts on is not a matter of file order. +(Found 2026-09-01, when adding capture-book-page.spec.ts broke workspace-tabs.spec.ts.) + +## One toolbox harness test asserts on classes that do not exist + +`react_components/ToolboxRootTestHarness`'s suite has one `test.fixme` because it asserts on +`.subscription-badge` (which only the legacy toolbox has) and `.toolbox-react-header-icon` (which +never existed). Re-enabling it needs a decision on whether the React toolbox header renders +badges and icons at all, and what classes to expose for them. Fix direction: make that decision +as part of the toolbox React refactor (BL-16608 / PR #8109), then rewrite the assertions against +what the header really renders. +(Was part of a larger entry about toolbox registration, whose other half was fixed 2026-09-01 by +extracting `bookEdit/toolbox/registerAllToolboxTools.ts`.) ## AI-image-editor selectors are an untested cross-repo contract @@ -98,21 +103,19 @@ did NOT drift. Fix direction: stable `data-testid`s on tool tiles and category h in the editor repo, or have it publish its host-harness selectors for import. (Promoted from PAPERCUTS 2026-07-30, BL-16603.) -## Driver-level CDP footguns that the automation library must absorb +## Driver-level CDP footguns the helper layer does not cover yet -Known WebView2/CDP behaviors that every ad-hoc script rediscovers the hard way; the -`src/BloomE2E` helper layer should encode them once: +Known WebView2/CDP behaviors that every ad-hoc script rediscovers the hard way. The screenshot +one is now absorbed by `helpers/screenshot.ts` (enlarge the window, clip, clear the override, +and time out every CDP request); these two are not, because they are about the scripts around a +capture rather than the capture itself: -- `Page.captureScreenshot` with `captureBeyondViewport:true` hangs (no response, no - error). Working pattern: `Emulation.setDeviceMetricsOverride` large enough for the - whole `.bloom-page`, screenshot with a `clip`, then `clearDeviceMetricsOverride`; - give every CDP request a timeout. - Never `taskkill //IM node.exe //F` to clean up a hung capture — it kills the go.sh vite/dotnet-watch flow and takes Bloom's server down. Kill only the script's own PID. - Reopening a book re-stamps it with freshly compiled xmatter CSS from `output/`, so "before" captures taken after a restart already show the new layout. -(Promoted from PAPERCUTS 2026-07-22.) +(Promoted from PAPERCUTS 2026-07-22; the screenshot item removed 2026-09-01.) ## Visual-regression baselines only match the CI runner @@ -126,15 +129,6 @@ authoring painful. Fix direction: a small per-comparison pixel tolerance, or machine-profile baselines, or render fonts only from Bloom's own WOFF2 set in --e2e mode. (Found 2026-09-01 while verifying the bloom-testing-inputs rewire.) -## Automation helper scripts run destructive defaults on unknown flags - -`node .github/skills/bloom-automation/killBloomProcess.mjs --help` killed the running -Bloom: unknown flags are ignored and the destructive default runs, so "read the usage -first" is itself the dangerous move; sibling scripts may share the shape. Fix -direction: recognize `--help`/`-h` and reject unknown flags in every script that kills -processes — and fold these helpers' jobs into the library's audited launch/teardown -fixture over time. (Promoted from PAPERCUTS 2026-07-24.) - ## Adding a page needs the Add Page dialog, which offers nothing to automate against A book made from a template starts with front and back matter only, because every page @@ -149,25 +143,45 @@ production `addPage` endpoint. Fix direction: if the page chooser ever gets its from C# instead of from the HTML, retire the hook and read that list. (Found 2026-09-01 automating Test Case ID 169.) -## The Edit tab silently drops a jump to a page while it is loading - -`editView/jumpToPage` is the only way to move a test to a particular page, and it is -also how a test saves what it typed, because Bloom writes a page only when the book -leaves it. Coming back from the Publish tab, the Edit tab accepts the POST, replies -success, and shows nothing: the page iframe stays empty until the test asks again. So -`helpers/bookMaking.ts` asks up to three times. Two costs: a test that jumps at the -wrong moment waits 20 seconds per attempt, and a real "this page will not load" bug -would look like the same flake. Fix direction: have `jumpToPage` queue the request -until the Edit tab is ready, or report that it refused it. -(Found 2026-09-01 automating Test Case ID 169.) - ## Filling a text box directly leaves part of the old text behind A `.bloom-editable` is a CKEditor surface, and Playwright's `fill()` on one leaves a tail of what was there ("Deux" became "eux"), so `typeInGroup` clicks in, selects all, -deletes, and types the new text one key at a time. That is closer to what a person does -and it is reliable, but it is also slow for anything longer than a few words, and no -test can currently clear a box by any faster route. Fix direction: understand what -CKEditor does with a programmatic value change; a supported "set the text of this box" -path would let long text be set at once. +deletes, and then puts the new text in. + +Partly fixed 2026-09-01: the typing half is no longer a key press per character. +`typeInGroup` now inserts the whole string in one call (`keyboard.insertText`), which +CKEditor and Bloom's markup code both handle through the input event it raises, so the +cost of typing no longer grows with the length of the text. What remains is clearing a +box: that still needs a click, Control+A and Delete, because neither `fill()` nor +setting the value leaves CKEditor in a state Bloom then saves correctly. Fix direction: +an `e2e/` hook, or a supported CKEditor path, that sets the text of one box outright. (Found 2026-09-01 automating Test Case ID 169.) + +## A test can attach to a shell document Bloom does not drive + +More than one document in a run carries the workspace root's markup, and therefore the +top bar's `data-testid`, so `fixtures/bloomTest.ts findShellPage` returns whichever the +debugging protocol lists first. When that is not the document Bloom drives, the test is +silently broken rather than failing: its own clicking and typing work, `expect` on what +it typed passes, and every page Bloom loads goes into the document it cannot see. The +symptom is a 60-second wait in `goToPage` for a page Bloom's own log says it showed. +This is why `publish-text-languages.spec.ts` fails perhaps one run in three. + +Fixed for tests, 2026-09-01. `e2e/shellUrl` reports the URL of the document Bloom drives, +and `findShellPage` now takes the page whose URL has the same file name (Bloom and the +debugging protocol escape the rest of the URL differently), re-resolving after +`bloomApp.restart`. It falls back to the first page carrying the marker only when the +endpoint never answers, which is what an old `Bloom.exe` in `output/Debug` does, and says +so. `goToPage`'s failure message names both URLs. Also, under `--e2e` every browser built +on the UI thread shares one CoreWebView2Environment, so those documents live in one +browser process with one debugging listener; before that, each environment was given the +same port number and only the first process to start could listen on it. That sharing is +deliberately limited to the UI thread: an environment belongs to the thread that created +it, and handing it to a browser built on the thread serving an API call hangs that thread. +Publishing a BloomPUB does exactly that, and its preview never appeared. + +What remains: nobody knows why a run has a second workspace root document at all. Bloom +creates one `_workspaceReactControl`. Worth finding, because the duplicate is what makes +the test-side check necessary. +(Found 2026-09-01 while making `jumpToPage` queue a jump.) diff --git a/src/BloomE2E/README.md b/src/BloomE2E/README.md index 7d9789b61559..82c2ff9d2ffd 100644 --- a/src/BloomE2E/README.md +++ b/src/BloomE2E/README.md @@ -58,12 +58,11 @@ and whatever tab is showing. Most tests need nothing else. | `restart` | Stop Bloom, run an optional callback, start it again on the same collection, and return the new page. | `restart(betweenStopAndStart)` is how a test changes something Bloom only reads at startup. The -collection's languages are the case that needed it: the collection Settings dialog is a WinForms -surface CDP cannot reach, and `collectionSettings/changeLanguage` only answers while that dialog -is open, so the way to change a language is to stop Bloom, rewrite the `.bloomCollection` with -`makeCollectionXml`, and start again. Bloom is killed rather than asked to quit, so leave the -page being edited before restarting or what was typed on it is lost. Use the page `restart` -returns; the old one is closed. +collection's languages are the case that needed it, and a test does not call `restart` itself for +that: `setCollectionLanguages(bloomApp, tags)` posts the new languages to the `e2e/` hook that +writes the `.bloomCollection`, then restarts Bloom and returns the new page. Bloom is killed +rather than asked to quit, so leave the page being edited before restarting or what was typed on +it is lost. Use the page `restart` returns; the old one is closed. Teardown kills the process tree, waits for the HTTP port to go dark, and deletes the temp copy. @@ -89,13 +88,17 @@ real bug in the code under test; read the message and fix it rather than working - `helpers/workspace.ts` — `switchTab`, `getTabs`, `waitForActiveTab`. Note that Bloom hides the Edit and Publish tabs until a book is selected. -- `helpers/collection.ts` — `selectBook`, `waitForCollectionReady`. +- `helpers/collection.ts` — `selectBook`, `waitForCollectionReady`, `setCollectionLanguages`. - `helpers/api.ts` — `apiGet`, `apiPost`, `apiGetJson`. These run `fetch` inside the page with a relative URL, which is not a style choice: Bloom's server rejects a `127.0.0.1` Host header, and the CDP endpoint does not answer on `localhost`. The file explains it. - `helpers/realClick.ts` — `realClick`, `realClickAt`. Book tiles, Settings, and PREVIEW ignore a synthetic `element.click()`. Never hand-roll `Input.dispatchMouseEvent` in a test; add the gesture here. +- `helpers/screenshot.ts` — `captureCurrentBookPage`, `captureElement`, `readPngSize`. Captures an + element taller than the window. `Page.captureScreenshot` with `captureBeyondViewport` hangs in + WebView2, so this enlarges the window, clips, clears the override, and times out every CDP + request. Never open a CDP session in a test; add the capture here. Two things a test must never do: trigger a native OS dialog (file pickers, the WinForms Image Toolbox, video capture), because Playwright cannot dismiss one and the run hangs; and wait on a @@ -126,7 +129,18 @@ pnpm exec playwright test -g "switching workspace tabs" # one test by titl pnpm exec playwright test --debug # step through it ``` -A run opens a real Bloom window. That is expected; do not click in it. +A run opens a real Bloom, but you will not see it: Bloom is launched with `--headless`, which puts +its window far outside every monitor, so a run can go on while you work. The window is moved +off-screen rather than minimized or hidden because WebView2 stops painting a minimized window, +which would make every screenshot blank. + +To watch the run instead, set `BLOOM_E2E_HEADED=1`; `--debug` turns it on for you. + +```bash +BLOOM_E2E_HEADED=1 pnpm exec playwright test tests/workspace-tabs.spec.ts +``` + +A headed run opens a real Bloom window. That is expected; do not click in it. The suite needs a built `Bloom.exe` under `output/{Debug,Release}/{x64,AnyCPU,}/` and the test inputs at `output/testing-inputs`, fetched by `node build/get-testing-inputs.mjs` at the commit @@ -143,3 +157,24 @@ BLOOM_TESTING_INPUTS_DIR=D:/bloom-testing-inputs pnpm test Either way the fixture copies the collection before Bloom opens it, so a run never modifies your inputs. + +## Testing a front-end change + +The launched Bloom serves its React UI from the built `output/browser`, so an edit to a `.tsx` +file does not reach a run until somebody rebuilds that bundle. To test the working tree instead, +start a Vite dev server and name its port in `BLOOM_E2E_VITE_PORT`; the fixture then passes +`--vite-port` to Bloom, which loads every React control from the dev server. + +```bash +# In one terminal, in src/BloomBrowserUI. Set PORT as well as --port: the port in +# vite.config.mts comes from process.env.PORT, and --port alone leaves the HMR and +# React-Refresh URLs pointing at 5173, which makes the page fail to load its entry module. +PORT=5199 pnpm exec vite --port 5199 --strictPort + +# In another, in src/BloomE2E +BLOOM_E2E_VITE_PORT=5199 pnpm exec playwright test +``` + +Pick a port other than 5173 if a Bloom is already running against a dev server there. Leaving the +variable unset does NOT mean "no dev server": a dev build of Bloom looks for one on 5173 by +itself, so what a run tests can depend on what else is running. See AUTOMATION-DEBT.md. diff --git a/src/BloomE2E/fixtures/bloomTest.ts b/src/BloomE2E/fixtures/bloomTest.ts index c0fef719f10d..655833af8016 100644 --- a/src/BloomE2E/fixtures/bloomTest.ts +++ b/src/BloomE2E/fixtures/bloomTest.ts @@ -86,8 +86,27 @@ const SHELL_READY_TIMEOUT_MS = 90000; const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); -// How we recognize Bloom's shell document: it is the page holding the top bar's tab strip. -const SHELL_MARKER = '[role="tablist"]'; +// How we recognize a candidate for Bloom's shell document: it is a page holding the top bar, which +// carries this test id (react_components/TopBar/TopBar.tsx). +const SHELL_MARKER = '[data-testid="workspace-top-bar"]'; + +// Bloom's own answer to "which document are you driving?". The endpoint exists only under --e2e, +// and reports the URL of the shell browser the C# side sends its commands to +// (E2eTestingApi.HandleGetShellUrl). +const SHELL_URL_ENDPOINT = "e2e/shellUrl"; + +/** + * The file name part of a shell URL, e.g. "bloom45mgnfsl.htm" from + * "http://localhost:8095/bloom/C$3A/.../bloom45mgnfsl.htm?x=1". Bloom names each shell document + * after a temp file, so the file name identifies the document while the query string does not: + * the workspace rewrites its own query as the user moves around (updateWorkspaceUrlParam). + */ +function shellDocumentName(url: string): string { + const withoutQuery = url.split(/[?#]/)[0]; + return withoutQuery + .substring(withoutQuery.lastIndexOf("/") + 1) + .toLowerCase(); +} /** * Connect to the WebView2's CDP endpoint, retrying while it comes up. Bloom's HTTP API reports @@ -113,34 +132,78 @@ async function connectOverCdpWithRetry(cdpPort: number): Promise { } /** - * Find Bloom's shell document among the CDP page targets. We identify it by the top bar's tab - * strip rather than by URL, which also excludes the separately-hosted problem dialog and any - * DevTools target. Polling matters: the WebView2 target exists, as about:blank, for a second or - * two before Bloom navigates it to the shell, and the React top bar mounts later still. + * Find the shell document Bloom is actually driving, among the CDP page targets. + * + * Two tests are applied, and both are needed. The top bar's test id finds the candidates, which + * also excludes the separately-hosted problem dialog and any DevTools target; then Bloom itself is + * asked which document it drives, and only that one is returned. The marker alone is not enough: + * a run can expose more than one workspace-root document, and attaching to an undriven one costs + * an hour, because the test's own clicks work while nothing Bloom loads ever appears (see + * AUTOMATION-DEBT.md). The hook alone is not enough either: it answers "" until the workspace + * view has built its browser, and an older Bloom.exe in output/Debug does not have the endpoint at + * all, so the marker match stays as the fallback for a hook that never answers. + * + * Polling matters: the WebView2 target exists, as about:blank, for a second or two before Bloom + * navigates it to the shell, and the React top bar mounts later still. */ -async function findShellPage(browser: Browser): Promise { +async function findShellPage( + browser: Browser, + httpPort: number, +): Promise { const deadline = Date.now() + SHELL_READY_TIMEOUT_MS; let lastUrls: string[] = []; + let markerOnlyMatch: Page | undefined; + let hookEverAnswered = false; while (Date.now() < deadline) { const pages = browser .contexts() .flatMap((context) => context.pages()) - .filter((page) => !page.url().startsWith("devtools://")); + .filter((page) => !page.url().startsWith("devtools://")) + // Keep only this Bloom's own documents. Another Bloom (another worktree, or the + // developer's) can answer on this CDP port, and its pages carry the same marker and + // answer the same hook, so a page from a different HTTP port must not win. + .filter( + (page) => + !page.url().startsWith("http") || + page.url().includes(`:${httpPort}/`), + ); lastUrls = pages.map((page) => page.url()); for (const page of pages) { - const hasTabs = await page + const hasTopBar = await page .evaluate( (marker) => !!document.querySelector(marker), SHELL_MARKER, ) .catch(() => false); - if (hasTabs) return page; + if (!hasTopBar) continue; + if (!markerOnlyMatch) markerOnlyMatch = page; + const drivenUrl = await page + .evaluate(async (endpoint) => { + const response = await fetch(`/bloom/api/${endpoint}`); + return response.ok ? await response.text() : ""; + }, SHELL_URL_ENDPOINT) + .catch(() => ""); + if (!drivenUrl) continue; + hookEverAnswered = true; + if (shellDocumentName(drivenUrl) === shellDocumentName(page.url())) + return page; } await delay(500); } + if (markerOnlyMatch && !hookEverAnswered) { + console.warn( + `Bloom never answered ${SHELL_URL_ENDPOINT}, so the shell document was chosen by ` + + `${SHELL_MARKER} alone. If this test fails oddly, check that output/Debug holds a ` + + `Bloom.exe new enough to have that endpoint.`, + ); + return markerOnlyMatch; + } throw new Error( - `Bloom's WebView2 never exposed a page containing ${SHELL_MARKER} within ` + - `${SHELL_READY_TIMEOUT_MS / 1000}s. Targets seen: ${lastUrls.join(", ") || "none"}.`, + `Bloom's WebView2 never exposed the shell document it is driving within ` + + `${SHELL_READY_TIMEOUT_MS / 1000}s. Pages carrying ${SHELL_MARKER} were ` + + `${markerOnlyMatch ? "found" : "not found"}; ${SHELL_URL_ENDPOINT} ` + + `${hookEverAnswered ? "answered, but named a different document" : "never answered"}. ` + + `Targets seen: ${lastUrls.join(", ") || "none"}.`, ); } @@ -165,7 +228,7 @@ export const test = base.extend({ // rejects a 127.0.0.1 Host header. See helpers/api.ts.) browser = await connectOverCdpWithRetry(launched.cdpPort); const bloomApp: IBloomApp = { - page: await findShellPage(browser), + page: await findShellPage(browser, launched.httpPort), httpPort: launched.httpPort, cdpPort: launched.cdpPort, bloomPid: launched.bloomPid, @@ -179,7 +242,12 @@ export const test = base.extend({ browser = await connectOverCdpWithRetry( launched!.cdpPort, ); - bloomApp.page = await findShellPage(browser); + // Resolve the shell again: the restarted Bloom has a new shell document, + // and the old page object points at a dead target. + bloomApp.page = await findShellPage( + browser, + launched!.httpPort, + ); bloomApp.httpPort = launched!.httpPort; bloomApp.cdpPort = launched!.cdpPort; bloomApp.bloomPid = launched!.bloomPid; diff --git a/src/BloomE2E/fixtures/launchBloom.ts b/src/BloomE2E/fixtures/launchBloom.ts index d7e4bc1b6e02..f263f08ca5ba 100644 --- a/src/BloomE2E/fixtures/launchBloom.ts +++ b/src/BloomE2E/fixtures/launchBloom.ts @@ -158,6 +158,40 @@ function samePath(a: string, b: string): boolean { const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); +/** + * Whether the Bloom we launch should appear on a monitor. By default it does not: --headless puts + * its window far outside every screen, so a run does not take the developer's desktop over. + * + * The window is moved off-screen rather than minimized or hidden because WebView2 stops painting a + * minimized window: screenshots come back blank and the layout is the wrong size. Off-screen, the + * window paints exactly as it would in front of a person, so keyboard input and rendering behave + * the same (see Shell.GetHeadlessBounds). + * + * Set BLOOM_E2E_HEADED=1 to watch the run. Playwright's --debug (which sets PWDEBUG) implies it: + * there is no point stepping through a test whose window you cannot see. + */ +function shouldShowBloomOnScreen(): boolean { + return process.env.BLOOM_E2E_HEADED === "1" || !!process.env.PWDEBUG; +} + +/** + * The Vite dev server port the launched Bloom should load its React front end from, or undefined + * to leave the choice to Bloom. + * + * A launched Bloom serves its UI from the built output/browser unless it is told about a dev + * server, so an edit to a .tsx file does not reach the suite until somebody rebuilds the bundle, + * which AGENTS.md reserves for a developer or CI. Set BLOOM_E2E_VITE_PORT= and Bloom loads the + * front end from that dev server instead, so the suite tests the working tree. + * + * Leaving the variable unset is NOT the same as "no dev server". A dev build of Bloom probes port + * 5173 by itself (ReactControl.TryGetActiveViteDevPort), so a developer's own dev server silently + * changes what the suite tests, and Bloom has no option that means "ignore any dev server" + * (--vite-port rejects 0). See AUTOMATION-DEBT.md. + */ +function getViteDevPort(): string | undefined { + return process.env.BLOOM_E2E_VITE_PORT || undefined; +} + /** What common/instanceInfo tells us about a running Bloom. Only the fields we use. */ interface IInstanceInfo { editableCollectionFolder?: string; @@ -355,11 +389,14 @@ async function startBloomOn( // --e2e: skip the DEBUG "attach debugger now" prompt and suppress modal error dialogs. // --automation: let this instance run alongside a Bloom the developer already has open. - const bloomProcess: ChildProcess = execFile(exe, [ - findCollectionFile(collectionDir), - "--e2e", - "--automation", - ]); + // --headless: keep the window off every screen (see shouldShowBloomOnScreen). + const args = [findCollectionFile(collectionDir), "--e2e", "--automation"]; + if (!shouldShowBloomOnScreen()) args.push("--headless"); + // --vite-port: serve the React front end from a dev server, so the suite tests the working + // tree rather than a stale output/browser (see getViteDevPort). + const vitePort = getViteDevPort(); + if (vitePort) args.push("--vite-port", vitePort); + const bloomProcess: ChildProcess = execFile(exe, args); let exitStatus: { code: number | null; signal: string | null } | undefined; bloomProcess.stdout?.on("data", (d) => recordOutput(String(d))); bloomProcess.stderr?.on("data", (d) => recordOutput(String(d))); diff --git a/src/BloomE2E/helpers/bookMaking.ts b/src/BloomE2E/helpers/bookMaking.ts index 578fca26163b..8a17b954b295 100644 --- a/src/BloomE2E/helpers/bookMaking.ts +++ b/src/BloomE2E/helpers/bookMaking.ts @@ -305,30 +305,38 @@ export async function addPage( * it is leaving, so text typed into a box reaches the file only once the book moves off that page. */ export async function goToPage(page: Page, pageId: string): Promise { - // The Edit tab drops a jump that arrives while it is still loading a page, so wait for it to - // be showing one before asking for another. - await waitForEditablePage(page); - - // Ask up to three times. Coming back from the Publish tab, the Edit tab can still swallow a - // jump after it looks ready, and asking again costs a few seconds where failing costs the run. + // One request is enough: the Edit tab queues a jump that arrives while it is loading a page or + // saving one, and answers with an error if it can do neither (EditingModel.JumpToPage). It used + // to drop such a jump and report success, which is why this helper used to ask three times. + await apiPost(page, "editView/jumpToPage", pageId, "text/plain"); const showing = async () => (await page .frame({ name: "page" }) ?.locator(`.bloom-page[id="${pageId}"]`) .count() .catch(() => 0)) ?? 0; - for (let attempt = 1; attempt <= 3; attempt++) { - await apiPost(page, "editView/jumpToPage", pageId, "text/plain"); - try { - await expect.poll(showing, { timeout: 20000 }).toBe(1); - return; - } catch { - // fall through and ask again - } + try { + await expect.poll(showing, { timeout: 60000 }).toBe(1); + } catch { + // Say what the frame does hold. "Bloom never showed the page" on its own cannot tell a + // page that never arrived from a page that arrived and was replaced by another one. + const frame = page.frame({ name: "page" }); + const heldPageIds = frame + ? await frame + .locator(".bloom-page") + .evaluateAll((pages) => pages.map((p) => p.id)) + .catch(() => ["(could not read the frame)"]) + : []; + throw new Error( + `Bloom never showed page ${pageId} in the Edit tab. ` + + (frame + ? `The 'page' frame is at ${frame.url()} and holds [${heldPageIds.join(", ")}].` + : `There is no 'page' frame.`) + + ` Bloom drives the shell at ${(await apiGet(page, "e2e/shellUrl")).body}; ` + + `this test is watching ${page.url()}. If those name different documents, the test ` + + `attached to the wrong one and nothing Bloom does will ever appear (see AUTOMATION-DEBT.md).`, + ); } - throw new Error( - `Bloom never showed page ${pageId} in the Edit tab, after three attempts.`, - ); } /** @@ -377,7 +385,10 @@ export async function typeInGroup( await box.click(); await box.press("Control+a"); await box.press("Delete"); - if (text) await box.pressSequentially(text); + // One insertion rather than a key press per character: the box has focus, and CKEditor and + // Bloom's own markup code both work from the input event this raises, so the result is the + // same and the cost does not grow with the length of the text. + if (text) await page.keyboard.insertText(text); // Bloom's editor reacts to typing; confirm the box holds what we meant before moving on, so a // later failure cannot be blamed on text that never arrived. await expect(box).toHaveText(text, { timeout: 15000 }); diff --git a/src/BloomE2E/helpers/collection.ts b/src/BloomE2E/helpers/collection.ts index c5abfcbb1351..2380fdfa73f3 100644 --- a/src/BloomE2E/helpers/collection.ts +++ b/src/BloomE2E/helpers/collection.ts @@ -7,6 +7,7 @@ import { expect, type Page } from "@playwright/test"; import { apiGet, apiPost } from "./api"; +import type { IBloomApp } from "../fixtures/bloomTest"; /** * Wait until the editable collection is loaded and its books can be enumerated. Switching to the @@ -45,3 +46,35 @@ export async function selectBook( `&collection-id=${encodeURIComponent(collectionId)}`, ); } + +/** + * Set the collection's languages, and give Bloom back to the test with the change in effect. + * + * `tags` holds one to three language tags, for Language 1, Language 2 and Language 3. Fewer than + * three leaves the rest empty, which is how a collection ends up with no Language 3. + * + * Bloom reads a collection's languages when it opens the collection, so this restarts it, exactly + * as clicking OK in the Collection Settings dialog makes a user reopen the collection. The restart + * kills the process, so leave the page being edited first or what was typed on it is lost, and use + * the page this returns: the old one points at a dead target. + * + * The `e2e/setCollectionLanguages` hook writes the .bloomCollection with Bloom's own code, so + * everything else in the collection's settings survives. See AUTOMATION-DEBT.md for why there is + * no production API for this. + */ +export async function setCollectionLanguages( + bloomApp: IBloomApp, + tags: string[], +): Promise { + if (tags.length < 1 || tags.length > 3) + throw new Error( + `A collection has one to three languages; setCollectionLanguages was given ${tags.length}.`, + ); + await apiPost( + bloomApp.page, + "e2e/setCollectionLanguages", + JSON.stringify(tags), + "application/json", + ); + return bloomApp.restart(); +} diff --git a/src/BloomE2E/helpers/publish.ts b/src/BloomE2E/helpers/publish.ts index 59815df5d07d..4f3f795ead7b 100644 --- a/src/BloomE2E/helpers/publish.ts +++ b/src/BloomE2E/helpers/publish.ts @@ -159,24 +159,36 @@ export async function clickTextLanguage( export async function showBloomPubPreview(page: Page): Promise { await page.locator('[aria-label="refresh preview"]').click(); let player: Frame | undefined; - await expect - .poll( - async () => { - player = page - .frames() - .find((f) => f.url().includes("bloomplayer.htm")); - if (!player) return 0; - return player - .locator('[aria-label="Choose Language"]') - .count() - .catch(() => 0); - }, - { - timeout: 120000, - message: "The BloomPUB preview never showed the book.", - }, - ) - .toBeGreaterThan(0); + try { + await expect + .poll( + async () => { + player = page + .frames() + .find((f) => f.url().includes("bloomplayer.htm")); + if (!player) return 0; + return player + .locator('[aria-label="Choose Language"]') + .count() + .catch(() => 0); + }, + { timeout: 120000 }, + ) + .toBeGreaterThan(0); + } catch { + // Say which of the two steps did not happen: Bloom never gave the player a book to show, + // or the player has one and never finished loading it. Without this the failure looks the + // same either way, and the two have nothing to do with each other. + const frameUrls = page.frames().map((frame) => frame.url()); + throw new Error( + `The BloomPUB preview never showed the book. ` + + (player + ? `bloom-player is loaded at ${player.url()} but never offered the language ` + + `menu, so it never finished showing a book.` + : `No frame of this page is bloom-player, so Bloom never staged the ` + + `publication. Frames: ${frameUrls.join(", ")}.`), + ); + } return player!; } diff --git a/src/BloomE2E/helpers/screenshot.ts b/src/BloomE2E/helpers/screenshot.ts new file mode 100644 index 000000000000..58aa5c2dbd77 --- /dev/null +++ b/src/BloomE2E/helpers/screenshot.ts @@ -0,0 +1,259 @@ +// Capture an image of one element in Bloom's WebView2, including an element taller than the +// window. +// +// A book page is the case that forces this. A `.bloom-page` in the Edit tab is usually taller than +// the WebView2's window, and the obvious route, `Page.captureScreenshot` with +// `captureBeyondViewport: true`, hangs in WebView2: no response, no error, and the run dies on a +// timeout with nothing to read. The pattern that works, and the one this module encodes, is: +// +// 1. Enlarge the window with Emulation.setDeviceMetricsOverride, big enough for the whole +// element. +// 2. Measure the element only after that, because enlarging the window re-lays out the page. +// 3. Screenshot with a `clip` for the element's box. +// 4. Emulation.clearDeviceMetricsOverride, always, even when the capture failed. A left-over +// override leaves the rest of the run driving a Bloom of the wrong size. +// +// Every CDP request also gets a timeout, because a WebView2 CDP call that never answers otherwise +// stops the run rather than failing it. (AUTOMATION-DEBT.md: "Driver-level CDP footguns that the +// automation library must absorb".) +// +// Tests do not open CDP sessions of their own. If you need a capture this file does not do, add it +// here. + +import type { CDPSession, Locator, Page } from "@playwright/test"; +import { editablePageFrame, waitForEditablePage } from "./bookMaking"; + +/** An element's image, and the size of that image in pixels. */ +export interface IElementImage { + /** The PNG bytes. */ + png: Buffer; + /** The image's width in pixels, read from the PNG itself. */ + width: number; + /** The image's height in pixels, read from the PNG itself. */ + height: number; + /** + * The element's own box, in CSS pixels, at the moment we captured it. The image should be this + * size (to within the rounding CDP does), so a caller can check that it captured the element + * rather than the window. + */ + elementWidth: number; + /** The element's own height in CSS pixels at the moment we captured it. */ + elementHeight: number; +} + +/** The CDP method names Playwright's session accepts. */ +type CdpMethod = Parameters[0]; + +/** How long any one CDP request may take before we call the driver stuck. */ +const CDP_TIMEOUT_MS = 30000; + +// The largest window we will pretend to have. A book page cannot legitimately need more than this, +// and an absurd number here would make WebView2 try to allocate an absurd surface. +const MAX_OVERRIDE_PIXELS = 8000; + +/** + * Capture the `.bloom-page` the Edit tab is showing. + * + * This waits for the Edit tab to have a page with editable text in it first, so a caller does not + * have to; call goToPage (helpers/bookMaking.ts) first to choose which page. + */ +export async function captureCurrentBookPage( + page: Page, + timeoutMs = 90000, +): Promise { + await waitForEditablePage(page, timeoutMs); + const bloomPage = editablePageFrame(page).locator(".bloom-page").first(); + return captureElement(bloomPage, timeoutMs); +} + +/** + * Capture one element as a PNG, whatever its size, and return the bytes with the image's real + * dimensions. + * + * The element may be inside an iframe: the box is measured in the top document's coordinates, + * which is what CDP's clip wants. Fails with a message naming the locator when the element has no + * on-screen box, which is what a collapsed or zero-size container looks like from here. + */ +export async function captureElement( + locator: Locator, + timeoutMs = 30000, +): Promise { + const page = locator.page(); + await locator.waitFor({ state: "visible", timeout: timeoutMs }); + + const session = await page.context().newCDPSession(page); + try { + // How big the window has to be for the whole element to be laid out at once. Ask for the + // element's own scroll size, plus where it sits, rather than the document's size: the + // document includes page-list thumbnails and other chrome we are not capturing. + const wanted = await elementExtent(locator, timeoutMs); + const viewport = page.viewportSize(); + const overrideWidth = clampOverride( + Math.max(wanted.right, viewport?.width ?? 0), + ); + const overrideHeight = clampOverride( + Math.max(wanted.bottom, viewport?.height ?? 0), + ); + + await sendWithTimeout(session, "Emulation.setDeviceMetricsOverride", { + width: overrideWidth, + height: overrideHeight, + deviceScaleFactor: 1, + mobile: false, + }); + + // Measure only now. Enlarging the window re-lays out the page, and on a book page it + // genuinely moves things: the Edit tab centres the page in the space it has. + const box = await documentBox(locator, timeoutMs); + + const result = (await sendWithTimeout( + session, + "Page.captureScreenshot", + { + format: "png", + // No captureBeyondViewport: it hangs in WebView2. The override above is what makes + // the whole element fit inside the window instead. + clip: { + x: box.x, + y: box.y, + width: box.width, + height: box.height, + scale: 1, + }, + }, + )) as { data: string }; + + const png = Buffer.from(result.data, "base64"); + const size = readPngSize(png); + return { + png, + width: size.width, + height: size.height, + elementWidth: box.width, + elementHeight: box.height, + }; + } finally { + // Always, including after a failed capture: the next test in this worker drives the same + // Bloom, and an emulated 8000-pixel window would make everything it sees wrong. + await sendWithTimeout( + session, + "Emulation.clearDeviceMetricsOverride", + {}, + ).catch(() => undefined); + await session.detach().catch(() => undefined); + } +} + +/** Keep an override within what WebView2 can reasonably allocate, and never ask for zero. */ +function clampOverride(pixels: number): number { + return Math.max(1, Math.min(MAX_OVERRIDE_PIXELS, Math.ceil(pixels))); +} + +/** + * How far right and down the element reaches in the top document, counting its own scrollable + * content. This is what the window has to be enlarged to before the element is fully laid out. + */ +async function elementExtent( + locator: Locator, + timeoutMs: number, +): Promise<{ right: number; bottom: number }> { + const box = await requireBoundingBox(locator, timeoutMs); + const scroll = await locator.evaluate((element) => ({ + width: element.scrollWidth, + height: element.scrollHeight, + })); + return { + right: box.x + Math.max(box.width, scroll.width), + bottom: box.y + Math.max(box.height, scroll.height), + }; +} + +/** + * The element's box in the TOP document's coordinates, which is the space CDP's clip is in. + * Playwright reports a box relative to the top document's window, so add that window's scroll. + */ +async function documentBox( + locator: Locator, + timeoutMs: number, +): Promise<{ x: number; y: number; width: number; height: number }> { + const box = await requireBoundingBox(locator, timeoutMs); + const scroll = await locator.page().evaluate(() => ({ + x: window.scrollX, + y: window.scrollY, + })); + return { + x: box.x + scroll.x, + y: box.y + scroll.y, + width: box.width, + height: box.height, + }; +} + +/** The element's on-screen box, or an error saying which element had none. */ +async function requireBoundingBox( + locator: Locator, + timeoutMs: number, +): Promise<{ x: number; y: number; width: number; height: number }> { + const box = await locator.boundingBox({ timeout: timeoutMs }); + if (!box) + throw new Error( + `${locator} is visible but has no bounding box, so there is nothing to capture. ` + + `It may be inside a collapsed or zero-size container.`, + ); + if (box.width < 1 || box.height < 1) + throw new Error( + `${locator} measures ${box.width}x${box.height}, which is too small to capture.`, + ); + return box; +} + +/** + * Send one CDP request, and fail rather than hang when WebView2 never answers. A CDP call with no + * reply is the failure mode this whole module exists to absorb, so it gets its own deadline here + * instead of relying on Playwright's test timeout to notice. + */ +async function sendWithTimeout( + session: CDPSession, + method: TMethod, + params: object, +): Promise { + let timer: NodeJS.Timeout | undefined; + const expired = new Promise((_resolve, reject) => { + timer = setTimeout( + () => + reject( + new Error( + `The CDP request ${method} got no reply within ${CDP_TIMEOUT_MS / 1000}s. ` + + `WebView2 stops answering rather than failing, so treat this as the ` + + `driver being stuck.`, + ), + ), + CDP_TIMEOUT_MS, + ); + }); + try { + // `as never` only because the two arguments are typed as one pair per method, and this + // wrapper is deliberately method-agnostic; the method name itself is still checked. + return await Promise.race([ + session.send(method, params as never), + expired, + ]); + } finally { + if (timer) clearTimeout(timer); + } +} + +/** + * Read a PNG's pixel dimensions out of its header, so a caller can assert on the size of what it + * captured without this package depending on an image library. + */ +export function readPngSize(png: Buffer): { width: number; height: number } { + // 8-byte signature, then a 4-byte length and the "IHDR" tag, then width and height as 32-bit + // big-endian integers. + const signature = "89504e470d0a1a0a"; + if (png.length < 24 || png.subarray(0, 8).toString("hex") !== signature) + throw new Error( + `That is not a PNG: ${png.length} bytes starting ${png.subarray(0, 8).toString("hex")}.`, + ); + return { width: png.readUInt32BE(16), height: png.readUInt32BE(20) }; +} diff --git a/src/BloomE2E/helpers/workspace.ts b/src/BloomE2E/helpers/workspace.ts index 342ce7e1dbbc..31d463968458 100644 --- a/src/BloomE2E/helpers/workspace.ts +++ b/src/BloomE2E/helpers/workspace.ts @@ -22,14 +22,11 @@ export interface IWorkspaceTabs { navigationLocked: boolean; } -// The visible label on each tab in the top bar. These differ from the API's tab ids, which is why -// a test names the id and this map does the translating. The labels are localized, so a run in -// another UI language would need this to come from the l10n data instead. -const TAB_LABEL: Record = { - collection: "Collections", - edit: "Edit", - publish: "Publish", -}; +// The test id on each tab in the top bar, set in react_components/TopBar/TopBar.tsx. The tab ids +// here are Bloom's own API names, and the test ids are built from them, so this needs no map. +function tabTestId(tab: WorkspaceTabId): string { + return `workspace-tab-${tab}`; +} /** Ask Bloom which workspace tab is active and what state the others are in. */ export async function getTabs(page: Page): Promise { @@ -59,13 +56,15 @@ export async function waitForActiveTab( * * Bloom hides the Edit and Publish tabs entirely until a book is selected, so a test that wants * either of them must select a book first (see helpers/collection.ts). + * + * The tab is found by its test id, not by its label, so this works in any UI language. */ export async function switchTab( page: Page, tab: WorkspaceTabId, timeoutMs = 30000, ): Promise { - const target = page.getByRole("tab", { name: TAB_LABEL[tab] }); + const target = page.getByTestId(tabTestId(tab)); await target.waitFor({ state: "visible", timeout: timeoutMs }); await target.click(); await waitForActiveTab(page, tab, timeoutMs); diff --git a/src/BloomE2E/tests/capture-book-page.spec.ts b/src/BloomE2E/tests/capture-book-page.spec.ts new file mode 100644 index 000000000000..2f7a2f7fe729 --- /dev/null +++ b/src/BloomE2E/tests/capture-book-page.spec.ts @@ -0,0 +1,53 @@ +// Proves the capture helper works against a real WebView2, on a page taller than the window. +// +// This is the test for helpers/screenshot.ts rather than a test of a Bloom behavior. It exists +// because the driver-level footgun it absorbs is invisible from the outside: the obvious CDP route +// to this image (captureBeyondViewport) hangs with no error, and nothing else in the suite would +// notice if the safe pattern stopped working. See AUTOMATION-DEBT.md, "Driver-level CDP footguns +// that the automation library must absorb". +// +// This test has no "[Test Case ID N]" tag because it covers the automation library, not a case in +// the Notion test inventory. + +import * as Path from "node:path"; +import { expect, test } from "../fixtures/bloomTest"; +import { selectBook } from "../helpers/collection"; +import { getPages } from "../helpers/bookMaking"; +import { captureCurrentBookPage } from "../helpers/screenshot"; +import { switchTab } from "../helpers/workspace"; + +test.use({ collectionName: "basic" }); + +test("capturing the cover page gives a PNG the size of the page", async ({ + page, + bloomApp, +}) => { + await selectBook(page, Path.join(bloomApp.collectionDir, "A5 Portrait")); + await switchTab(page, "edit"); + + // Sanity check: the Edit tab opens on the cover, so there is a page to capture and it is the + // one this test says it captures. + const pages = await getPages(page); + expect(pages.length).toBeGreaterThan(0); + + const image = await captureCurrentBookPage(page); + + // Non-empty, and a PNG: readPngSize inside the helper already rejects anything else, so a + // plausible byte count is what is left to check. + expect(image.png.length).toBeGreaterThan(1000); + + // The image is the page, not the window: its pixels match the element's own box. CDP rounds + // the clip, so allow a pixel either way. + expect(image.width).toBeGreaterThan(100); + expect(image.height).toBeGreaterThan(100); + expect(Math.abs(image.width - image.elementWidth)).toBeLessThanOrEqual(1); + expect(Math.abs(image.height - image.elementHeight)).toBeLessThanOrEqual(1); + + // A5 Portrait is taller than it is wide, which is the case that needs the window override. + expect(image.height).toBeGreaterThan(image.width); + + // Put the workspace back on the collection tab. The launched Bloom is worker-scoped, so every + // test with these same options shares it: a test that ends on the Edit tab makes the next one + // start there. See AUTOMATION-DEBT.md, "One test's tab is the next test's starting state". + await switchTab(page, "collection"); +}); diff --git a/src/BloomE2E/tests/publish-text-languages.spec.ts b/src/BloomE2E/tests/publish-text-languages.spec.ts index 0181e7b04ab8..97b23cdda65e 100644 --- a/src/BloomE2E/tests/publish-text-languages.spec.ts +++ b/src/BloomE2E/tests/publish-text-languages.spec.ts @@ -11,10 +11,7 @@ // run before the test that clicks a box, and the file would quietly stop testing the defaults if // they were reordered. -import * as fs from "node:fs"; -import * as Path from "node:path"; import { expect, test } from "../fixtures/bloomTest"; -import { makeCollectionXml } from "../fixtures/launchBloom"; import { addPage, findBookFolder, @@ -25,7 +22,7 @@ import { setContentLanguages, typeInGroup, } from "../helpers/bookMaking"; -import { selectBook } from "../helpers/collection"; +import { selectBook, setCollectionLanguages } from "../helpers/collection"; import { clickTextLanguage, expectTextLanguageRows, @@ -97,19 +94,9 @@ test.describe("the Text Languages publish list", () => { ); await goToPage(page, coverBeforeRestart!.id); - // Now swap German out for Spanish. Collection settings have no API and their dialog is a - // WinForms surface CDP cannot reach, so the way to change them is to quit Bloom, rewrite - // the .bloomCollection, and start again. See AUTOMATION-DEBT.md. - const newPage = await bloomApp.restart(() => - fs.writeFileSync( - Path.join( - bloomApp.collectionDir, - `${COLLECTION_NAME}.bloomCollection`, - ), - makeCollectionXml(FINAL_LANGUAGES), - "utf8", - ), - ); + // Now swap German out for Spanish. Bloom reads the collection's languages when it opens + // the collection, so this restarts it; see setCollectionLanguages. + const newPage = await setCollectionLanguages(bloomApp, FINAL_LANGUAGES); bookFolder = await findBookFolder(newPage, BOOK_TITLE); await selectBook(newPage, bookFolder); await switchTab(newPage, "edit"); @@ -356,29 +343,27 @@ test.describe("the Text Languages publish list", () => { await setContentLanguages(page, ["en"]); }); - // KNOWN FLAKE: this test failed once in six full runs on 2026-09-01, and the cause is not - // known. The failure was not reproduced, and the log kept only the tail, so the assertion that - // failed was not captured. Whoever sees it fail again: keep the whole log. This test does not - // change which languages the book shows, so the earlier suspicion about quick successive - // calls to editView/topBar/contentLanguageUsageChange does not explain it. - test("keeps a language that the collection no longer has, under its own name [Test Case ID 169]", async ({ + // FORMER FLAKE: this test failed once in six full runs on 2026-09-01, and the cause was not + // known then. The likely cause was found later the same day: the fixture could attach to a + // workspace document Bloom was not driving, which fails the tests that come after a restart + // (the file runs serially). bloomTest.findShellPage now asks Bloom which document it drives. + // If this test fails again, keep the whole log; the first failure kept only the tail. + test("keeps a language that the collection no longer has, under the name the collection remembers [Test Case ID 169]", async ({ bloomApp, }) => { test.setTimeout(180000); // Drop Spanish from the collection. The book still has Spanish text, so the language stays - // in the list; but the collection no longer supplies a name for it, so Bloom falls back to - // the name the language calls itself. - const withoutSpanish = await bloomApp.restart(() => - fs.writeFileSync( - Path.join( - bloomApp.collectionDir, - `${COLLECTION_NAME}.bloomCollection`, - ), - makeCollectionXml(["en", "fr"]), - "utf8", - ), - ); + // in the list, named "Spanish". Removing a language does not throw its settings away: + // CollectionSettingsDialog.UpdateLanguageSettings moves the displaced language to the end + // of the collection's list, name and font and all, so the name outlives the removal. (This + // test used to expect "español", the language's own name for itself, because it dropped + // the language by rewriting the .bloomCollection, which no part of Bloom does. Going + // through the same code the dialog's OK button runs is what changed the answer.) + const withoutSpanish = await setCollectionLanguages(bloomApp, [ + "en", + "fr", + ]); await selectBook(withoutSpanish, bookFolder); await openPublishDestination(withoutSpanish, "Web"); // In any order: where a language the collection no longer names sits in the list is not @@ -399,25 +384,19 @@ test.describe("the Text Languages publish list", () => { disabled: false, }, { - name: "español", + name: "Spanish", incomplete: false, checked: false, disabled: false, }, ], - "Spanish did not stay in the list, unchecked and under its own name.", + "Spanish did not stay in the list, unchecked and under the name the collection remembers.", ); // Put Spanish back, for the test that follows. - const withSpanish = await bloomApp.restart(() => - fs.writeFileSync( - Path.join( - bloomApp.collectionDir, - `${COLLECTION_NAME}.bloomCollection`, - ), - makeCollectionXml(FINAL_LANGUAGES), - "utf8", - ), + const withSpanish = await setCollectionLanguages( + bloomApp, + FINAL_LANGUAGES, ); await selectBook(withSpanish, bookFolder); }); diff --git a/src/BloomE2E/tests/workspace-tabs.spec.ts b/src/BloomE2E/tests/workspace-tabs.spec.ts index 0079e3ac5405..322e933621ac 100644 --- a/src/BloomE2E/tests/workspace-tabs.spec.ts +++ b/src/BloomE2E/tests/workspace-tabs.spec.ts @@ -22,7 +22,9 @@ test("switching workspace tabs through the real top bar", async ({ bloomApp, }) => { // Sanity check the start state, so a failure below means a click failed rather than that we - // were already on the tab we were about to click. + // were already on the tab we were about to click. This reads the state once rather than waiting + // for it: a Bloom that is not on the collection tab here has been left that way by an earlier + // test sharing this worker's Bloom, and waiting would only turn that into a slow failure. expect((await getTabs(page)).tabStates.collection).toBe("active"); // Setup, not the behavior under test: Bloom hides the Edit and Publish tabs until a book is diff --git a/src/BloomExe/Edit/EditingModel.cs b/src/BloomExe/Edit/EditingModel.cs index 86a906d6a121..a8c945f16183 100644 --- a/src/BloomExe/Edit/EditingModel.cs +++ b/src/BloomExe/Edit/EditingModel.cs @@ -47,6 +47,10 @@ public class EditingModel private bool _reloadFromDiskOnLeavingEditTab; public bool Visible; + + // A page a JumpToPage call asked for while the Edit tab was not showing. OnBecomeVisible + // displays this page instead of the one it would otherwise choose. See JumpToPage. + private string _pageIdToShowWhenVisible; private Book.Book _currentlyDisplayedBook; private Book.Book _bookForToolboxContent; private EditingView _view; @@ -515,6 +519,8 @@ BookSelectionChangedEventArgs bookSelectionChangedEventArgs // This edit tab can ignore changes that don't actually involve selecting a different book. if (_bookSelection.CurrentSelection == _currentlyDisplayedBook) return; + // A jump queued for a page of the book we are leaving means nothing in the new one. + _pageIdToShowWhenVisible = null; //prevent trying to save this page in whatever comes next var hadPageToSave = _havePageToSave; _havePageToSave = false; @@ -996,11 +1002,23 @@ public void OnBecomeVisible() ErrorReportUtils.CheckForFakeTestErrorsIfNotRealUser(_currentlyDisplayedBook.Title); - // BL-2339: try to choose the last edited page - var page = - _currentlyDisplayedBook.GetPageByIndex( - _currentlyDisplayedBook.UserPrefs.MostRecentPage - ) ?? _currentlyDisplayedBook.FirstPage; + // A jump asked for while this tab was not showing wins over the remembered page: it is + // the more recent request. See JumpToPage. + var requestedPageId = _pageIdToShowWhenVisible; + _pageIdToShowWhenVisible = null; + IPage page = null; + if (requestedPageId != null) + page = _currentlyDisplayedBook + .GetPages() + .FirstOrDefault(p => p.Id == requestedPageId); + if (page == null) + { + // BL-2339: try to choose the last edited page + page = + _currentlyDisplayedBook.GetPageByIndex( + _currentlyDisplayedBook.UserPrefs.MostRecentPage + ) ?? _currentlyDisplayedBook.FirstPage; + } if (page != null) _view.GoToPage(page); @@ -1687,6 +1705,58 @@ internal void SavePageAndReloadIt(ApiRequest request) request.PostSucceeded(); } + /// + /// Show the page with this id in the Edit tab. The page being left is saved on the way, + /// which is how anything typed on it reaches the file. + /// + /// A jump can arrive when the Edit tab cannot act on it at once: the tab is not showing + /// yet, it is still navigating to a page, or a save is in flight. In each of those cases + /// the jump is remembered and done as soon as that finishes. Bloom used to drop it and + /// report success, which left the caller waiting for a page that was never coming (see + /// src/BloomE2E/AUTOMATION-DEBT.md). + /// + /// Returns false when the jump can be neither started nor queued, so the caller can say so + /// rather than wait: there is no book, or the Edit tab is in the momentary state after a + /// save in which no transition is allowed. + /// + public bool JumpToPage(string pageId) + { + if (CurrentBook == null || string.IsNullOrEmpty(pageId)) + return false; + + if (!Visible) + { + // The Edit tab is not showing, so there is nothing to save and nowhere to + // navigate. OnBecomeVisible chooses the page to display, so hand it this one. + _pageIdToShowWhenVisible = pageId; + return true; + } + + // Ask to be run again when the Edit tab is done with what it is doing. We check the + // state before calling SaveThen so that the answer we give the caller is decided + // before any save starts. + if (_stateMachine.SavePending) + return _stateMachine.DeferUntilSaveCompletes(() => JumpToPage(pageId)); + if (_stateMachine.Navigating) + return _stateMachine.DeferUntilPageIsLoaded(() => JumpToPage(pageId)); + + var jumped = true; + SaveThen( + () => pageId, + doIfNotInRightStateToSave: () => + { + // We ruled out the two states that refuse a save above, so we are in the + // momentary SavedAndStripped state, which an API call should not be able to + // observe. Say so rather than drop the request silently. + Logger.WriteEvent( + $"EditingModel.JumpToPage({pageId}): the edit tab was not in a state to save, so the jump was refused." + ); + jumped = false; + } + ); + return jumped; + } + private bool CannotSavePage() { return _bookSelection == null @@ -2202,6 +2272,13 @@ public void HandlePageDomLoadedEvent(string pageId) // move on to the next page. See StartUpdatingAllPages(). if (nowEditing && _updatingAllPages) AdvanceUpdatingAllPages(pageId); + + // Last of all, a jump that arrived while we were navigating here (see JumpToPage). + // It saves this page and navigates off it, so everything above, which acts on the page + // that just loaded, has to happen first. If the work above left us navigating or + // saving again, JumpToPage queues itself once more. + if (nowEditing) + _stateMachine.TakeWorkDeferredUntilPageIsLoaded()?.Invoke(); } // The one action queued by RunAfterNextPageLoad, or null. diff --git a/src/BloomExe/Edit/EditingStateMachine.cs b/src/BloomExe/Edit/EditingStateMachine.cs index 2bbe9717ab0b..6011ecfbf397 100644 --- a/src/BloomExe/Edit/EditingStateMachine.cs +++ b/src/BloomExe/Edit/EditingStateMachine.cs @@ -50,6 +50,10 @@ public class EditingStateMachine // save has completed and we are back in a state that allows transitions. // See DeferUntilSaveCompletes. private Action _workToDoAfterInFlightSave; + + // Work that arrived while we were navigating to a page and could not be done then. It runs + // once that page has loaded. See DeferUntilPageIsLoaded. + private Action _workToDoAfterNavigation; private Action _hidePage; private Action _enableStateTransitions; // arg is (enabled) @@ -105,6 +109,10 @@ public bool ToNoPage() return true; case State.Navigating: LogShortcut("empty page"); + // The navigation this work was waiting for is over, and no page loaded, so + // the work must not run: it belonged to a page we are no longer showing. This + // is the path a switch away from the Edit tab takes. + _workToDoAfterNavigation = null; _hidePage(); _currentState = State.NoPage; return true; @@ -197,9 +205,32 @@ private void StartNavigating(string pageId) } /// - /// Called after we hear from the browser JS that the dom is finished loading + /// Called after we hear from the browser JS that the dom is finished loading. + /// Work that had to wait for this navigation does NOT run here; the caller takes it with + /// TakeWorkDeferredUntilPageIsLoaded once it has done its own work on the loaded page. /// public bool ToEditing(string pageId) + { + return ToEditingFromNavigating(pageId); + } + + /// + /// Hand back the work that was deferred until a page loaded (see DeferUntilPageIsLoaded), and + /// forget it, so the caller can run it. Returns null when there is none. + /// + /// The caller runs it rather than ToEditing, because the code that hears "the page has loaded" + /// has its own work to do first: an action queued for the next page load, and the next step of + /// the Update Book pass. A jump that ran before those would save and navigate away from the + /// page they were about to act on. See EditingModel.HandlePageDomLoadedEvent. + /// + public Action TakeWorkDeferredUntilPageIsLoaded() + { + var deferredWork = _workToDoAfterNavigation; + _workToDoAfterNavigation = null; + return deferredWork; + } + + private bool ToEditingFromNavigating(string pageId) { try { @@ -404,6 +435,32 @@ public bool DeferUntilSaveCompletes(Action work) return true; } + /// + /// For a caller whose work needs a loaded page, and which found us navigating to one (which is + /// why ToSavePending refused it). If we really are navigating, is + /// remembered and run once that page has loaded, and this returns true — the caller must then + /// do nothing else. Otherwise it returns false and the caller must handle its own request. + /// + /// A navigation that ends any other way than with a loaded page (a switch away from the Edit + /// tab, which comes through ToNoPage) throws the work away: it belonged to a page that is no + /// longer being shown. + /// + /// A jump to a page is the case this exists for: it arrives from an API call at any moment, + /// including while the Edit tab is loading the page it displays on becoming visible. Bloom used + /// to drop such a jump and tell the caller it had succeeded (see src/BloomE2E/AUTOMATION-DEBT.md). + /// + /// Only one piece of deferred work is kept: a later request supersedes an earlier one, since it + /// is the more recent thing that was asked for. + /// + public bool DeferUntilPageIsLoaded(Action work) + { + // We are not navigating, or there is nothing to do: the caller must handle it itself. + if (_currentState != State.Navigating || work == null) + return false; + _workToDoAfterNavigation = work; + return true; + } + /// /// Source: API call providing content of current page will request this after saving and before executing pending action /// (e.g. changing pages) @@ -505,6 +562,11 @@ public bool ToSavedAndStripped(Func postSaveAction, string pageContentOr private void Log(string message) { Debug.WriteLine("[EditingStateMachine] " + message); + // Under --e2e these go into Bloom's log as well. A test that waits for a page the edit tab + // never shows is otherwise a mystery: nothing else records which state the tab was in or + // which request it turned down. + if (Bloom.Program.RunningE2eTests) + Logger.WriteEvent("[EditingStateMachine] " + message); } private void LogTransition(string nextState, string nextPageId) diff --git a/src/BloomExe/Program.cs b/src/BloomExe/Program.cs index 24e841b11c01..1495dc052564 100644 --- a/src/BloomExe/Program.cs +++ b/src/BloomExe/Program.cs @@ -108,6 +108,11 @@ static class Program internal static string StartupLabel { get; private set; } internal static bool StartupAutomation { get; private set; } + // True when --headless was passed. An e2e run wants Bloom's window to paint (screenshots + // and keyboard input both need that) but not to appear on any monitor, so Shell places the + // window far outside every screen instead of minimizing or hiding it. See Shell_Load. + internal static bool StartupHeadless { get; private set; } + // Control port of the dev launcher (scripts/watchBloomExe.mjs) that started // this Bloom, passed as --launcher-port. When present, DevLauncher watches for // pending C# changes and offers a dev-only toast that asks the launcher to @@ -120,6 +125,7 @@ static class Program new[] { StartupAutomation ? "automation=true" : null, + StartupHeadless ? "headless=true" : null, StartupVitePort.HasValue ? $"vitePort={StartupVitePort.Value}" : null, StartupLauncherPort.HasValue ? $"launcherPort={StartupLauncherPort.Value}" @@ -785,6 +791,7 @@ internal static string[] ParseStartupPortArguments(string[] args, out string err StartupVitePort = null; StartupLabel = null; StartupAutomation = false; + StartupHeadless = false; StartupLauncherPort = null; RunningE2eTests = false; @@ -826,6 +833,14 @@ out errorMessage value => StartupAutomation = value, out errorMessage ) + || TryHandleStartupFlagArgument( + args, + ref i, + "--headless", + () => StartupHeadless, + value => StartupHeadless = value, + out errorMessage + ) || TryHandleStartupFlagArgument( args, ref i, diff --git a/src/BloomExe/Shell.cs b/src/BloomExe/Shell.cs index 54cf046b558e..12f10b15b6a7 100644 --- a/src/BloomExe/Shell.cs +++ b/src/BloomExe/Shell.cs @@ -48,8 +48,10 @@ public static Form GetShellOrOtherOpenForm() private bool _finishedLoading; // During an automation run (--automation, e.g. the Playwright suites) the window must - // not steal the user's keyboard focus when it is shown. - protected override bool ShowWithoutActivation => Program.StartupAutomation; + // not steal the user's keyboard focus when it is shown. The same goes for a headless run + // (--headless), whose window sits off-screen where the user cannot see it at all. + protected override bool ShowWithoutActivation => + Program.StartupAutomation || Program.StartupHeadless; /// /// The screen that an automation run (--automation) should open windows on: the one @@ -72,6 +74,30 @@ public static Screen GetAutomationScreen() return Screen.PrimaryScreen; } + /// + /// Where a headless run (--headless) puts its window: the size of the automation screen's + /// working area, but positioned to the left of every monitor, so that not one pixel of it + /// is on any screen. + /// + /// The window is moved rather than minimized or hidden because a minimized WebView2 stops + /// painting: screenshots come back blank and the layout is the wrong size. An off-screen + /// window of the normal size keeps painting, so a test sees exactly what a user would. + /// + public static Rectangle GetHeadlessBounds() + { + var size = GetAutomationScreen().WorkingArea.Size; + // -32000 is as far left as a window may go: Windows still places a window there, and + // anything beyond about -32768 runs into the 16-bit coordinates that some of the older + // window messages still carry. Far enough left of every monitor that nothing shows, + // and unlike a margin computed from the screen layout it does not depend on Windows + // and this process agreeing about how many pixels wide a monitor is (they disagree + // when the monitors have different scale factors). + const int farLeftOfEveryScreen = -32000; + var leftmostX = Screen.AllScreens.Min(screen => screen.Bounds.Left); + var x = Math.Min(farLeftOfEveryScreen, leftmostX - size.Width - 1000); + return new Rectangle(x, 0, size.Width, size.Height); + } + public Shell( Func projectViewFactory, CollectionSettings collectionSettings, @@ -424,8 +450,10 @@ public static void ComeToFront() public void ReallyComeToFront() { // During an automation run, grabbing focus would yank the user's keyboard away - // from whatever they are doing on another monitor while tests run. - if (!Program.StartupAutomation) + // from whatever they are doing on another monitor while tests run. A headless + // window must not come to the front either: it is off-screen on purpose, and + // TopMost/BringToFront on it would take the foreground away for nothing. + if (!Program.StartupAutomation && !Program.StartupHeadless) { //try really hard to become top most. See http://stackoverflow.com/questions/5282588/how-can-i-bring-my-application-window-to-the-front TopMost = true; @@ -446,7 +474,18 @@ private void Shell_Load(object sender, EventArgs e) { SuspendLayout(); - if (Program.StartupAutomation) + if (Program.StartupHeadless) + { + // A headless run keeps the window off every screen and out of the task bar, + // so a test can run while the developer works. The window stays Normal (not + // minimized) and full size, because WebView2 only paints a window that is + // neither minimized nor hidden. See GetHeadlessBounds. + StartPosition = FormStartPosition.Manual; + WindowState = FormWindowState.Normal; + Bounds = GetHeadlessBounds(); + ShowInTaskbar = false; + } + else if (Program.StartupAutomation) { // An automation run must not open on whichever monitor the user is // currently working on, and must not disturb the saved window placement. @@ -468,7 +507,7 @@ private void Shell_Load(object sender, EventArgs e) // This feature is not yet a normal part of Bloom, since we think just maximizing is more rice-farmer-friendly. // However, we added the ability to remember this stuff at the request of the person making videos, who needs // Bloom to open in the same place / size each time. - if (Program.StartupAutomation) + if (Program.StartupAutomation || Program.StartupHeadless) { // Placement is already pinned above; leave the user's saved placement alone. } @@ -530,6 +569,10 @@ private void Shell_ResizeEnd(object sender, EventArgs e) return; if (WindowState != FormWindowState.Normal) return; + // A headless window is deliberately off every screen and is Normal rather than + // maximized, so saving its bounds would leave the developer's next Bloom invisible. + if (Program.StartupHeadless || Program.StartupAutomation) + return; Settings.Default.RestoreBounds = new Rectangle(Left, Top, Width, Height); Settings.Default.Save(); diff --git a/src/BloomExe/SplashScreen.cs b/src/BloomExe/SplashScreen.cs index aee6218cb1a9..3fcdf5caed90 100644 --- a/src/BloomExe/SplashScreen.cs +++ b/src/BloomExe/SplashScreen.cs @@ -29,13 +29,23 @@ public void FadeAndClose() } // During an automation run (--automation) the splash must not steal the user's - // keyboard focus when it is shown. - protected override bool ShowWithoutActivation => Program.StartupAutomation; + // keyboard focus when it is shown. Neither must a headless run (--headless), whose + // windows all sit off-screen. + protected override bool ShowWithoutActivation => + Program.StartupAutomation || Program.StartupHeadless; private SplashScreen() { InitializeComponent(); - if (Program.StartupAutomation) + if (Program.StartupHeadless) + { + // A headless run shows nothing on any monitor, so the splash goes off-screen with + // the main window. See Shell.GetHeadlessBounds. + StartPosition = FormStartPosition.Manual; + var headlessArea = Shell.GetHeadlessBounds(); + Location = new System.Drawing.Point(headlessArea.Left, headlessArea.Top); + } + else if (Program.StartupAutomation) { // An automation run must not open on whichever monitor the user is currently // working on. Center the splash on the automation screen instead. @@ -111,8 +121,9 @@ private void _fadeOutTimer_Tick(object sender, EventArgs e) private void SplashScreen_Load(object sender, EventArgs e) { // During an automation run, grabbing focus would yank the user's keyboard away - // from whatever they are doing on another monitor while tests run. - if (!Program.StartupAutomation) + // from whatever they are doing on another monitor while tests run. A headless splash + // is off-screen, so bringing it to the front would take the foreground for nothing. + if (!Program.StartupAutomation && !Program.StartupHeadless) { //try really hard to become top most. See http://stackoverflow.com/questions/5282588/how-can-i-bring-my-application-window-to-the-front TopMost = true; @@ -122,7 +133,7 @@ private void SplashScreen_Load(object sender, EventArgs e) _channelLabel.Visible = channel.ToLowerInvariant() != "release"; _channelLabel.Text = channel; // No need to localize this: seen only by testers or special users (BL-4451) _copyrightlabel.Text = $"© 2011-{DateTime.Now.Year} SIL Global"; - if (!Program.StartupAutomation) + if (!Program.StartupAutomation && !Program.StartupHeadless) BringToFront(); } diff --git a/src/BloomExe/WebView2Browser.cs b/src/BloomExe/WebView2Browser.cs index e47eb7f19b2c..b61a5ba4bb9b 100644 --- a/src/BloomExe/WebView2Browser.cs +++ b/src/BloomExe/WebView2Browser.cs @@ -309,6 +309,12 @@ CoreWebView2ContextMenuRequestedEventArgs e private static bool _useSharedEnvironment; private static CoreWebView2Environment _sharedEnvironment; + // The one environment every browser of an e2e run shares, so the run has a single browser + // process and therefore a single remote-debugging listener. See where it is used in + // InitWebView. Like the statics above it is unsynchronized, which is safe for the same + // reason: browser construction is marshalled to the UI thread. + private static CoreWebView2Environment _environmentForE2eTests; + public static void BeginSharedEnvironmentBatch() { AssertSharedEnvironmentStaticsAreUiThreadOnly(); @@ -396,6 +402,14 @@ private async Task InitWebView() { additionalBrowserArgs += " --accept-lang=" + _uiLanguageOfThisRun; } + if (Program.StartupHeadless) + { + // A headless run keeps Bloom's window far off-screen (see Shell.GetHeadlessBounds), + // so Windows reports the window as occluded and Chromium stops rendering it. A + // screenshot of an unrendered page comes back blank, so turn that behavior off. + featuresToDisable.Add("CalculateNativeWinOcclusion"); + additionalBrowserArgs += " --disable-backgrounding-occluded-windows"; + } if (RemoteDebuggingPort.HasValue && !Program.RunningUnitTests) { // Expose a CDP endpoint so Playwright and other automation can attach to the real Bloom WebView2 surface. @@ -475,6 +489,21 @@ private async Task InitWebView() // - _sharedEnvironment: the legacy on-UI-thread shared-environment batch (BookProcessor's old path). // Otherwise we fall through and create a fresh one. var env = _injectedEnvironment ?? (_useSharedEnvironment ? _sharedEnvironment : null); + // An e2e run attaches a test to ONE of these browser processes over the remote debugging + // port, and every environment we create is given that same port number, so only the + // process that starts first can listen on it. Which one that is depends on startup + // timing, so a test could attach to a browser Bloom is not driving: its scripts appeared + // to run (ExecuteScriptAsync reported success against the browser Bloom does drive) + // while the document the test was watching never changed. One environment for the whole + // run means one browser process, one listener, and every document visible to the test. + // + // Only for browsers built on the UI thread, which is every browser a test can see. A + // CoreWebView2Environment belongs to the thread that created it, so handing this one to + // a browser built on a server thread hangs that thread: publishing a BloomPUB, which + // makes its browsers on the thread serving the API call, waited forever and the preview + // never appeared. + if (env == null && Program.RunningE2eTests && Program.RunningOnUiThread) + env = _environmentForE2eTests; if (env == null) { string dataFolder; @@ -494,6 +523,8 @@ private async Task InitWebView() ); if (_useSharedEnvironment) _sharedEnvironment = env; + if (Program.RunningE2eTests && Program.RunningOnUiThread) + _environmentForE2eTests = env; } await _webview.EnsureCoreWebView2Async(env); // Added as a footnote to BL-15466 to prevent popups generated from title diff --git a/src/BloomExe/Workspace/WorkspaceView.cs b/src/BloomExe/Workspace/WorkspaceView.cs index 8707ea3666c2..f713289c5e5f 100644 --- a/src/BloomExe/Workspace/WorkspaceView.cs +++ b/src/BloomExe/Workspace/WorkspaceView.cs @@ -179,6 +179,8 @@ TeamCollectionApi teamCollectionApi _workspaceReactControl.BrowserCreated += (unused, args) => { _mainBrowser = _workspaceReactControl.Browser; + if (Program.RunningE2eTests) + MainBrowserForE2eTests = _mainBrowser; _mainBrowser?.SetBuiltInBrowserZoomEnabled(false); _editingView.InitializeMainBrowserForEditMode(); MaybeOpenMainBrowserDevTools(); @@ -281,6 +283,14 @@ protected override void OnLoad(EventArgs e) ); // possibility of error message boxes (BL-12155) } + /// + /// The browser holding the workspace root document that Bloom drives: the one whose page + /// iframe the Edit tab navigates. Set only under --e2e, for the e2e/shellUrl endpoint. + /// More than one document in a run carries the workspace root's markup, and a test that + /// attaches to the wrong one sees its own clicks work while nothing Bloom does arrives. + /// + internal static Browser MainBrowserForE2eTests { get; private set; } + internal void ReloadWorkspaceRootDocument() { _workspaceReactControl?.Reload(); diff --git a/src/BloomExe/web/controllers/E2eTestingApi.cs b/src/BloomExe/web/controllers/E2eTestingApi.cs index c0d3e3da4f77..22fe8efce6bb 100644 --- a/src/BloomExe/web/controllers/E2eTestingApi.cs +++ b/src/BloomExe/web/controllers/E2eTestingApi.cs @@ -1,3 +1,4 @@ +using System; using System.Linq; using Bloom.Api; using Bloom.Book; @@ -97,6 +98,21 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) false // does not need the UI thread ); + // POST body is a JSON array of one to three language tags, e.g. ["en","fr","es"], for + // Language1, Language2 and Language3. Sets the collection's languages and writes the + // .bloomCollection file, which a test otherwise has to compose by hand: the Collection + // Settings dialog is a WinForms surface CDP cannot reach, and its own + // collectionSettings/changeLanguage endpoint only answers while that dialog is open. + // Changing a collection's languages still needs the collection to be reopened, exactly + // as it does for a user who clicks OK in that dialog, so the caller must restart Bloom + // (src/BloomE2E/helpers/collection.ts setCollectionLanguages does both). + // Must run on the UI thread because it changes the settings the UI is showing. + apiHandler.RegisterEndpointHandler( + kApiUrlPart + "setCollectionLanguages", + HandleSetCollectionLanguages, + true + ); + // GET returns the pages the Add Page dialog would offer for the selected book: the // path of its template book, and the id and label of each template page. A test needs // these to call the production "addPage" endpoint, and the dialog itself reads them @@ -106,6 +122,24 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) HandleGetTemplatePages, false // does not need the UI thread ); + + // GET returns the URL of the workspace root document Bloom drives, or an empty string + // before that browser exists. A run has more than one document carrying the workspace + // root's markup, so the top bar's test id alone does not identify the right one, and a + // test that attaches to the wrong one is silently broken: its own typing and clicking + // work, while every page Bloom loads goes somewhere it cannot see. Compare on the file + // name, which is unique per document; the rest of the URL is escaped differently by + // Bloom and by the debugging protocol. Needs the UI thread to read the browser. + apiHandler.RegisterEndpointHandler(kApiUrlPart + "shellUrl", HandleGetShellUrl, true); + } + + /// + /// Reply with the URL of the workspace root document Bloom drives (see the registration + /// above), or an empty string if the main browser is not up yet. + /// + private void HandleGetShellUrl(ApiRequest request) + { + request.ReplyWithText(Workspace.WorkspaceView.MainBrowserForE2eTests?.Url ?? ""); } /// @@ -183,6 +217,76 @@ private void HandleGetTemplatePages(ApiRequest request) request.ReplyWithJson(pages); } + /// + /// Set the collection's Language1, Language2 and Language3 to the tags in the POST body (a + /// JSON array of one to three tags), and save the .bloomCollection file. Fewer than three + /// tags leaves the languages that were not named empty, except that a collection naming + /// only one language repeats it as Language2, which is what Bloom's own new-collection code + /// writes. + /// + /// This does the same work as clicking OK in the Collection Settings dialog, including + /// keeping a language that is no longer one of the first three in the collection's list of + /// languages. Like that dialog, it needs the collection reopened before the change is + /// everywhere it should be; the caller restarts Bloom. + /// + private void HandleSetCollectionLanguages(ApiRequest request) + { + var tags = request.RequiredPostObject(); + if (tags == null || tags.Length < 1 || tags.Length > 3) + throw new ArgumentException( + "e2e/setCollectionLanguages takes a JSON array of one to three language tags." + ); + if (string.IsNullOrWhiteSpace(tags[0])) + throw new ArgumentException( + "e2e/setCollectionLanguages needs a tag for Language1." + ); + + // Start from the collection's own writing systems, so that everything about each + // language except its tag (the font, the line height, the writing direction) keeps the + // value it had, then put the requested tag on each one. + // + // Each language is named in English ("French", not "français"), which is what a person + // gets by keeping the English name Bloom's language chooser offers. Bloom calls a name + // that is not the language's own name for itself a custom name, and a custom name is + // the one thing it shows verbatim everywhere; leaving the name uncustomized would make + // each screen name the language in whatever language it liked. + var pending = new WritingSystem[3]; + for (var i = 0; i < 3; i++) + { + pending[i] = _collectionSettings.AllLanguages[i].Clone(); + var tag = i < tags.Length ? tags[i].Trim() : string.Empty; + pending[i].ChangeTag(tag); + if (!string.IsNullOrEmpty(tag)) + { + // ChangeTag has already replaced the name with the one the language uses for + // itself, but it leaves IsCustomName alone. So clear that flag before asking + // for the English name: a language that arrived here with a custom name would + // otherwise be told "you have a name a person chose", and hand back the name + // ChangeTag just computed instead of the English one. + pending[i].SetName(pending[i].Name, false); + pending[i].SetName(pending[i].GetNameInLanguage("en"), true); + } + } + if (string.IsNullOrEmpty(pending[1].Tag)) + { + pending[1].ChangeTag(pending[0].Tag); + // Same reason as the loop above: ChangeTag leaves IsCustomName alone, so a + // Language2 that arrived here with a custom name would keep the name of the + // language it just replaced. + pending[1].SetName(pending[1].Name, false); + pending[1].SetName(pending[1].GetNameInLanguage("en"), true); + } + + CollectionSettingsDialog.UpdateLanguageSettings( + _collectionSettings.AllLanguages, + pending, + pending.Select(language => language.FontName).ToArray() + ); + _collectionSettings.Save(); + + request.PostSucceeded(); + } + /// /// Stage the currently selected book as a BloomPUB and reply with the localhost URL of the /// staged .htm file, which a test can load in bloom-player. diff --git a/src/BloomExe/web/controllers/EditingViewApi.cs b/src/BloomExe/web/controllers/EditingViewApi.cs index 0935efc49de0..858109699605 100644 --- a/src/BloomExe/web/controllers/EditingViewApi.cs +++ b/src/BloomExe/web/controllers/EditingViewApi.cs @@ -320,11 +320,22 @@ private void HandleSetCustomPageLayout(ApiRequest request) ); } + /// + /// Show the page whose id is in the POST body. The reply comes after asking the model, so + /// that a jump the Edit tab cannot do is reported as a failure rather than as a success + /// that shows nothing. EditingModel.JumpToPage queues a jump that arrives at an awkward + /// moment, so a caller does not have to ask twice. + /// + /// A failure here is a matter of timing, not of anything a user could put right, so the + /// front end posts to this endpoint with error reporting turned off. + /// private void HandleJumpToPage(ApiRequest request) { var pageId = request.GetPostStringOrNull(); - request.PostSucceeded(); - View.Model.SaveThen(() => pageId, () => { }); + if (View.Model.JumpToPage(pageId)) + request.PostSucceeded(); + else + request.Failed($"The edit tab could not show page {pageId}."); } /// diff --git a/src/BloomTests/ProgramTests.cs b/src/BloomTests/ProgramTests.cs index f4c68c636f68..4dd021366f6c 100644 --- a/src/BloomTests/ProgramTests.cs +++ b/src/BloomTests/ProgramTests.cs @@ -56,6 +56,33 @@ out var automationErrorMessage Assert.That(Program.StartupRequestedPortSummary, Is.EqualTo("automation=true")); } + [Test] + public void ParseStartupPortArguments_StoresHeadlessFlag() + { + var remainingArgs = Program.ParseStartupPortArguments( + new[] { "--automation", "--headless", @"C:\Temp\Example.bloomcollection" }, + out var errorMessage + ); + + Assert.That(errorMessage, Is.Null); + Assert.That(Program.StartupHeadless, Is.True); + Assert.That(Program.StartupAutomation, Is.True); + Assert.That( + Program.StartupRequestedPortSummary, + Is.EqualTo("automation=true, headless=true") + ); + Assert.That(remainingArgs, Is.EqualTo(new[] { @"C:\Temp\Example.bloomcollection" })); + } + + [Test] + public void ParseStartupPortArguments_LeavesHeadlessFalseWithoutTheFlag() + { + Program.ParseStartupPortArguments(new[] { "--automation" }, out var errorMessage); + + Assert.That(errorMessage, Is.Null); + Assert.That(Program.StartupHeadless, Is.False); + } + [Test] public void ParseStartupPortArguments_VitePortAloneDoesNotEnableAutomation() { diff --git a/src/BloomVisualRegressionTests/index.spec.ts b/src/BloomVisualRegressionTests/index.spec.ts index c755e4d612de..c28a5bbe195d 100644 --- a/src/BloomVisualRegressionTests/index.spec.ts +++ b/src/BloomVisualRegressionTests/index.spec.ts @@ -201,6 +201,12 @@ describe("All books", () => { let playerPage: Page; let browser: Browser; let context: BrowserContext; + // Every image comparison the CURRENT case has failed. A case compares one book-preview image + // and one image per bloom-player page, and a stale baseline usually affects several of them. + // Throwing on the first mismatch meant the later comparisons never even captured their + // images, so accepting a real layout change took one ~3-minute run per image (BL-16638 took + // three rounds). So collect them all and fail once, at the end of the case. + let comparisonFailures: string[] = []; beforeAll(async () => { await launchDedicatedBloom(); @@ -275,6 +281,7 @@ describe("All books", () => { }); test.each(cases)("$title", async (testCase) => { + comparisonFailures = []; // Park the capture pages before we mutate this book. Otherwise the previous case's still-open // book-preview / bloom-player page keeps requesting book and staged-BloomPUB files while this // case rewrites them, which caused mid-run "file is being used by another process" and @@ -312,6 +319,14 @@ describe("All books", () => { await selectTab("publish"); const stagedUrl = await makeBloomPubPreview(); await capturePlayerPages(stagedUrl, testCase.label, screenshotsDir); + + // One failure for the whole case, listing every image that did not match, so a single run + // shows all the baselines that need looking at. + if (comparisonFailures.length > 0) + throw new Error( + `${comparisonFailures.length} of this case's images did not match their ` + + `reference:\n ${comparisonFailures.join("\n ")}`, + ); }); // Create the reference image if it does not exist yet; otherwise capture a current image and @@ -619,10 +634,29 @@ describe("All books", () => { } } + // Compare one captured image against its reference. This does NOT throw on a mismatch: it + // appends a description to comparisonFailures, and the test body fails the case once it has + // compared every image. Anything thrown while comparing (notably Pixelmatch's "Image sizes do + // not match", which is itself a real failure) is recorded the same way. async function comparePreviewImage( referencePath: string, testPath: string, diffPath: string, + ) { + try { + await compareOrThrow(referencePath, testPath, diffPath); + } catch (error) { + comparisonFailures.push( + `${testPath}: ${error instanceof Error ? error.message : String(error)}`, + ); + } + } + + // The comparison itself: write a diff image and throw when the two images differ at all. + async function compareOrThrow( + referencePath: string, + testPath: string, + diffPath: string, ) { const referenceImage = PNG.sync.read(fs.readFileSync(referencePath)); const testImage = PNG.sync.read(fs.readFileSync(testPath)); @@ -652,7 +686,10 @@ describe("All books", () => { `If the new version is correct, replace ${referencePath} with ${testPath}`, ), ); - expect(numberOfDifferentPixels).toBe(0); + throw new Error( + `differed from ${referencePath} by ${numberOfDifferentPixels} pixels; ` + + `the diff image is at ${diffPath}`, + ); } } }); From cd2d58050d04cc13094378e1813448bdf9ea56c9 Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 2 Sep 2026 07:44:09 -0600 Subject: [PATCH 02/10] Reject a malformed process id instead of killing every Bloom killBloomProcess.mjs read --pid and --watch-pid with Number(), so "--pid abc", "--pid" with no value, and "--pid 0" all produced NaN. NaN is falsy, so the script decided no target had been named and fell through to its no-target behavior, which is to kill every Bloom this worktree owns. A mistyped process id therefore threw away unsaved edits in every Bloom on the machine. A new requireProcessIdOption in bloomProcessCommon.mjs validates all four spellings of the two options at parse time and exits non-zero before the script looks at any process. The "did the caller name a target?" test now asks whether the option was supplied rather than whether its value is truthy, so another target option cannot reintroduce this. Also, captureElement now says on standard error when it cannot clear the window size override, rather than swallowing the failure: the module promises the override is always cleared, and a run that silently kept an 8000-pixel window would make every later test in that worker see the wrong Bloom. Co-Authored-By: Claude Opus 5 (1M context) --- .../bloom-automation/bloomProcessCommon.mjs | 22 ++++++++++++++ .../bloom-automation/killBloomProcess.mjs | 29 +++++++++++++++---- src/BloomE2E/helpers/screenshot.ts | 9 +++++- 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/.github/skills/bloom-automation/bloomProcessCommon.mjs b/.github/skills/bloom-automation/bloomProcessCommon.mjs index 21d2784acc66..db8d7735ee7b 100644 --- a/.github/skills/bloom-automation/bloomProcessCommon.mjs +++ b/.github/skills/bloom-automation/bloomProcessCommon.mjs @@ -42,6 +42,28 @@ export const requireTcpPortOption = (optionName, value) => { return port; }; +/** + * A process id given on the command line, as a positive integer. + * + * This throws rather than returning undefined, because the caller that wants a process id wants + * to act on exactly that process. A killer script that read a malformed id as "no id given" would + * fall through to whatever its no-target default is, and killBloomProcess.mjs's default is to kill + * every Bloom the worktree owns. + */ +export const requireProcessIdOption = (optionName, value) => { + const normalized = value === undefined ? "" : String(value).trim(); + const processId = /^\d+$/.test(normalized) + ? toPositiveInteger(normalized) + : undefined; + if (!processId) { + throw new Error( + `${optionName} must be a positive integer process id. Received: ${value}`, + ); + } + + return processId; +}; + export const requireOptionValue = (args, index, optionName) => { const value = args[index + 1]; if (!value || value.startsWith("--")) { diff --git a/.github/skills/bloom-automation/killBloomProcess.mjs b/.github/skills/bloom-automation/killBloomProcess.mjs index f68a8fc35873..6b5ffdc08775 100644 --- a/.github/skills/bloom-automation/killBloomProcess.mjs +++ b/.github/skills/bloom-automation/killBloomProcess.mjs @@ -7,6 +7,7 @@ import { killProcessIds, normalizeBloomInstanceInfo, requireOptionValue, + requireProcessIdOption, requireTcpPortOption, } from "./bloomProcessCommon.mjs"; @@ -89,24 +90,36 @@ const parseArgs = () => { } if (arg === "--pid") { - options.pid = Number(args[i + 1]); + options.pid = requireProcessIdOption( + "--pid", + requireOptionValue(args, i, "--pid"), + ); i++; continue; } if (arg.startsWith("--pid=")) { - options.pid = Number(arg.slice("--pid=".length)); + options.pid = requireProcessIdOption( + "--pid", + arg.slice("--pid=".length), + ); continue; } if (arg === "--watch-pid") { - options.watchPid = Number(args[i + 1]); + options.watchPid = requireProcessIdOption( + "--watch-pid", + requireOptionValue(args, i, "--watch-pid"), + ); i++; continue; } if (arg.startsWith("--watch-pid=")) { - options.watchPid = Number(arg.slice("--watch-pid=".length)); + options.watchPid = requireProcessIdOption( + "--watch-pid", + arg.slice("--watch-pid=".length), + ); continue; } @@ -119,8 +132,14 @@ const parseArgs = () => { const options = parseArgs(); const processState = classifyProcesses(options.repoRoot); const processIds = new Set(); +// Whether the caller named a target, not whether the value we parsed from it is usable. The +// two are the same now that every target option is validated at parse time, and this says the +// intended thing: a caller who asked for one process must never reach the default that kills +// every Bloom this worktree owns. const exactTargetRequested = - !!options.httpPort || !!options.pid || !!options.watchPid; + options.httpPort !== undefined || + options.pid !== undefined || + options.watchPid !== undefined; let targetedInstance; let exactTargetResolutionError; diff --git a/src/BloomE2E/helpers/screenshot.ts b/src/BloomE2E/helpers/screenshot.ts index 58aa5c2dbd77..34cac8bb3e58 100644 --- a/src/BloomE2E/helpers/screenshot.ts +++ b/src/BloomE2E/helpers/screenshot.ts @@ -135,11 +135,18 @@ export async function captureElement( } finally { // Always, including after a failed capture: the next test in this worker drives the same // Bloom, and an emulated 8000-pixel window would make everything it sees wrong. + // Say so if the clear fails, rather than throwing: we may be here because the capture + // failed, and that error is the one the test needs to see. But the run is now driving a + // Bloom of the wrong size, so it must not pass in silence. await sendWithTimeout( session, "Emulation.clearDeviceMetricsOverride", {}, - ).catch(() => undefined); + ).catch((error) => + console.error( + `captureElement could not clear the window size override, so the rest of this worker's tests drive a Bloom of the wrong size: ${error}`, + ), + ); await session.detach().catch(() => undefined); } } From ef5542833bd17f37e8292b6370128cde1023b948 Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 2 Sep 2026 07:51:07 -0600 Subject: [PATCH 03/10] Fail the test when the window size override cannot be cleared captureElement enlarges Bloom's window so a whole book page fits, then puts it back in a finally. A failed put-back only wrote a line to standard error, so the test passed and every later test in that Playwright worker drove a Bloom several thousand pixels wide. Whatever those tests measured or clicked was measured against a window no user has. The failure now throws, so the test that made the window big is the test that fails. The exception is a clear failure that arrives while a capture error is already on its way out: that error is what the test needs to read, so throwing over it would hide why the capture failed. The code tells the two apart by whether an image was produced. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomE2E/helpers/screenshot.ts | 32 +++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/BloomE2E/helpers/screenshot.ts b/src/BloomE2E/helpers/screenshot.ts index 34cac8bb3e58..9430295ad050 100644 --- a/src/BloomE2E/helpers/screenshot.ts +++ b/src/BloomE2E/helpers/screenshot.ts @@ -82,6 +82,9 @@ export async function captureElement( await locator.waitFor({ state: "visible", timeout: timeoutMs }); const session = await page.context().newCDPSession(page); + // Set once the capture has produced an image, so the cleanup below can tell a clear failure + // that is the only thing wrong from one that is trailing a capture error. + let captured: IElementImage | undefined; try { // How big the window has to be for the whole element to be laid out at once. Ask for the // element's own scroll size, plus where it sits, rather than the document's size: the @@ -125,29 +128,40 @@ export async function captureElement( const png = Buffer.from(result.data, "base64"); const size = readPngSize(png); - return { + captured = { png, width: size.width, height: size.height, elementWidth: box.width, elementHeight: box.height, }; + return captured; } finally { // Always, including after a failed capture: the next test in this worker drives the same // Bloom, and an emulated 8000-pixel window would make everything it sees wrong. - // Say so if the clear fails, rather than throwing: we may be here because the capture - // failed, and that error is the one the test needs to see. But the run is now driving a - // Bloom of the wrong size, so it must not pass in silence. + let clearError: unknown; await sendWithTimeout( session, "Emulation.clearDeviceMetricsOverride", {}, - ).catch((error) => - console.error( - `captureElement could not clear the window size override, so the rest of this worker's tests drive a Bloom of the wrong size: ${error}`, - ), - ); + ).catch((error) => { + clearError = error; + }); await session.detach().catch(() => undefined); + + if (clearError !== undefined) { + // The window is still the size we made it, and the rest of this worker's tests would + // measure that Bloom rather than the real one. Fail rather than warn. + // + // Only when the capture itself succeeded, though. If we are in this block because the + // capture threw, that error is the one the test needs to read, and throwing here would + // replace it. In that case the message is all we can give. + const message = `captureElement could not clear the window size override, so the rest of this worker's tests would drive a Bloom of the wrong size: ${clearError}`; + if (captured) { + throw new Error(message); + } + console.error(message); + } } } From 64d0c4bbb3c27113320720e978e4353b9e10e9b3 Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 2 Sep 2026 08:58:02 -0600 Subject: [PATCH 04/10] Stop the e2e suite losing a page change to the Edit tab Bloom's Edit tab loses a request that changes the page when that request arrives while the tab is still loading a page. The suite hit this twice in publish-text-languages.spec.ts, once through goToPage and once through setContentLanguages, each time as a 60-second wait for something that had already been asked for. Bloom's own log shows the mechanism. A page announces that its DOM has loaded more than once. The first announcement releases the request that was queued while the tab navigated; that request asks the browser for the page content so it can save the page being left. The second announcement then arrives, Bloom refuses it because a save is in flight, and the browser never answers the save request. The tab stays in SavePending, so nothing more happens: the page never changes, and the tab switch that follows is held as well. Navigating(f45a2ef8) --> editing(f45a2ef8) Editing(f45a2ef8) --> savePending() Ignoring edit() request while in SavePending(f45a2ef8) Worked around for the suite by keeping every request out of that queue: - e2e/editState reports what the Edit tab is doing, which page it is about, whether it is showing, and how many times the page it shows has announced itself. Read-only, off the UI thread, and registered only under --e2e. - waitForEditTabSettled waits until the tab reads Editing twice, 1500 ms apart, on one page, with the announcement count unchanged. The state alone reads Editing between the two announcements, when a request would still be lost, and the gap between them has been seen to reach a second. - goToPage, setContentLanguages, addPage and duplicateCurrentPage wait for it before they ask for anything. No test can see any of this from the DOM: Bloom leaves the previous page in the frame while it loads the next one. goToPage's failure message now names the state the tab is stuck in. AUTOMATION-DEBT.md records the Bloom defect itself, which remains: a user hits it whenever something asks for a page change in that window, and the Edit tab then stops accepting page changes altogether. Both fix directions change production save behavior, so that is a decision rather than a quiet fix. Also in this commit, the fixes for the remaining review findings on this branch: - captureElement refuses an element that needs a window larger than the cap, rather than returning a truncated image nothing downstream could detect, and fails rather than warns when it cannot clear the window size override. - goToPage reads the driven shell URL forgivingly, so a failing request cannot replace the diagnostic the message exists to give. - Only an environment that carries a debugging port is kept for later UI-thread browsers, so a browser built before BloomServer had its port cannot leave the whole run unable to listen. - findShellPage re-checks a marker-only match before returning it; it may have been found ninety seconds earlier, and Bloom navigates the shell while it starts up. - typeInGroup says what keyboard.insertText does not do, and AUTOMATION-DEBT.md records that no test that types exercises anything in Bloom that listens for a key. - Under --headless the problem-report screenshot renders the window rather than copying from screen coordinates that sit outside every monitor. Validated: e2e 9/9 twice, 3313 C# tests, e2e typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomE2E/AUTOMATION-DEBT.md | 59 +++++++++++ src/BloomE2E/fixtures/bloomTest.ts | 12 +++ src/BloomE2E/helpers/bookMaking.ts | 100 ++++++++++++++++-- src/BloomE2E/helpers/screenshot.ts | 24 ++++- src/BloomExe/Edit/EditingModel.cs | 44 ++++++++ src/BloomExe/Edit/EditingStateMachine.cs | 12 +++ src/BloomExe/WebView2Browser.cs | 12 ++- src/BloomExe/web/controllers/E2eTestingApi.cs | 34 ++++++ .../web/controllers/ProblemReportApi.cs | 8 +- 9 files changed, 295 insertions(+), 10 deletions(-) diff --git a/src/BloomE2E/AUTOMATION-DEBT.md b/src/BloomE2E/AUTOMATION-DEBT.md index 95de29bf21f2..8b7fa67c3271 100644 --- a/src/BloomE2E/AUTOMATION-DEBT.md +++ b/src/BloomE2E/AUTOMATION-DEBT.md @@ -158,6 +158,22 @@ setting the value leaves CKEditor in a state Bloom then saves correctly. Fix dir an `e2e/` hook, or a supported CKEditor path, that sets the text of one box outright. (Found 2026-09-01 automating Test Case ID 169.) +## Typing in a text box raises no key events + +`typeInGroup` puts the whole string in with `keyboard.insertText`, which raises `input` +and nothing else. So no test that types exercises anything in Bloom that listens for +`keydown`, `keypress` or `keyup`, and the `toHaveText` check that follows cannot tell the +difference: the text arrives either way. The pieces of Bloom that watch for a particular +key, rather than for a change to the text, are therefore not covered by any test that +types. + +This is a deliberate trade for speed, taken because a key press per character made every +test that fills a book slower in proportion to how much it typed. Fix direction: a helper +that presses one named key in a box, for the tests whose subject is the key press itself +(Enter splitting a paragraph, Tab moving between boxes, a shortcut), and a note in that +helper that `typeInGroup` is not the way to test those. (Found 2026-09-02, during the +review of the headless work.) + ## A test can attach to a shell document Bloom does not drive More than one document in a run carries the workspace root's markup, and therefore the @@ -185,3 +201,46 @@ What remains: nobody knows why a run has a second workspace root document at all creates one `_workspaceReactControl`. Worth finding, because the duplicate is what makes the test-side check necessary. (Found 2026-09-01 while making `jumpToPage` queue a jump.) + +## A page change asked for while the Edit tab is still loading a page can be lost + +`editView/jumpToPage` accepts a jump that arrives while the Edit tab is navigating, and +`EditingStateMachine.DeferUntilPageIsLoaded` runs it once that page loads +(`EditingModel.HandlePageDomLoadedEvent`). That queued jump is sometimes lost, and the +caller waits 60 seconds for a page that never comes. + +The cause is a page that announces itself to Bloom twice. Bloom's own log, from the run +that found this on 2026-09-02: + +``` +Navigating(f45a2ef8) --> editing(f45a2ef8) +Editing(f45a2ef8) --> savePending() +Ignoring edit() request while in SavePending(f45a2ef8) +``` + +The first announcement moves the tab to Editing and releases the queued jump, which asks +the browser for the page content so it can save the page it is leaving. The second +announcement then arrives, is refused because a save is in flight, and the browser never +answers the save request. The tab stays in SavePending, so the jump never navigates. The +same three lines in the order that works read `--> editing`, `Ignoring edit()`, then +`--> savePending`, and the save completes. + +The same thing happens to any request that changes the page, not only a jump. The run that +found it lost a language change from `setContentLanguages`, and the Edit tab then held the +tab switch that followed, so a Publish test failed instead. + +Worked around for tests, 2026-09-02: `e2e/editState` reports what the Edit tab is doing and +how many times the page it shows has announced itself, and every helper that changes the +page waits for `waitForEditTabSettled` first. That keeps the request out of the queue. The +wait reads the state twice, 1500 ms apart, and needs Editing both times with the count +unchanged: the state alone reads Editing between the two announcements, when a request +would still be lost, and the gap between them has been seen to reach a second. No test can +see any of this from the DOM, because Bloom leaves the previous page in the frame while it +loads the next one. + +What remains: the Bloom defect itself. A user hits it whenever something asks for a page +change in that window, and the Edit tab then stops responding to page changes altogether. +Fix direction: either stop the browser announcing a page twice, or make the state machine +treat a second announcement of the page it is saving as a reason to discard that save +(`DiscardInFlightSave` already exists for BL-16766). Both change production save behavior, +so this needs a decision rather than a quiet fix. diff --git a/src/BloomE2E/fixtures/bloomTest.ts b/src/BloomE2E/fixtures/bloomTest.ts index 655833af8016..7668f16d492c 100644 --- a/src/BloomE2E/fixtures/bloomTest.ts +++ b/src/BloomE2E/fixtures/bloomTest.ts @@ -190,6 +190,18 @@ async function findShellPage( } await delay(500); } + // Re-check it before handing it back. It was found on some earlier turn of the loop, possibly + // ninety seconds ago, and Bloom navigates the shell target while it starts up, so by now the + // page may be gone. + if (markerOnlyMatch) { + const stillThere = await markerOnlyMatch + .evaluate( + (marker) => !!document.querySelector(marker), + SHELL_MARKER, + ) + .catch(() => false); + if (!stillThere) markerOnlyMatch = undefined; + } if (markerOnlyMatch && !hookEverAnswered) { console.warn( `Bloom never answered ${SHELL_URL_ENDPOINT}, so the shell document was chosen by ` + diff --git a/src/BloomE2E/helpers/bookMaking.ts b/src/BloomE2E/helpers/bookMaking.ts index 8a17b954b295..ca8bc4655e22 100644 --- a/src/BloomE2E/helpers/bookMaking.ts +++ b/src/BloomE2E/helpers/bookMaking.ts @@ -145,6 +145,9 @@ export async function setContentLanguages( page: Page, tags: string[], ): Promise { + // Each change makes Bloom reload the page, so this is a page-changing request and must not + // arrive while the Edit tab is still loading one. See waitForEditTabSettled. + await waitForEditTabSettled(page); const usage = await apiGetJson( page, "editView/topBar/contentLanguageUsage", @@ -178,7 +181,7 @@ export async function setContentLanguages( ) .toBe(wanted); } - await waitForEditablePage(page); + await waitForEditTabSettled(page); } /** The Edit tab's frame holding the page being edited. Throws if the Edit tab is not showing. */ @@ -218,6 +221,67 @@ export async function waitForEditablePage( .toBeGreaterThan(0); } +/** What e2e/editState replies with: what the Edit tab is doing. */ +interface IEditTabState { + /** A name from Bloom's State enum: NoPage, Navigating, Editing, SavePending, SavedAndStripped. */ + state: string; + /** The page that state is about, or "". */ + pageId: string; + /** Whether the Edit tab is the tab being shown. */ + visible: boolean; + /** How many times the page now shown has told Bloom its DOM had loaded. */ + pageLoadAnnouncements: number; +} + +/** + * Wait until the Edit tab is settled on a page: showing it, in the Editing state, and done + * announcing that the page has loaded. + * + * Ask this before any request that changes the page. The Edit tab defers such a request while it + * navigates, and a deferred one is lost (AUTOMATION-DEBT.md: "A page change asked for while the + * Edit tab is still loading a page can be lost"). A page announces itself to Bloom more than once, + * and a request that arrives between two of those announcements is lost as well, so waiting for + * the Editing state alone is not enough. Hence the second reading: the tab is settled once the + * count of announcements has stopped rising. + * + * The DOM is no help here. Bloom leaves the previous page in the frame while it loads the next + * one, so a test looking at the frame sees a settled Edit tab throughout. + */ +export async function waitForEditTabSettled( + page: Page, + timeoutMs = 90000, +): Promise { + const editState = () => apiGetJson(page, "e2e/editState"); + await expect + .poll( + async () => { + const first = await editState(); + if (!first.visible || first.state !== "Editing") + return first.state; + // Long enough to cover the gap between two announcements of one page load, which + // has been seen to reach a second. + await page.waitForTimeout(1500); + const second = await editState(); + if ( + second.state !== "Editing" || + second.pageId !== first.pageId + ) + return second.state; + if ( + second.pageLoadAnnouncements !== first.pageLoadAnnouncements + ) + return "Navigating"; + return "Editing"; + }, + { + timeout: timeoutMs, + message: + "The Edit tab never settled on a page, so a request to change pages could be lost.", + }, + ) + .toBe("Editing"); +} + /** One page of the selected book, as e2e/pages reports it. */ export interface IBookPage { /** The page's id, which is what editView/jumpToPage takes. */ @@ -297,7 +361,7 @@ export async function addPage( message: `Bloom never added the "${templatePageLabel}" page(s).`, }) .toBe(before + times); - await waitForEditablePage(page); + await waitForEditTabSettled(page); } /** @@ -305,9 +369,13 @@ export async function addPage( * it is leaving, so text typed into a box reaches the file only once the book moves off that page. */ export async function goToPage(page: Page, pageId: string): Promise { - // One request is enough: the Edit tab queues a jump that arrives while it is loading a page or - // saving one, and answers with an error if it can do neither (EditingModel.JumpToPage). It used - // to drop such a jump and report success, which is why this helper used to ask three times. + // Wait for the Edit tab first. It queues a jump that arrives while it is loading a page, and + // one of those queued jumps is lost (see waitForEditTabSettled). Asking at a moment when the + // tab acts on the jump at once avoids the whole queue. + await waitForEditTabSettled(page); + // Then one request is enough: the tab answers with an error if it can neither jump nor queue + // (EditingModel.JumpToPage). It used to drop such a jump and report success, which is why this + // helper used to ask three times. await apiPost(page, "editView/jumpToPage", pageId, "text/plain"); const showing = async () => (await page @@ -327,12 +395,24 @@ export async function goToPage(page: Page, pageId: string): Promise { .evaluateAll((pages) => pages.map((p) => p.id)) .catch(() => ["(could not read the frame)"]) : []; + // Read this the forgiving way. A shell document Bloom is not driving is one of the + // failures this message exists to explain, and in exactly that case the request can + // fail. Letting it throw here would replace the whole diagnostic with its own error. + const drivenShell = await apiGet(page, "e2e/shellUrl") + .then((response) => response.body) + .catch((error) => `(could not be read: ${error})`); + // The state the Edit tab is stuck in says which of the two failures this is: a jump that + // was lost leaves the tab in SavePending or Navigating on the page it already had. + const editState = await apiGet(page, "e2e/editState") + .then((response) => response.body) + .catch((error) => `(could not be read: ${error})`); throw new Error( `Bloom never showed page ${pageId} in the Edit tab. ` + + `The Edit tab is at ${editState}. ` + (frame ? `The 'page' frame is at ${frame.url()} and holds [${heldPageIds.join(", ")}].` : `There is no 'page' frame.`) + - ` Bloom drives the shell at ${(await apiGet(page, "e2e/shellUrl")).body}; ` + + ` Bloom drives the shell at ${drivenShell}; ` + `this test is watching ${page.url()}. If those name different documents, the test ` + `attached to the wrong one and nothing Bloom does will ever appear (see AUTOMATION-DEBT.md).`, ); @@ -360,7 +440,7 @@ export async function duplicateCurrentPage( message: "Bloom never added the duplicated page(s).", }) .toBe(before + times); - await waitForEditablePage(page); + await waitForEditTabSettled(page); } /** @@ -388,6 +468,12 @@ export async function typeInGroup( // One insertion rather than a key press per character: the box has focus, and CKEditor and // Bloom's own markup code both work from the input event this raises, so the result is the // same and the cost does not grow with the length of the text. + // + // What this does NOT do is raise keydown, keypress or keyup. So a test that types here does + // not exercise anything in Bloom that listens for a key rather than for input, and the + // assertion below cannot tell the difference. A test whose subject IS a key press needs a + // helper of its own that presses that key. (AUTOMATION-DEBT.md: "Typing in a text box raises + // no key events".) if (text) await page.keyboard.insertText(text); // Bloom's editor reacts to typing; confirm the box holds what we meant before moving on, so a // later failure cannot be blamed on text that never arrived. diff --git a/src/BloomE2E/helpers/screenshot.ts b/src/BloomE2E/helpers/screenshot.ts index 9430295ad050..16d4da8a644f 100644 --- a/src/BloomE2E/helpers/screenshot.ts +++ b/src/BloomE2E/helpers/screenshot.ts @@ -90,6 +90,22 @@ export async function captureElement( // element's own scroll size, plus where it sits, rather than the document's size: the // document includes page-list thumbnails and other chrome we are not capturing. const wanted = await elementExtent(locator, timeoutMs); + // An element bigger than the cap would be clipped to its full box against a window that + // was never made large enough, so the image would be a truncated element rather than a + // failure, and nothing downstream could tell: the box we measure afterwards is measured + // inside the too-small window, so it agrees with the truncated image. Say so instead. + if ( + wanted.right > MAX_OVERRIDE_PIXELS || + wanted.bottom > MAX_OVERRIDE_PIXELS + ) { + throw new Error( + `captureElement cannot capture this element: laying all of it out needs a window ` + + `${Math.ceil(wanted.right)}x${Math.ceil(wanted.bottom)} pixels, and the ` + + `largest this helper will ask WebView2 for is ${MAX_OVERRIDE_PIXELS}. ` + + `Capture a smaller part of it, or raise MAX_OVERRIDE_PIXELS if WebView2 can ` + + `still allocate that.`, + ); + } const viewport = page.viewportSize(); const overrideWidth = clampOverride( Math.max(wanted.right, viewport?.width ?? 0), @@ -165,7 +181,13 @@ export async function captureElement( } } -/** Keep an override within what WebView2 can reasonably allocate, and never ask for zero. */ +/** + * Keep an override within what WebView2 can reasonably allocate, and never ask for zero. + * + * The caller checks the element against MAX_OVERRIDE_PIXELS before calling this, so the cap here + * only ever applies to the viewport's own size. A silently capped element would be captured + * truncated rather than reported. + */ function clampOverride(pixels: number): number { return Math.max(1, Math.min(MAX_OVERRIDE_PIXELS, Math.ceil(pixels))); } diff --git a/src/BloomExe/Edit/EditingModel.cs b/src/BloomExe/Edit/EditingModel.cs index a8c945f16183..466cab0e88e9 100644 --- a/src/BloomExe/Edit/EditingModel.cs +++ b/src/BloomExe/Edit/EditingModel.cs @@ -117,6 +117,11 @@ ITemplateFinder sourceCollectionsList _server = server; _webSocketServer = webSocketServer; _sourceCollectionsList = sourceCollectionsList; + // A run has one EditingModel, and E2eTestingApi has no way to be handed it: the + // container builds that api before any book is selected. So hand it over here, the + // same way WorkspaceView hands over its browser for e2e/shellUrl. + if (Program.RunningE2eTests) + ModelForE2eTests = this; _stateMachine = new EditingStateMachine( // navigate, @@ -1705,6 +1710,35 @@ internal void SavePageAndReloadIt(ApiRequest request) request.PostSucceeded(); } + /// + /// The one EditingModel of an e2e run, or null outside such a run. See the constructor. + /// + public static EditingModel ModelForE2eTests; + + /// + /// What the Edit tab is doing, for the e2e suite to wait on (see e2e/editState). Editing + /// is the only state in which the tab accepts a request without deferring it. + /// + public State EditTabState => _stateMachine.CurrentState; + + /// + /// The page EditTabState is about, or null. + /// + public string EditTabStatePageId => _stateMachine.CurrentPageId; + + // How many times the page named by _announcedPageId has told us its DOM had loaded. See + // HandlePageDomLoadedEvent. + private string _announcedPageId; + private int _pageLoadAnnouncements; + + /// + /// How many times the page now being shown has announced that its DOM had loaded. A page + /// does that more than once, and a request that changes the page is lost if it arrives + /// between two of those announcements (see src/BloomE2E/AUTOMATION-DEBT.md). So the e2e + /// suite waits for this to stop rising. For automation only. + /// + public int PageLoadAnnouncements => _pageLoadAnnouncements; + /// /// Show the page with this id in the Edit tab. The page being left is saved on the way, /// which is how anything typed on it reaches the file. @@ -2256,6 +2290,16 @@ public UrlPathString AddWidgetFilesToBookFolder(string fullWidgetPath) public void HandlePageDomLoadedEvent(string pageId) { + // Count the announcement before acting on it. A page announces itself more than once, + // and the e2e suite has to know that the last one has been and gone before it asks + // this tab for anything (see the editState endpoint and PageLoadAnnouncements). + if (pageId != _announcedPageId) + { + _announcedPageId = pageId; + _pageLoadAnnouncements = 0; + } + _pageLoadAnnouncements++; + var nowEditing = _stateMachine.ToEditing(pageId); if (nowEditing) { diff --git a/src/BloomExe/Edit/EditingStateMachine.cs b/src/BloomExe/Edit/EditingStateMachine.cs index 6011ecfbf397..eb9fa2124689 100644 --- a/src/BloomExe/Edit/EditingStateMachine.cs +++ b/src/BloomExe/Edit/EditingStateMachine.cs @@ -151,6 +151,18 @@ public bool ToNoPage() /// public bool SavePending => _currentState == State.SavePending; + /// + /// The state we are in, and the page it is about. These exist for automation: the e2e suite + /// waits until the Edit tab is settled before it asks that tab for anything, and the DOM + /// cannot tell it that (see E2eTestingApi's editState endpoint). Read only. + /// + public State CurrentState => _currentState; + + /// + /// The page the current state is about, or null. See CurrentState. + /// + public string CurrentPageId => _pageId; + /// /// Called to initiate navigation to a new page (or the same one again). /// Should not be called when there are unsaved (or incompletely saved) changes. diff --git a/src/BloomExe/WebView2Browser.cs b/src/BloomExe/WebView2Browser.cs index b61a5ba4bb9b..7819ddfffe64 100644 --- a/src/BloomExe/WebView2Browser.cs +++ b/src/BloomExe/WebView2Browser.cs @@ -523,7 +523,17 @@ private async Task InitWebView() ); if (_useSharedEnvironment) _sharedEnvironment = env; - if (Program.RunningE2eTests && Program.RunningOnUiThread) + // Only keep it when it actually carries a debugging port. The port lives in the + // options, which are fixed when the environment is made, so an environment built + // before BloomServer had its port would have none, and every UI-thread browser + // after it would inherit that: no browser in the run would ever listen, and the + // suite would report a startup timeout rather than a reason. No browser is built + // that early today, and this keeps it that way if one ever is. + if ( + Program.RunningE2eTests + && Program.RunningOnUiThread + && RemoteDebuggingPort.HasValue + ) _environmentForE2eTests = env; } await _webview.EnsureCoreWebView2Async(env); diff --git a/src/BloomExe/web/controllers/E2eTestingApi.cs b/src/BloomExe/web/controllers/E2eTestingApi.cs index 22fe8efce6bb..d9644bed5adf 100644 --- a/src/BloomExe/web/controllers/E2eTestingApi.cs +++ b/src/BloomExe/web/controllers/E2eTestingApi.cs @@ -131,6 +131,40 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) // name, which is unique per document; the rest of the URL is escaped differently by // Bloom and by the debugging protocol. Needs the UI thread to read the browser. apiHandler.RegisterEndpointHandler(kApiUrlPart + "shellUrl", HandleGetShellUrl, true); + + // GET returns what the Edit tab is doing, as + // {state, pageId, visible, pageLoadAnnouncements}. A test must not ask that tab to + // change pages while it is still loading one: the request is queued until the page + // loads, and a page announces itself more than once, so a request released by the + // first announcement starts a save that the second one leaves unanswered. Nothing + // happens after that (see src/BloomE2E/AUTOMATION-DEBT.md). The DOM cannot tell a test + // any of this, because the frame still holds the page from before the tab switch, so + // the count of announcements is how a test knows the last one has been and gone. Off + // the UI thread on purpose: this reads three fields, and a test asks for them exactly + // when the UI thread is busy. + apiHandler.RegisterEndpointHandler( + kApiUrlPart + "editState", + HandleGetEditState, + false // does not need the UI thread + ); + } + + /// + /// Reply with what the Edit tab is doing (see the registration above). Before the first + /// book is selected there is no EditingModel, which reads as NoPage. + /// + private void HandleGetEditState(ApiRequest request) + { + var model = Bloom.Edit.EditingModel.ModelForE2eTests; + request.ReplyWithJson( + new + { + state = (model?.EditTabState ?? State.NoPage).ToString(), + pageId = model?.EditTabStatePageId ?? "", + visible = model?.Visible ?? false, + pageLoadAnnouncements = model?.PageLoadAnnouncements ?? 0, + } + ); } /// diff --git a/src/BloomExe/web/controllers/ProblemReportApi.cs b/src/BloomExe/web/controllers/ProblemReportApi.cs index 567ec4e5b05c..3eded937d1ba 100644 --- a/src/BloomExe/web/controllers/ProblemReportApi.cs +++ b/src/BloomExe/web/controllers/ProblemReportApi.cs @@ -1173,10 +1173,16 @@ private static void TryGetScreenshot(Control controlForScreenshotting) { ResetScreenshotFile(); } - else if (IsBloomProcessInForeground()) + else if (IsBloomProcessInForeground() && !Program.StartupHeadless) { // Bloom is the foreground app: a plain screen copy is cheaper // and avoids re-triggering any paint-related bugs. + // + // Not under --headless, though. That run's window sits far + // outside every monitor, so copying from those screen coordinates + // would save whatever the desktop has there, which is nothing. + // Render the window itself instead, the way the not-in-front case + // already does. var scaledBounds = controlForScreenshotting.Bounds; #if !__MonoCS__ scaledBounds = From f48c62a488748cbb1a3ddeee80b6807cd2498345 Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 2 Sep 2026 13:04:17 -0600 Subject: [PATCH 05/10] Refuse a page jump the Edit tab cannot do, instead of queueing it Devin found that the queue this branch added made things worse rather than better. JumpToPage remembered a jump that arrived while the Edit tab was loading a page, and acted on it when that page announced its DOM had loaded. A page announces that more than once: the jump ran on the first announcement and started saving the page being left, the second announcement was refused because a save was in flight, and the browser never answered the save request. The Edit tab then refused every page change until it was left. That is worse than what it replaced. Before, a person's click on a page thumbnail at that moment was dropped and they clicked again. After, it left them unable to change pages at all. So JumpToPage now either shows the page at once or refuses and says so, and editView/jumpToPage answers with an error. Both front-end callers already pass report:false, so a refusal raises no problem report. This restores what a person sees today and keeps the honest answer the branch set out to give: the old code reported success for a jump it had dropped. DeferUntilPageIsLoaded and TakeWorkDeferredUntilPageIsLoaded go with it. The Bloom defect underneath is older than this suite and remains: any page change reaching the Edit tab between two announcements of one page load wedges it. AUTOMATION-DEBT.md records it with two fix directions, both of which change how saving behaves. Also: goToPage asks up to three times, waiting for a settled Edit tab before each, since a refusal is now an error rather than a silent drop; and E2eTestingApi says why the editState reply reads the announcement count last, which is what stops a mixed reply letting a test stop waiting early. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomE2E/AUTOMATION-DEBT.md | 45 +++++++------- src/BloomE2E/helpers/bookMaking.ts | 44 +++++++++----- src/BloomExe/Edit/EditingModel.cs | 43 +++++++------- src/BloomExe/Edit/EditingStateMachine.cs | 58 +------------------ src/BloomExe/web/controllers/E2eTestingApi.cs | 5 ++ 5 files changed, 83 insertions(+), 112 deletions(-) diff --git a/src/BloomE2E/AUTOMATION-DEBT.md b/src/BloomE2E/AUTOMATION-DEBT.md index fe92f4d43a02..9ace589ae508 100644 --- a/src/BloomE2E/AUTOMATION-DEBT.md +++ b/src/BloomE2E/AUTOMATION-DEBT.md @@ -219,10 +219,10 @@ the test-side check necessary. ## A page change asked for while the Edit tab is still loading a page can be lost -`editView/jumpToPage` accepts a jump that arrives while the Edit tab is navigating, and -`EditingStateMachine.DeferUntilPageIsLoaded` runs it once that page loads -(`EditingModel.HandlePageDomLoadedEvent`). That queued jump is sometimes lost, and the -caller waits 60 seconds for a page that never comes. +A page change that reaches the Edit tab between two announcements of one page load wedges +that tab: it stays in SavePending and refuses every later page change. The caller waits 60 +seconds for a page that never comes, and a user who clicks a page thumbnail in that window +cannot change pages at all until they leave the tab. The cause is a page that announces itself to Bloom twice. Bloom's own log, from the run that found this on 2026-09-02: @@ -233,32 +233,37 @@ Editing(f45a2ef8) --> savePending() Ignoring edit() request while in SavePending(f45a2ef8) ``` -The first announcement moves the tab to Editing and releases the queued jump, which asks -the browser for the page content so it can save the page it is leaving. The second -announcement then arrives, is refused because a save is in flight, and the browser never -answers the save request. The tab stays in SavePending, so the jump never navigates. The -same three lines in the order that works read `--> editing`, `Ignoring edit()`, then -`--> savePending`, and the save completes. +The first announcement moves the tab to Editing. The page change then asks the browser for +the page content so it can save the page it is leaving. The second announcement arrives, is +refused because a save is in flight, and the browser never answers the save request. The +tab stays in SavePending. The same three lines in the order that works read `--> editing`, +`Ignoring edit()`, then `--> savePending`, and the save completes. -The same thing happens to any request that changes the page, not only a jump. The run that -found it lost a language change from `setContentLanguages`, and the Edit tab then held the -tab switch that followed, so a Publish test failed instead. +Any request that changes the page does this, not only a jump. The run that found it lost a +language change from `setContentLanguages`, and the Edit tab then held the tab switch that +followed, so a Publish test failed instead. + +A first attempt at this, on 2026-09-02, had `JumpToPage` queue a jump that arrived while +the tab was navigating and release it on the next page-load announcement. That made the +wedge easier to reach rather than harder, because the released jump is itself a page change +arriving in exactly the window above. `JumpToPage` now refuses such a jump and says so, so +the caller can wait and ask again. Worked around for tests, 2026-09-02: `e2e/editState` reports what the Edit tab is doing and how many times the page it shows has announced itself, and every helper that changes the -page waits for `waitForEditTabSettled` first. That keeps the request out of the queue. The +page waits for `waitForEditTabSettled` first. That keeps the request out of the window. The wait reads the state twice, 1500 ms apart, and needs Editing both times with the count unchanged: the state alone reads Editing between the two announcements, when a request would still be lost, and the gap between them has been seen to reach a second. No test can see any of this from the DOM, because Bloom leaves the previous page in the frame while it loads the next one. -What remains: the Bloom defect itself. A user hits it whenever something asks for a page -change in that window, and the Edit tab then stops responding to page changes altogether. -Fix direction: either stop the browser announcing a page twice, or make the state machine -treat a second announcement of the page it is saving as a reason to discard that save -(`DiscardInFlightSave` already exists for BL-16766). Both change production save behavior, -so this needs a decision rather than a quiet fix. +What remains: the Bloom defect itself, which is older than this suite. A user hits it +whenever something asks for a page change in that window, and the Edit tab then stops +responding to page changes altogether. Fix direction: either stop the browser announcing a +page twice, or make the state machine treat a second announcement of the page it is saving +as a reason to discard that save (`DiscardInFlightSave` already exists for BL-16766). Both +change production save behavior, so this needs a decision rather than a quiet fix. ## A Vite dev server only reaches the whole UI on port 5173 diff --git a/src/BloomE2E/helpers/bookMaking.ts b/src/BloomE2E/helpers/bookMaking.ts index 24c447f9a7bf..c9f93915ea61 100644 --- a/src/BloomE2E/helpers/bookMaking.ts +++ b/src/BloomE2E/helpers/bookMaking.ts @@ -252,10 +252,11 @@ interface IEditTabState { * Wait until the Edit tab is settled on a page: showing it, in the Editing state, and done * announcing that the page has loaded. * - * Ask this before any request that changes the page. The Edit tab defers such a request while it - * navigates, and a deferred one is lost (AUTOMATION-DEBT.md: "A page change asked for while the - * Edit tab is still loading a page can be lost"). A page announces itself to Bloom more than once, - * and a request that arrives between two of those announcements is lost as well, so waiting for + * Ask this before any request that changes the page. The Edit tab refuses such a request while it + * navigates, and a page announces itself to Bloom more than once, so a request that arrives + * between two of those announcements starts a save that the second announcement leaves unanswered + * (AUTOMATION-DEBT.md: "A page change asked for while the Edit tab is still loading a page can be + * lost"). Waiting for * the Editing state alone is not enough. Hence the second reading: the tab is settled once the * count of announcements has stopped rising. * @@ -384,14 +385,31 @@ export async function addPage( * it is leaving, so text typed into a box reaches the file only once the book moves off that page. */ export async function goToPage(page: Page, pageId: string): Promise { - // Wait for the Edit tab first. It queues a jump that arrives while it is loading a page, and - // one of those queued jumps is lost (see waitForEditTabSettled). Asking at a moment when the - // tab acts on the jump at once avoids the whole queue. - await waitForEditTabSettled(page); - // Then one request is enough: the tab answers with an error if it can neither jump nor queue - // (EditingModel.JumpToPage). It used to drop such a jump and report success, which is why this - // helper used to ask three times. - await apiPost(page, "editView/jumpToPage", pageId, "text/plain"); + // Wait for the Edit tab first. A jump asked for while it is still loading a page is refused, + // and one asked for between two announcements of one page load wedges it (see + // waitForEditTabSettled). Asking at a moment when the tab acts on the jump at once avoids + // both. + // Then ask, and ask again if the tab was busy after all. The tab answers with an error when + // it cannot jump (EditingModel.JumpToPage), rather than dropping the jump and reporting + // success as it used to, so a refusal is visible here instead of turning into a 60-second + // wait for a page that is not coming. Three tries, because the wait above closes the window + // for a refusal without quite shutting it: the tab can start loading the page list in the + // moment between the wait finishing and this request arriving. + let refusal: unknown; + for (let attempt = 0; attempt < 3; attempt++) { + if (attempt > 0) await waitForEditTabSettled(page); + refusal = undefined; + await apiPost(page, "editView/jumpToPage", pageId, "text/plain").catch( + (error) => { + refusal = error; + }, + ); + if (refusal === undefined) break; + } + if (refusal !== undefined) + throw new Error( + `The Edit tab refused to show page ${pageId} three times running: ${refusal}`, + ); const showing = async () => (await page .frame({ name: "page" }) @@ -417,7 +435,7 @@ export async function goToPage(page: Page, pageId: string): Promise { .then((response) => response.body) .catch((error) => `(could not be read: ${error})`); // The state the Edit tab is stuck in says which of the two failures this is: a jump that - // was lost leaves the tab in SavePending or Navigating on the page it already had. + // wedged the tab leaves it in SavePending or Navigating on the page it already had. const editState = await apiGet(page, "e2e/editState") .then((response) => response.body) .catch((error) => `(could not be read: ${error})`); diff --git a/src/BloomExe/Edit/EditingModel.cs b/src/BloomExe/Edit/EditingModel.cs index 466cab0e88e9..73d0293bb479 100644 --- a/src/BloomExe/Edit/EditingModel.cs +++ b/src/BloomExe/Edit/EditingModel.cs @@ -1743,15 +1743,14 @@ internal void SavePageAndReloadIt(ApiRequest request) /// Show the page with this id in the Edit tab. The page being left is saved on the way, /// which is how anything typed on it reaches the file. /// - /// A jump can arrive when the Edit tab cannot act on it at once: the tab is not showing - /// yet, it is still navigating to a page, or a save is in flight. In each of those cases - /// the jump is remembered and done as soon as that finishes. Bloom used to drop it and - /// report success, which left the caller waiting for a page that was never coming (see - /// src/BloomE2E/AUTOMATION-DEBT.md). + /// A jump that arrives before the Edit tab is showing is remembered, and OnBecomeVisible + /// displays that page instead of the one it would otherwise choose. /// - /// Returns false when the jump can be neither started nor queued, so the caller can say so - /// rather than wait: there is no book, or the Edit tab is in the momentary state after a - /// save in which no transition is allowed. + /// Otherwise the jump either happens at once or is refused: this returns false when the + /// Edit tab is busy with a page (loading one, or saving one), when there is no book, and + /// when the tab is in the momentary state after a save in which no transition is allowed. + /// Bloom used to drop such a jump and report success, which left the caller waiting for a + /// page that was never coming (see src/BloomE2E/AUTOMATION-DEBT.md). /// public bool JumpToPage(string pageId) { @@ -1766,13 +1765,20 @@ public bool JumpToPage(string pageId) return true; } - // Ask to be run again when the Edit tab is done with what it is doing. We check the - // state before calling SaveThen so that the answer we give the caller is decided - // before any save starts. - if (_stateMachine.SavePending) - return _stateMachine.DeferUntilSaveCompletes(() => JumpToPage(pageId)); - if (_stateMachine.Navigating) - return _stateMachine.DeferUntilPageIsLoaded(() => JumpToPage(pageId)); + // The Edit tab is busy with a page: saving one, or loading one. Say so rather than + // queue the jump. A queued jump has to be released by some signal that the tab is + // ready, and the only signal available is a page announcing that its DOM has loaded, + // which a page does more than once. A jump released by the first announcement starts + // a save, the second announcement is then refused because a save is in flight, and + // the browser never answers the save request, which leaves the tab unable to change + // pages at all. See src/BloomE2E/AUTOMATION-DEBT.md. + if (_stateMachine.SavePending || _stateMachine.Navigating) + { + Logger.WriteEvent( + $"EditingModel.JumpToPage({pageId}): the edit tab was busy with a page, so the jump was refused." + ); + return false; + } var jumped = true; SaveThen( @@ -2316,13 +2322,6 @@ public void HandlePageDomLoadedEvent(string pageId) // move on to the next page. See StartUpdatingAllPages(). if (nowEditing && _updatingAllPages) AdvanceUpdatingAllPages(pageId); - - // Last of all, a jump that arrived while we were navigating here (see JumpToPage). - // It saves this page and navigates off it, so everything above, which acts on the page - // that just loaded, has to happen first. If the work above left us navigating or - // saving again, JumpToPage queues itself once more. - if (nowEditing) - _stateMachine.TakeWorkDeferredUntilPageIsLoaded()?.Invoke(); } // The one action queued by RunAfterNextPageLoad, or null. diff --git a/src/BloomExe/Edit/EditingStateMachine.cs b/src/BloomExe/Edit/EditingStateMachine.cs index 2bf598440c13..e5a1e51f3990 100644 --- a/src/BloomExe/Edit/EditingStateMachine.cs +++ b/src/BloomExe/Edit/EditingStateMachine.cs @@ -51,9 +51,6 @@ public class EditingStateMachine // See DeferUntilSaveCompletes. private Action _workToDoAfterInFlightSave; - // Work that arrived while we were navigating to a page and could not be done then. It runs - // once that page has loaded. See DeferUntilPageIsLoaded. - private Action _workToDoAfterNavigation; private Action _hidePage; private Action _enableStateTransitions; // arg is (enabled) @@ -109,10 +106,6 @@ public bool ToNoPage() return true; case State.Navigating: LogShortcut("empty page"); - // The navigation this work was waiting for is over, and no page loaded, so - // the work must not run: it belonged to a page we are no longer showing. This - // is the path a switch away from the Edit tab takes. - _workToDoAfterNavigation = null; _hidePage(); _currentState = State.NoPage; return true; @@ -223,32 +216,9 @@ private void StartNavigating(string pageId) } /// - /// Called after we hear from the browser JS that the dom is finished loading. - /// Work that had to wait for this navigation does NOT run here; the caller takes it with - /// TakeWorkDeferredUntilPageIsLoaded once it has done its own work on the loaded page. + /// Called after we hear from the browser JS that the dom is finished loading /// public bool ToEditing(string pageId) - { - return ToEditingFromNavigating(pageId); - } - - /// - /// Hand back the work that was deferred until a page loaded (see DeferUntilPageIsLoaded), and - /// forget it, so the caller can run it. Returns null when there is none. - /// - /// The caller runs it rather than ToEditing, because the code that hears "the page has loaded" - /// has its own work to do first: an action queued for the next page load, and the next step of - /// the Update Book pass. A jump that ran before those would save and navigate away from the - /// page they were about to act on. See EditingModel.HandlePageDomLoadedEvent. - /// - public Action TakeWorkDeferredUntilPageIsLoaded() - { - var deferredWork = _workToDoAfterNavigation; - _workToDoAfterNavigation = null; - return deferredWork; - } - - private bool ToEditingFromNavigating(string pageId) { try { @@ -453,32 +423,6 @@ public bool DeferUntilSaveCompletes(Action work) return true; } - /// - /// For a caller whose work needs a loaded page, and which found us navigating to one (which is - /// why ToSavePending refused it). If we really are navigating, is - /// remembered and run once that page has loaded, and this returns true — the caller must then - /// do nothing else. Otherwise it returns false and the caller must handle its own request. - /// - /// A navigation that ends any other way than with a loaded page (a switch away from the Edit - /// tab, which comes through ToNoPage) throws the work away: it belonged to a page that is no - /// longer being shown. - /// - /// A jump to a page is the case this exists for: it arrives from an API call at any moment, - /// including while the Edit tab is loading the page it displays on becoming visible. Bloom used - /// to drop such a jump and tell the caller it had succeeded (see src/BloomE2E/AUTOMATION-DEBT.md). - /// - /// Only one piece of deferred work is kept: a later request supersedes an earlier one, since it - /// is the more recent thing that was asked for. - /// - public bool DeferUntilPageIsLoaded(Action work) - { - // We are not navigating, or there is nothing to do: the caller must handle it itself. - if (_currentState != State.Navigating || work == null) - return false; - _workToDoAfterNavigation = work; - return true; - } - /// /// Source: API call providing content of current page will request this after saving and before executing pending action /// (e.g. changing pages) diff --git a/src/BloomExe/web/controllers/E2eTestingApi.cs b/src/BloomExe/web/controllers/E2eTestingApi.cs index 798f252c1cc4..43d5e414cdec 100644 --- a/src/BloomExe/web/controllers/E2eTestingApi.cs +++ b/src/BloomExe/web/controllers/E2eTestingApi.cs @@ -172,6 +172,11 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) private void HandleGetEditState(ApiRequest request) { var model = Bloom.Edit.EditingModel.ModelForE2eTests; + // The UI thread can change these fields between one read and the next, so keep + // pageLoadAnnouncements last. A count read after the state can only be the same or + // higher, which makes a test see the count still rising and wait again. Read first, it + // could pair a stale count with a settled state, and a test would stop waiting too + // soon. request.ReplyWithJson( new { From 12671ef86bf158896b2222a753fde017e29c897b Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 2 Sep 2026 13:41:07 -0600 Subject: [PATCH 06/10] Close the review findings: argument parsing, cleanup, window placement, tool registration Four separate defects, all raised in review of this branch. `--repo-root` in killBloomProcess.mjs and bloomProcessStatus.mjs took the next argument as its value without checking it. `--repo-root --pid 123` therefore consumed `--pid` as the repository path, named no target at all, and reached killBloomProcess.mjs's default, which kills every Bloom this worktree owns. It now uses requireOptionValue, the same check every other option already had, so the malformed command exits non-zero and kills nothing. captureElement warned on standard error, rather than failing, when it could not clear the window size override after the capture itself had failed. A warning in a long run is easy to miss, and every later test in that worker then drives a Bloom of the wrong size. It now always throws, and carries the capture's own error as the cause so neither error is lost. GetHeadlessBounds could ask Windows for an x coordinate further left than the -32000 its own comment names as the limit, because Math.Min took whichever of the two candidates was further left. No monitor layout reaches that far left, so there is nothing to compute from the layout: it returns the constant. registerAllToolboxTools skipped all eleven tools whenever the master list held any entry at all. It now checks each tool by id, so a list holding some other tool cannot leave the toolbox with no sections. Co-Authored-By: Claude Opus 5 (1M context) --- .../bloom-automation/bloomProcessStatus.mjs | 6 ++- .../bloom-automation/killBloomProcess.mjs | 6 ++- .../toolbox/registerAllToolboxTools.ts | 51 ++++++++++++------- src/BloomE2E/helpers/screenshot.ts | 32 +++++++----- src/BloomExe/Shell.cs | 7 ++- 5 files changed, 64 insertions(+), 38 deletions(-) diff --git a/.github/skills/bloom-automation/bloomProcessStatus.mjs b/.github/skills/bloom-automation/bloomProcessStatus.mjs index 8c8652557fab..b6b1345c1800 100644 --- a/.github/skills/bloom-automation/bloomProcessStatus.mjs +++ b/.github/skills/bloom-automation/bloomProcessStatus.mjs @@ -53,7 +53,11 @@ const parseArgs = () => { } if (arg === "--repo-root") { - options.repoRoot = args[i + 1] || options.repoRoot; + // A required value, checked the same way as every other option's. Taking + // `args[i + 1]` and falling back to the default would swallow the next flag as + // this option's value. killBloomProcess.mjs has the same parser, where that + // mistake is destructive. + options.repoRoot = requireOptionValue(args, i, "--repo-root"); i++; continue; } diff --git a/.github/skills/bloom-automation/killBloomProcess.mjs b/.github/skills/bloom-automation/killBloomProcess.mjs index 6b5ffdc08775..20b483ae161c 100644 --- a/.github/skills/bloom-automation/killBloomProcess.mjs +++ b/.github/skills/bloom-automation/killBloomProcess.mjs @@ -67,7 +67,11 @@ const parseArgs = () => { } if (arg === "--repo-root") { - options.repoRoot = args[i + 1] || options.repoRoot; + // A required value, checked the same way as every other option's. Taking + // `args[i + 1]` and falling back to the default would swallow the NEXT FLAG as + // this option's value, so `--repo-root --pid 123` would name no target at all and + // reach the default that kills every Bloom this worktree owns. + options.repoRoot = requireOptionValue(args, i, "--repo-root"); i++; continue; } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts b/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts index 9e0135e465a0..1baf3e8fd6e4 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts @@ -8,7 +8,7 @@ // both toolboxBootstrap.ts and react_components/ToolboxRootTestHarness call the function below. // (AUTOMATION-DEBT.md: "Toolbox tool registration is a side effect of toolboxBootstrap".) -import { ToolBox, getMasterToolList } from "./toolbox"; +import { ITool, ToolBox, getMasterToolList } from "./toolbox"; import { DecodableReaderTool } from "./readers/decodableReader/decodableReaderTool"; import { LeveledReaderTool } from "./readers/leveledReader/leveledReaderTool"; import { MusicToolAdaptor } from "./music/musicToolControls"; @@ -25,23 +25,38 @@ import { SettingsTool } from "./settings/settingsTool"; * Make the one instance of each toolbox class and register it with the master toolbox. The * imports above also serve to ensure that each tool's code is part of the bundle. * - * Calling this twice registers nothing the second time. ToolBox.registerTool is a bare push with - * no check for duplicates, and the guard keys off the shared master list rather than a flag in - * this module: a caller that is a React-Refresh boundary re-executes its own module during - * `pnpm dev` while masterToolList, which lives in toolbox.ts, keeps its entries. A flag here - * would reset and we would get eleven duplicate tools and duplicate accordion sections. + * Calling this twice registers nothing the second time. See registerOnce for why the check is + * per tool and reads the shared master list. */ export function registerAllToolboxTools(): void { - if (getMasterToolList().length > 0) return; - ToolBox.registerTool(new DecodableReaderTool()); - ToolBox.registerTool(new LeveledReaderTool()); - ToolBox.registerTool(new MusicToolAdaptor()); - ToolBox.registerTool(new ImpairmentVisualizerAdaptor()); - ToolBox.registerTool(new MotionTool()); - ToolBox.registerTool(new TalkingBookTool()); - ToolBox.registerTool(new SignLanguageTool()); - ToolBox.registerTool(new ImageDescriptionAdapter()); - ToolBox.registerTool(new CanvasTool()); - ToolBox.registerTool(new GameTool()); - ToolBox.registerTool(new SettingsTool()); + registerOnce(new DecodableReaderTool()); + registerOnce(new LeveledReaderTool()); + registerOnce(new MusicToolAdaptor()); + registerOnce(new ImpairmentVisualizerAdaptor()); + registerOnce(new MotionTool()); + registerOnce(new TalkingBookTool()); + registerOnce(new SignLanguageTool()); + registerOnce(new ImageDescriptionAdapter()); + registerOnce(new CanvasTool()); + registerOnce(new GameTool()); + registerOnce(new SettingsTool()); +} + +/** + * Register one tool, unless the master list already has a tool of that id. + * + * ToolBox.registerTool is a bare push with no check for duplicates, so something has to do the + * check. It reads the shared master list rather than a flag in this module because a caller that + * is a React-Refresh boundary re-executes its own module during `pnpm dev` while masterToolList, + * which lives in toolbox.ts, keeps its entries. A flag here would reset and we would get eleven + * duplicate tools and duplicate accordion sections. + * + * The check is per tool, not "is the list empty": a list holding some other tool is not evidence + * that these eleven are registered, and skipping all of them on that evidence would leave the + * toolbox missing every section. + */ +function registerOnce(tool: ITool): void { + if (getMasterToolList().some((registered) => registered.id() === tool.id())) + return; + ToolBox.registerTool(tool); } diff --git a/src/BloomE2E/helpers/screenshot.ts b/src/BloomE2E/helpers/screenshot.ts index 16d4da8a644f..2d1addff2b21 100644 --- a/src/BloomE2E/helpers/screenshot.ts +++ b/src/BloomE2E/helpers/screenshot.ts @@ -82,9 +82,9 @@ export async function captureElement( await locator.waitFor({ state: "visible", timeout: timeoutMs }); const session = await page.context().newCDPSession(page); - // Set once the capture has produced an image, so the cleanup below can tell a clear failure - // that is the only thing wrong from one that is trailing a capture error. - let captured: IElementImage | undefined; + // The capture's own failure, kept so that a cleanup failure on top of it can carry it as a + // cause instead of hiding it. + let captureError: unknown; try { // How big the window has to be for the whole element to be laid out at once. Ask for the // element's own scroll size, plus where it sits, rather than the document's size: the @@ -144,14 +144,16 @@ export async function captureElement( const png = Buffer.from(result.data, "base64"); const size = readPngSize(png); - captured = { + return { png, width: size.width, height: size.height, elementWidth: box.width, elementHeight: box.height, }; - return captured; + } catch (error) { + captureError = error; + throw error; } finally { // Always, including after a failed capture: the next test in this worker drives the same // Bloom, and an emulated 8000-pixel window would make everything it sees wrong. @@ -167,16 +169,18 @@ export async function captureElement( if (clearError !== undefined) { // The window is still the size we made it, and the rest of this worker's tests would - // measure that Bloom rather than the real one. Fail rather than warn. + // measure that Bloom rather than the real one. Fail rather than warn, whether or not + // the capture itself got that far: a warning on standard error is easy to miss in a + // long run, and every later test in this worker is then wrong. // - // Only when the capture itself succeeded, though. If we are in this block because the - // capture threw, that error is the one the test needs to read, and throwing here would - // replace it. In that case the message is all we can give. - const message = `captureElement could not clear the window size override, so the rest of this worker's tests would drive a Bloom of the wrong size: ${clearError}`; - if (captured) { - throw new Error(message); - } - console.error(message); + // When the capture failed too, that error is the one the test author needs to read, + // so hand it on as the cause rather than let this one replace it. + throw new Error( + `captureElement could not clear the window size override, so the rest of this worker's tests would drive a Bloom of the wrong size: ${clearError}`, + captureError === undefined + ? undefined + : { cause: captureError }, + ); } } } diff --git a/src/BloomExe/Shell.cs b/src/BloomExe/Shell.cs index 12f10b15b6a7..ac84cf9d7bb3 100644 --- a/src/BloomExe/Shell.cs +++ b/src/BloomExe/Shell.cs @@ -91,11 +91,10 @@ public static Rectangle GetHeadlessBounds() // window messages still carry. Far enough left of every monitor that nothing shows, // and unlike a margin computed from the screen layout it does not depend on Windows // and this process agreeing about how many pixels wide a monitor is (they disagree - // when the monitors have different scale factors). + // when the monitors have different scale factors). No monitor layout reaches this far + // left, so there is nothing to compute from the layout at all. const int farLeftOfEveryScreen = -32000; - var leftmostX = Screen.AllScreens.Min(screen => screen.Bounds.Left); - var x = Math.Min(farLeftOfEveryScreen, leftmostX - size.Width - 1000); - return new Rectangle(x, 0, size.Width, size.Height); + return new Rectangle(farLeftOfEveryScreen, 0, size.Width, size.Height); } public Shell( From 9267d1e705342d5600f8130172af91b691ba0a09 Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 2 Sep 2026 15:27:10 -0600 Subject: [PATCH 07/10] Place the headless window inside both bounds, not one or the other GetHeadlessBounds has two constraints and the earlier versions each honoured only one. Taking whichever of the two candidates was further left could ask Windows for a coordinate past the -32000 that the comment itself names as the limit. Returning the constant instead could put the window on a monitor, on a leftward run of monitors wide enough to reach that far. It now computes the position that clears the leftmost monitor, keeping the 1000-pixel cushion for the case where Windows and this process disagree about how wide a monitor is, and clamps that to -32000. On any real layout the first bound lands a few thousand pixels to the left, well inside the limit. A leftward run of monitors more than about 30000 pixels wide satisfies neither bound, and there the limit wins: a coordinate Windows will not honour is worse than an overlap. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomExe/Shell.cs | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/BloomExe/Shell.cs b/src/BloomExe/Shell.cs index ac84cf9d7bb3..9e076b8afd29 100644 --- a/src/BloomExe/Shell.cs +++ b/src/BloomExe/Shell.cs @@ -86,15 +86,25 @@ public static Screen GetAutomationScreen() public static Rectangle GetHeadlessBounds() { var size = GetAutomationScreen().WorkingArea.Size; - // -32000 is as far left as a window may go: Windows still places a window there, and - // anything beyond about -32768 runs into the 16-bit coordinates that some of the older - // window messages still carry. Far enough left of every monitor that nothing shows, - // and unlike a margin computed from the screen layout it does not depend on Windows - // and this process agreeing about how many pixels wide a monitor is (they disagree - // when the monitors have different scale factors). No monitor layout reaches this far - // left, so there is nothing to compute from the layout at all. - const int farLeftOfEveryScreen = -32000; - return new Rectangle(farLeftOfEveryScreen, 0, size.Width, size.Height); + // Two bounds, and the window has to respect both. + // + // The first is the leftmost monitor: the window's right edge has to be left of it, or + // part of the window shows. The 1000-pixel cushion is there because Windows and this + // process do not always agree about how many pixels wide a monitor is, which is what + // happens when the monitors have different scale factors. + // + // The second is -32000, as far left as a window may go: Windows still places a window + // there, and anything beyond about -32768 runs into the 16-bit coordinates that some + // of the older window messages still carry. + // + // On any real layout the first bound gives a few thousand pixels to the left, well + // inside the second. A leftward run of monitors more than about 30000 pixels wide + // would need a position that satisfies neither, and then the limit wins: a window at + // a coordinate Windows will not honour is worse than one that overlaps a monitor. + const int farLeftWindowsAllows = -32000; + var leftmostX = Screen.AllScreens.Min(screen => screen.Bounds.Left); + var x = Math.Max(farLeftWindowsAllows, leftmostX - size.Width - 1000); + return new Rectangle(x, 0, size.Width, size.Height); } public Shell( From 512d963e25eb9f72f9d19118a87467d9a835b170 Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 2 Sep 2026 16:13:11 -0600 Subject: [PATCH 08/10] Hand the off-screen work to BL-16804, and keep the automation debt here BL-16799 asked for two things: an e2e suite that does not open a window on the developer's desktop, and the automation debt that fixing it exposed. Those are now two cards and two pull requests, at the developer's request, so that the off-screen change can be reviewed on its own. This branch keeps the debt. Everything about the off-screen window moves to BL-16804: the --headless flag and its unit tests, the window and splash-screen placement, the WebView2 occlusion settings that an off-screen window needs, the problem-report screenshot path that must render the window rather than copy the screen, the fixture that passes the flag, and the documentation of it. Neither branch depends on the other. Until BL-16804 merges, a run of this suite opens a Bloom window again. Co-Authored-By: Claude Opus 5 (1M context) --- .github/skills/add-e2e-test/SKILL.md | 6 -- src/BloomE2E/README.md | 13 +--- src/BloomE2E/fixtures/launchBloom.ts | 18 ------ src/BloomExe/Program.cs | 15 ----- src/BloomExe/Shell.cs | 64 ++----------------- src/BloomExe/SplashScreen.cs | 23 ++----- src/BloomExe/WebView2Browser.cs | 8 --- .../web/controllers/ProblemReportApi.cs | 8 +-- src/BloomTests/ProgramTests.cs | 27 -------- 9 files changed, 14 insertions(+), 168 deletions(-) diff --git a/.github/skills/add-e2e-test/SKILL.md b/.github/skills/add-e2e-test/SKILL.md index 186cd3a1e5cf..70a7418c0e34 100644 --- a/.github/skills/add-e2e-test/SKILL.md +++ b/.github/skills/add-e2e-test/SKILL.md @@ -338,12 +338,6 @@ pnpm exec playwright test tests/workspace-tabs.spec.ts # one file pnpm exec playwright test -g "switching workspace tabs" # one test by title ``` -A run launches a real Bloom, but no window appears: the fixture passes `--headless`, which puts -Bloom's window far outside every monitor, so a run does not take your desktop over. Set -`BLOOM_E2E_HEADED=1` to watch it (`--debug` sets it for you). The window goes off-screen rather -than minimized or hidden because WebView2 stops painting a minimized window, which makes every -screenshot blank. - A run needs a built `Bloom.exe` under `output/{Debug,Release}/{x64,AnyCPU,}/` (build it yourself; see "Build Bloom whenever it helps") and the inputs at `output/testing-inputs`. Point `BLOOM_TESTING_INPUTS_DIR` at a bloom-testing-inputs checkout to use your own in-progress diff --git a/src/BloomE2E/README.md b/src/BloomE2E/README.md index 8159fafe7536..ffd63c26ed84 100644 --- a/src/BloomE2E/README.md +++ b/src/BloomE2E/README.md @@ -138,18 +138,7 @@ pnpm exec playwright test -g "switching workspace tabs" # one test by titl pnpm exec playwright test --debug # step through it ``` -A run opens a real Bloom, but you will not see it: Bloom is launched with `--headless`, which puts -its window far outside every monitor, so a run can go on while you work. The window is moved -off-screen rather than minimized or hidden because WebView2 stops painting a minimized window, -which would make every screenshot blank. - -To watch the run instead, set `BLOOM_E2E_HEADED=1`; `--debug` turns it on for you. - -```bash -BLOOM_E2E_HEADED=1 pnpm exec playwright test tests/workspace-tabs.spec.ts -``` - -A headed run opens a real Bloom window. That is expected; do not click in it. +A run opens a real Bloom window. That is expected; do not click in it. The suite needs a built `Bloom.exe` under `output/{Debug,Release}/{x64,AnyCPU,}/` and the test inputs at `output/testing-inputs`, fetched by `node build/get-testing-inputs.mjs` at the commit diff --git a/src/BloomE2E/fixtures/launchBloom.ts b/src/BloomE2E/fixtures/launchBloom.ts index cd7038db7e46..8f5aad836518 100644 --- a/src/BloomE2E/fixtures/launchBloom.ts +++ b/src/BloomE2E/fixtures/launchBloom.ts @@ -158,22 +158,6 @@ function samePath(a: string, b: string): boolean { const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); -/** - * Whether the Bloom we launch should appear on a monitor. By default it does not: --headless puts - * its window far outside every screen, so a run does not take the developer's desktop over. - * - * The window is moved off-screen rather than minimized or hidden because WebView2 stops painting a - * minimized window: screenshots come back blank and the layout is the wrong size. Off-screen, the - * window paints exactly as it would in front of a person, so keyboard input and rendering behave - * the same (see Shell.GetHeadlessBounds). - * - * Set BLOOM_E2E_HEADED=1 to watch the run. Playwright's --debug (which sets PWDEBUG) implies it: - * there is no point stepping through a test whose window you cannot see. - */ -function shouldShowBloomOnScreen(): boolean { - return process.env.BLOOM_E2E_HEADED === "1" || !!process.env.PWDEBUG; -} - /** * The Vite dev server port the launched Bloom should load its React front end from, or undefined * to leave the choice to Bloom. @@ -397,9 +381,7 @@ async function startBloomOn( // --e2e: skip the DEBUG "attach debugger now" prompt and suppress modal error dialogs. // --automation: let this instance run alongside a Bloom the developer already has open. - // --headless: keep the window off every screen (see shouldShowBloomOnScreen). const args = [findCollectionFile(collectionDir), "--e2e", "--automation"]; - if (!shouldShowBloomOnScreen()) args.push("--headless"); // --vite-port: serve the React front end from a dev server, so the suite tests the working // tree rather than a stale output/browser (see getViteDevPort). const vitePort = getViteDevPort(); diff --git a/src/BloomExe/Program.cs b/src/BloomExe/Program.cs index 9a22212450aa..d90eb537146b 100644 --- a/src/BloomExe/Program.cs +++ b/src/BloomExe/Program.cs @@ -108,11 +108,6 @@ static class Program internal static string StartupLabel { get; private set; } internal static bool StartupAutomation { get; private set; } - // True when --headless was passed. An e2e run wants Bloom's window to paint (screenshots - // and keyboard input both need that) but not to appear on any monitor, so Shell places the - // window far outside every screen instead of minimizing or hiding it. See Shell_Load. - internal static bool StartupHeadless { get; private set; } - // Control port of the dev launcher (scripts/watchBloomExe.mjs) that started // this Bloom, passed as --launcher-port. When present, DevLauncher watches for // pending C# changes and offers a dev-only toast that asks the launcher to @@ -125,7 +120,6 @@ static class Program new[] { StartupAutomation ? "automation=true" : null, - StartupHeadless ? "headless=true" : null, StartupVitePort.HasValue ? $"vitePort={StartupVitePort.Value}" : null, StartupLauncherPort.HasValue ? $"launcherPort={StartupLauncherPort.Value}" @@ -791,7 +785,6 @@ internal static string[] ParseStartupPortArguments(string[] args, out string err StartupVitePort = null; StartupLabel = null; StartupAutomation = false; - StartupHeadless = false; StartupLauncherPort = null; RunningE2eTests = false; @@ -833,14 +826,6 @@ out errorMessage value => StartupAutomation = value, out errorMessage ) - || TryHandleStartupFlagArgument( - args, - ref i, - "--headless", - () => StartupHeadless, - value => StartupHeadless = value, - out errorMessage - ) || TryHandleStartupFlagArgument( args, ref i, diff --git a/src/BloomExe/Shell.cs b/src/BloomExe/Shell.cs index 9e076b8afd29..54cf046b558e 100644 --- a/src/BloomExe/Shell.cs +++ b/src/BloomExe/Shell.cs @@ -48,10 +48,8 @@ public static Form GetShellOrOtherOpenForm() private bool _finishedLoading; // During an automation run (--automation, e.g. the Playwright suites) the window must - // not steal the user's keyboard focus when it is shown. The same goes for a headless run - // (--headless), whose window sits off-screen where the user cannot see it at all. - protected override bool ShowWithoutActivation => - Program.StartupAutomation || Program.StartupHeadless; + // not steal the user's keyboard focus when it is shown. + protected override bool ShowWithoutActivation => Program.StartupAutomation; /// /// The screen that an automation run (--automation) should open windows on: the one @@ -74,39 +72,6 @@ public static Screen GetAutomationScreen() return Screen.PrimaryScreen; } - /// - /// Where a headless run (--headless) puts its window: the size of the automation screen's - /// working area, but positioned to the left of every monitor, so that not one pixel of it - /// is on any screen. - /// - /// The window is moved rather than minimized or hidden because a minimized WebView2 stops - /// painting: screenshots come back blank and the layout is the wrong size. An off-screen - /// window of the normal size keeps painting, so a test sees exactly what a user would. - /// - public static Rectangle GetHeadlessBounds() - { - var size = GetAutomationScreen().WorkingArea.Size; - // Two bounds, and the window has to respect both. - // - // The first is the leftmost monitor: the window's right edge has to be left of it, or - // part of the window shows. The 1000-pixel cushion is there because Windows and this - // process do not always agree about how many pixels wide a monitor is, which is what - // happens when the monitors have different scale factors. - // - // The second is -32000, as far left as a window may go: Windows still places a window - // there, and anything beyond about -32768 runs into the 16-bit coordinates that some - // of the older window messages still carry. - // - // On any real layout the first bound gives a few thousand pixels to the left, well - // inside the second. A leftward run of monitors more than about 30000 pixels wide - // would need a position that satisfies neither, and then the limit wins: a window at - // a coordinate Windows will not honour is worse than one that overlaps a monitor. - const int farLeftWindowsAllows = -32000; - var leftmostX = Screen.AllScreens.Min(screen => screen.Bounds.Left); - var x = Math.Max(farLeftWindowsAllows, leftmostX - size.Width - 1000); - return new Rectangle(x, 0, size.Width, size.Height); - } - public Shell( Func projectViewFactory, CollectionSettings collectionSettings, @@ -459,10 +424,8 @@ public static void ComeToFront() public void ReallyComeToFront() { // During an automation run, grabbing focus would yank the user's keyboard away - // from whatever they are doing on another monitor while tests run. A headless - // window must not come to the front either: it is off-screen on purpose, and - // TopMost/BringToFront on it would take the foreground away for nothing. - if (!Program.StartupAutomation && !Program.StartupHeadless) + // from whatever they are doing on another monitor while tests run. + if (!Program.StartupAutomation) { //try really hard to become top most. See http://stackoverflow.com/questions/5282588/how-can-i-bring-my-application-window-to-the-front TopMost = true; @@ -483,18 +446,7 @@ private void Shell_Load(object sender, EventArgs e) { SuspendLayout(); - if (Program.StartupHeadless) - { - // A headless run keeps the window off every screen and out of the task bar, - // so a test can run while the developer works. The window stays Normal (not - // minimized) and full size, because WebView2 only paints a window that is - // neither minimized nor hidden. See GetHeadlessBounds. - StartPosition = FormStartPosition.Manual; - WindowState = FormWindowState.Normal; - Bounds = GetHeadlessBounds(); - ShowInTaskbar = false; - } - else if (Program.StartupAutomation) + if (Program.StartupAutomation) { // An automation run must not open on whichever monitor the user is // currently working on, and must not disturb the saved window placement. @@ -516,7 +468,7 @@ private void Shell_Load(object sender, EventArgs e) // This feature is not yet a normal part of Bloom, since we think just maximizing is more rice-farmer-friendly. // However, we added the ability to remember this stuff at the request of the person making videos, who needs // Bloom to open in the same place / size each time. - if (Program.StartupAutomation || Program.StartupHeadless) + if (Program.StartupAutomation) { // Placement is already pinned above; leave the user's saved placement alone. } @@ -578,10 +530,6 @@ private void Shell_ResizeEnd(object sender, EventArgs e) return; if (WindowState != FormWindowState.Normal) return; - // A headless window is deliberately off every screen and is Normal rather than - // maximized, so saving its bounds would leave the developer's next Bloom invisible. - if (Program.StartupHeadless || Program.StartupAutomation) - return; Settings.Default.RestoreBounds = new Rectangle(Left, Top, Width, Height); Settings.Default.Save(); diff --git a/src/BloomExe/SplashScreen.cs b/src/BloomExe/SplashScreen.cs index 3fcdf5caed90..aee6218cb1a9 100644 --- a/src/BloomExe/SplashScreen.cs +++ b/src/BloomExe/SplashScreen.cs @@ -29,23 +29,13 @@ public void FadeAndClose() } // During an automation run (--automation) the splash must not steal the user's - // keyboard focus when it is shown. Neither must a headless run (--headless), whose - // windows all sit off-screen. - protected override bool ShowWithoutActivation => - Program.StartupAutomation || Program.StartupHeadless; + // keyboard focus when it is shown. + protected override bool ShowWithoutActivation => Program.StartupAutomation; private SplashScreen() { InitializeComponent(); - if (Program.StartupHeadless) - { - // A headless run shows nothing on any monitor, so the splash goes off-screen with - // the main window. See Shell.GetHeadlessBounds. - StartPosition = FormStartPosition.Manual; - var headlessArea = Shell.GetHeadlessBounds(); - Location = new System.Drawing.Point(headlessArea.Left, headlessArea.Top); - } - else if (Program.StartupAutomation) + if (Program.StartupAutomation) { // An automation run must not open on whichever monitor the user is currently // working on. Center the splash on the automation screen instead. @@ -121,9 +111,8 @@ private void _fadeOutTimer_Tick(object sender, EventArgs e) private void SplashScreen_Load(object sender, EventArgs e) { // During an automation run, grabbing focus would yank the user's keyboard away - // from whatever they are doing on another monitor while tests run. A headless splash - // is off-screen, so bringing it to the front would take the foreground for nothing. - if (!Program.StartupAutomation && !Program.StartupHeadless) + // from whatever they are doing on another monitor while tests run. + if (!Program.StartupAutomation) { //try really hard to become top most. See http://stackoverflow.com/questions/5282588/how-can-i-bring-my-application-window-to-the-front TopMost = true; @@ -133,7 +122,7 @@ private void SplashScreen_Load(object sender, EventArgs e) _channelLabel.Visible = channel.ToLowerInvariant() != "release"; _channelLabel.Text = channel; // No need to localize this: seen only by testers or special users (BL-4451) _copyrightlabel.Text = $"© 2011-{DateTime.Now.Year} SIL Global"; - if (!Program.StartupAutomation && !Program.StartupHeadless) + if (!Program.StartupAutomation) BringToFront(); } diff --git a/src/BloomExe/WebView2Browser.cs b/src/BloomExe/WebView2Browser.cs index 7819ddfffe64..c90fbffb0111 100644 --- a/src/BloomExe/WebView2Browser.cs +++ b/src/BloomExe/WebView2Browser.cs @@ -402,14 +402,6 @@ private async Task InitWebView() { additionalBrowserArgs += " --accept-lang=" + _uiLanguageOfThisRun; } - if (Program.StartupHeadless) - { - // A headless run keeps Bloom's window far off-screen (see Shell.GetHeadlessBounds), - // so Windows reports the window as occluded and Chromium stops rendering it. A - // screenshot of an unrendered page comes back blank, so turn that behavior off. - featuresToDisable.Add("CalculateNativeWinOcclusion"); - additionalBrowserArgs += " --disable-backgrounding-occluded-windows"; - } if (RemoteDebuggingPort.HasValue && !Program.RunningUnitTests) { // Expose a CDP endpoint so Playwright and other automation can attach to the real Bloom WebView2 surface. diff --git a/src/BloomExe/web/controllers/ProblemReportApi.cs b/src/BloomExe/web/controllers/ProblemReportApi.cs index 3eded937d1ba..567ec4e5b05c 100644 --- a/src/BloomExe/web/controllers/ProblemReportApi.cs +++ b/src/BloomExe/web/controllers/ProblemReportApi.cs @@ -1173,16 +1173,10 @@ private static void TryGetScreenshot(Control controlForScreenshotting) { ResetScreenshotFile(); } - else if (IsBloomProcessInForeground() && !Program.StartupHeadless) + else if (IsBloomProcessInForeground()) { // Bloom is the foreground app: a plain screen copy is cheaper // and avoids re-triggering any paint-related bugs. - // - // Not under --headless, though. That run's window sits far - // outside every monitor, so copying from those screen coordinates - // would save whatever the desktop has there, which is nothing. - // Render the window itself instead, the way the not-in-front case - // already does. var scaledBounds = controlForScreenshotting.Bounds; #if !__MonoCS__ scaledBounds = diff --git a/src/BloomTests/ProgramTests.cs b/src/BloomTests/ProgramTests.cs index 4dd021366f6c..f4c68c636f68 100644 --- a/src/BloomTests/ProgramTests.cs +++ b/src/BloomTests/ProgramTests.cs @@ -56,33 +56,6 @@ out var automationErrorMessage Assert.That(Program.StartupRequestedPortSummary, Is.EqualTo("automation=true")); } - [Test] - public void ParseStartupPortArguments_StoresHeadlessFlag() - { - var remainingArgs = Program.ParseStartupPortArguments( - new[] { "--automation", "--headless", @"C:\Temp\Example.bloomcollection" }, - out var errorMessage - ); - - Assert.That(errorMessage, Is.Null); - Assert.That(Program.StartupHeadless, Is.True); - Assert.That(Program.StartupAutomation, Is.True); - Assert.That( - Program.StartupRequestedPortSummary, - Is.EqualTo("automation=true, headless=true") - ); - Assert.That(remainingArgs, Is.EqualTo(new[] { @"C:\Temp\Example.bloomcollection" })); - } - - [Test] - public void ParseStartupPortArguments_LeavesHeadlessFalseWithoutTheFlag() - { - Program.ParseStartupPortArguments(new[] { "--automation" }, out var errorMessage); - - Assert.That(errorMessage, Is.Null); - Assert.That(Program.StartupHeadless, Is.False); - } - [Test] public void ParseStartupPortArguments_VitePortAloneDoesNotEnableAutomation() { From 94134b31419d6d99ca2c87bbf74ea8ad1fd34c08 Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 2 Sep 2026 16:29:57 -0600 Subject: [PATCH 09/10] Do not construct a toolbox tool the toolbox already has registerAllToolboxTools built all eleven tools and then discarded the ones the master list already held. Two of them point a static field at the instance being constructed: CanvasTool.theOneCanvasTool and GameTool.theOneDragActivityTool. So a second call left those fields holding a tool the toolbox does not know about, and a canvas refresh or a game's state then went nowhere. Each entry now names its id and passes a factory, so a tool the list already has is never made. Where a tool names its id in a constant, the entry uses that constant; where it uses a string literal, registerOnce checks the constructed tool's own id against the name it was given and throws if they differ, because nothing else ties the two together. Found by Devin on PR 8276. Co-Authored-By: Claude Opus 5 (1M context) --- .../toolbox/registerAllToolboxTools.ts | 57 ++++++++++++++----- 1 file changed, 42 insertions(+), 15 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts b/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts index 1baf3e8fd6e4..574bbf084c3a 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts @@ -9,6 +9,12 @@ // (AUTOMATION-DEBT.md: "Toolbox tool registration is a side effect of toolboxBootstrap".) import { ITool, ToolBox, getMasterToolList } from "./toolbox"; +import { + kCanvasToolId, + kGameToolId, + kMotionToolId, + kMusicToolId, +} from "./toolIds"; import { DecodableReaderTool } from "./readers/decodableReader/decodableReaderTool"; import { LeveledReaderTool } from "./readers/leveledReader/leveledReaderTool"; import { MusicToolAdaptor } from "./music/musicToolControls"; @@ -25,21 +31,27 @@ import { SettingsTool } from "./settings/settingsTool"; * Make the one instance of each toolbox class and register it with the master toolbox. The * imports above also serve to ensure that each tool's code is part of the bundle. * - * Calling this twice registers nothing the second time. See registerOnce for why the check is - * per tool and reads the shared master list. + * Calling this twice registers nothing the second time, and makes no second instance either. See + * registerOnce for both. */ export function registerAllToolboxTools(): void { - registerOnce(new DecodableReaderTool()); - registerOnce(new LeveledReaderTool()); - registerOnce(new MusicToolAdaptor()); - registerOnce(new ImpairmentVisualizerAdaptor()); - registerOnce(new MotionTool()); - registerOnce(new TalkingBookTool()); - registerOnce(new SignLanguageTool()); - registerOnce(new ImageDescriptionAdapter()); - registerOnce(new CanvasTool()); - registerOnce(new GameTool()); - registerOnce(new SettingsTool()); + registerOnce("decodableReader", () => new DecodableReaderTool()); + registerOnce("leveledReader", () => new LeveledReaderTool()); + registerOnce(kMusicToolId, () => new MusicToolAdaptor()); + registerOnce( + "impairmentVisualizer", + () => new ImpairmentVisualizerAdaptor(), + ); + registerOnce(kMotionToolId, () => new MotionTool()); + registerOnce("talkingBook", () => new TalkingBookTool()); + registerOnce(SignLanguageTool.kToolID, () => new SignLanguageTool()); + registerOnce( + ImageDescriptionAdapter.kToolID, + () => new ImageDescriptionAdapter(), + ); + registerOnce(kCanvasToolId, () => new CanvasTool()); + registerOnce(kGameToolId, () => new GameTool()); + registerOnce("settings", () => new SettingsTool()); } /** @@ -54,9 +66,24 @@ export function registerAllToolboxTools(): void { * The check is per tool, not "is the list empty": a list holding some other tool is not evidence * that these eleven are registered, and skipping all of them on that evidence would leave the * toolbox missing every section. + * + * The caller passes the id and a factory rather than a tool, so that a tool the list already has + * is never constructed. CanvasTool and GameTool each point a static field at the instance being + * constructed (CanvasTool.theOneCanvasTool, GameTool.theOneDragActivityTool), so constructing one + * only to discard it as a duplicate leaves every reader of that field holding a tool the toolbox + * does not know about, and a canvas refresh or a game's state then goes nowhere. */ -function registerOnce(tool: ITool): void { - if (getMasterToolList().some((registered) => registered.id() === tool.id())) +function registerOnce(id: string, makeTool: () => ITool): void { + if (getMasterToolList().some((registered) => registered.id() === id)) return; + const tool = makeTool(); + // The id above has to be the tool's own, or the duplicate check reads the wrong entry and a + // second call registers the tool again. Some of these tools name their id in a constant and + // some in a string literal, so nothing but this check ties the two together. + if (tool.id() !== id) + throw new Error( + `registerAllToolboxTools names the tool "${id}", but the tool calls itself ` + + `"${tool.id()}".`, + ); ToolBox.registerTool(tool); } From 7b82421bbf764d5f94e9bd5195d717727f2f5eae Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 3 Sep 2026 10:55:38 -0600 Subject: [PATCH 10/10] Fix the sign language tool id in registerAllToolboxTools SignLanguageTool.kToolID does not exist. The static kToolID = "signLanguage" belongs to SignLanguageToolControls, which is a different class in the same file. So the id passed to registerOnce was undefined, and the check that the id matches the tool's own id() threw: registerAllToolboxTools names the tool "undefined", but the tool calls itself "signLanguage". toolboxBootstrap.ts calls registerAllToolboxTools() at module load, so the throw stopped that module after six tools. The Edit tab toolbox lost every later tool, and window.toolboxBundle never got assigned. It also failed the seven component tests in react_components/ToolboxRootTestHarness/component-tests/toolbox-root-react.uitest.ts. The typecheck does not catch a missing static, so only running the component-tester suite found it. That suite now passes: 144 passed, 25 skipped, both warm and with the Vite dependency cache deleted. The other ten ids in the list are checked against each tool's id() and are correct. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts b/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts index 574bbf084c3a..403924d6a7b8 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts @@ -44,7 +44,7 @@ export function registerAllToolboxTools(): void { ); registerOnce(kMotionToolId, () => new MotionTool()); registerOnce("talkingBook", () => new TalkingBookTool()); - registerOnce(SignLanguageTool.kToolID, () => new SignLanguageTool()); + registerOnce("signLanguage", () => new SignLanguageTool()); registerOnce( ImageDescriptionAdapter.kToolID, () => new ImageDescriptionAdapter(),