diff --git a/.github/skills/add-e2e-test/SKILL.md b/.github/skills/add-e2e-test/SKILL.md index 25bbc75c8999..d1fafb90738e 100644 --- a/.github/skills/add-e2e-test/SKILL.md +++ b/.github/skills/add-e2e-test/SKILL.md @@ -338,9 +338,19 @@ 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 -`output/{Debug,Release}/{x64,AnyCPU,}/` (build it yourself; see "Build Bloom whenever it -helps") and the inputs at `output/testing-inputs`. Point +A run launches a real Bloom and its window appears on the developer's desktop, unless +`BLOOM_AUTOMATION_MONITOR` says otherwise. That one variable decides where every window a run +opens goes, the splash screen included: `headless`, or `0`, puts them all off every monitor; a +1-based monitor number puts them on that monitor; and any other value, unset included, leaves Bloom +to place them as it always does. `headless` moves the window off-screen rather than minimizing it, +because WebView2 stops painting a minimized window and every screenshot then comes back blank. +`--debug` clears a `headless` setting, so a debug session has a window to step through. The monitor +number counts left to right, so 1 is the leftmost monitor; it is **not** the number Windows Settings +shows beside each display, and no API reproduces those. Bloom writes the whole mapping to its log at +startup. See `src/BloomE2E/README.md` for the table and that log line. + +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 collections instead of the pinned ones. diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 1580914ccfd6..d5e120d1eeb5 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -273,6 +273,12 @@ jobs: # The html reporter would otherwise try to serve the report at the end of a # failing run, which on a runner means a step that never returns. PLAYWRIGHT_HTML_OPEN: never + # Put every window this run opens off every monitor. Bloom obeys this variable + # only under --automation, which is what the fixture launches; see + # src/BloomE2E/README.md. The runner has a desktop, so a visible window would + # also work, but asking for it here says what the run needs instead of leaving + # it to whatever the runner happens to do. + BLOOM_AUTOMATION_MONITOR: headless run: pnpm test --reporter=list,junit,html # What explains an e2e failure: Playwright's HTML report, plus the trace and failure diff --git a/src/BloomE2E/AUTOMATION-DEBT.md b/src/BloomE2E/AUTOMATION-DEBT.md index f489f060e2d8..bb878139c3c0 100644 --- a/src/BloomE2E/AUTOMATION-DEBT.md +++ b/src/BloomE2E/AUTOMATION-DEBT.md @@ -287,3 +287,100 @@ the file said `en` again a moment later. The same test has to restore the zoom i that setting is shared too. Fix direction: under `--e2e`, point the settings provider at a per-instance folder (a sibling of the temp collection would do), so a test's Bloom starts from defaults and its changes die with it. + +## No way to run the suite at a chosen monitor resolution and scale factor + +Every run takes the resolution and the scale factor of whatever monitor it lands on, so a +suite proves the layout only at the DPI of the machine that ran it. That is exactly where a +class of Bloom bugs lives: a control that fits at 100% and overlaps at 150%, a dialog that +opens off the edge on a short screen, a size computed in one coordinate space and used in +another. A developer at 150% and a CI runner at 100% each pass while the other's bug goes +unseen, and neither can reproduce what a user reports. + +Found 2026-09-03, twice in one change (BL-16804), which is what makes this worth scheduling: + +- The off-screen window asked for the primary monitor's working-area size, and Windows + interpreted that size at the scale factor of the nearest monitor. On a machine with a 150% + primary and a 100% monitor beside it, a window meant to be 3840x2100 came out 3840x2100 real + pixels, taller than any monitor on the machine. `format-gear-positioning.spec.ts` failed + because the page viewport was 1990 CSS pixels high, a size no user has. The same mismatch, + in its first guise, had eaten all but 27 pixels of a 1000-pixel off-screen cushion. +- Both bugs passed every unit test, because a unit test compares numbers inside one process's + own coordinate space. Only a real window at a real scale factor shows them. + +Fix direction, cheapest first, none of it tried yet: + +- **An RDP session to the machine.** An `.rdp` file takes `desktopwidth`, `desktopheight` and + `desktopscalefactor` (100, 125, 150, 175, 200), so one connection per combination gives a + real desktop at a chosen scale with no driver to install. This looks like the least work and + the most likely to run in CI, but nobody has tried driving the suite inside one. +- **A virtual display driver.** Windows has an indirect-display driver model (IddCx), and + several drivers built on it create a monitor with no hardware behind it, at a resolution the + driver is told to offer. Setting that monitor's *scale factor* is the harder half: Windows + exposes per-monitor scale only through display-config calls Microsoft does not document. + Worth an afternoon of investigation before committing to it. +- **A virtual machine or Windows Sandbox** at a chosen resolution and scale. Heaviest, but it + is the only one that also isolates the shared `user.config` described above. + +Whatever the mechanism, the suite needs the same thing from it: a way to say "run these tests +at 1920x1080 at 150%" and have the run either honour it or refuse, rather than silently using +the desktop it found. + +One piece of this is a known limit in the code already, and it is what the fix direction above +would settle. `AutomationWindowPlacement.GetBoundsOffEveryMonitor` puts an off-screen window +directly below the primary monitor, because the nearest monitor is the one whose scale factor +Windows applies, and on the layouts we have that keeps the primary nearest. It stops being true +when a monitor sits *below* the primary in the same band of x: that lower monitor is then nearest, +and if its scale factor differs the window comes out the wrong size, which is the same bug in a +new layout. Fixing it properly means asking Windows for the nearest monitor's scale factor and +scaling the requested size by the ratio, which needs the per-monitor DPI calls this entry is +about. Nobody on the team has such a layout today, which is why it is written down rather than +fixed. (Devin raised it on PR 8285, 2026-09-03.) + +## Every run takes the developer's window size, so small-screen bugs go unseen + +A run makes its window as big as the monitor it lands on, so the suite proves the layout only at +the size of a developer's screen. Many Bloom users are on inexpensive machines with small screens, +and that is where a class of bugs lives that nobody on the team meets: a control that overlaps +another, a dialog that opens past an edge, a toolbar that quietly drops an item. This is the +window-size half of the DPI entry above, and it is much cheaper to fix, because it needs no +virtual monitor. + +The plan: give every automation run a window of **1024x586**, the working area of a 1024x768 +screen once a task bar of the usual height is taken off, wherever the window goes. +`BLOOM_AUTOMATION_WINDOW_SIZE=1600x900` asks for a different size, for chasing a bug that only +shows on a big screen. The floor is 400x300, which is `Shell.MinimumSize`; anything Bloom cannot +use, a typo included, gives the default rather than a broken run. The size must be the same for +all three values of `BLOOM_AUTOMATION_MONITOR`, so that variable decides only *where* a window +goes: a suite whose size changed with its placement would let one test pass in one mode and fail +in another, a trap that caught this code twice on BL-16804. + +The work is not the window size, which is about thirty lines in `AutomationWindowPlacement.cs` and +`Shell.cs`. The work is the suite going red, which is the point of the change. One full run of the +35 tests at 1024x586 on 2026-09-03 gave **16 passed, 6 failed, 13 did not run**, against 23 +passed and 2 failed at the size of a developer's monitor. Two of the six fail at either size, so +they are not the window's doing: Test Case ID 349 (BL-16807) and Test Case ID 606, which times out +after 60 seconds waiting for the publish-to-web steps. The small window is what added these four: + +- `copy-page.spec.ts:85` (Test Case ID 348), failed in 8 seconds. +- `derivative-keeps-template-pages.spec.ts:106` (Test Case ID 72), failed after 48 seconds. +- `format-gear-positioning.spec.ts:130` (Test Case ID 356), failed in 335 ms: the Format dialog no + longer opened close to its gear, while the test above it in the same file passed. So the small + window moved the dialog. +- `publish-text-languages.spec.ts:416` (Test Case ID 169), failed after 37 seconds. Read this one + with care: it is BL-16806, which is machine-dependent, and it passed in the full-size run of the + same build. So the window may have caused it or may not. + +Because the suite is serial per file, those 6 failures also stop 13 more tests from running, so +the small window costs 7 passes and hides 13 results until the fixes land. + +Each failure then needs triage into one of two piles, and the second pile is the reason to do any +of this: either the test assumed a large window and has to be rewritten, or **Bloom itself +misbehaves at 1024x586**, which is a real user-facing bug and wants its own card. Timeouts rather +than quick failures are the common failure mode, so the suite is also much slower while the fixes +are outstanding. Whoever picks this up has to decide what the nightly workflow does in the +meantime: run small and stay red, or stay large until the tests are fixed. + +(Written and measured on 2026-09-03 during BL-16804, then deliberately taken back out: the +developer chose to record the plan here rather than carry a red suite. The code is not in the +history, so rebuilding it from this entry is part of the job.) diff --git a/src/BloomE2E/README.md b/src/BloomE2E/README.md index a21850ea1240..c7f58794d51d 100644 --- a/src/BloomE2E/README.md +++ b/src/BloomE2E/README.md @@ -135,7 +135,62 @@ 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 window on your desktop. That is expected; do not click in it. + +### Where the Bloom window goes: `BLOOM_AUTOMATION_MONITOR` + +One environment variable decides where every window an e2e run opens goes, the main window and +the splash screen alike. Set it in your shell, or per run: + +| Value | What happens | +| --- | --- | +| `headless`, or `0` | Every window opens far outside every monitor. You see nothing, so a run can go on while you work. | +| a 1-based monitor number, counted left to right | Every window opens on that monitor, so a run stays off the one you are working on. | +| unset, or anything else | Bloom places its windows as it always does, and you see the run. | + +```bash +BLOOM_AUTOMATION_MONITOR=headless pnpm test # see nothing +BLOOM_AUTOMATION_MONITOR=0 pnpm test # the same thing: no monitor at all +BLOOM_AUTOMATION_MONITOR=2 pnpm test # on the second monitor from the left +pnpm test # wherever Bloom normally opens +``` + +A value Bloom cannot use, a typo or a monitor you do not have, counts as "anything else": you get +a visible window, which is exactly what tells you the variable did not take effect. A setting that +hid the window on a typo would leave you nothing to notice. + +**The number counts left to right, which is not the number Windows Settings shows.** Monitor 1 is +your leftmost monitor, 2 the next one to the right, and so on, matching the arrangement picture in +Windows Settings but not the numbers printed on it. Windows does not document how the Settings app +makes those numbers, and no API reproduces them: on one three-monitor machine Windows Settings said +1 (primary, centre), 2 (right) and 3 (left), while left to right is 1 (left), 2 (primary, centre) +and 3 (right). So read the arrangement, not the numbers on it. Bloom also writes the whole mapping +to `%TEMP%\SIL\Bloom\Log.txt` on every automation start: + +``` +BLOOM_AUTOMATION_MONITOR='2': every window goes on the monitor at {X=0,Y=0,Width=2560,Height=1440}. +The monitors this process sees, numbered left to right as this variable numbers them (which is NOT +how Windows Settings numbers them): 1=(-1920,601) 1920x1200, 2=(0,0) 2560x1440 primary, +3=(3840,432) 1920x1200. +``` + +`headless` moves the window off-screen rather than minimizing or hiding it, because WebView2 stops +painting a minimized window, which would make every screenshot blank. Off-screen the window paints +exactly as it would in front of you, so rendering and keyboard input behave the same. + +It goes **below** your primary monitor, not off to one side, and that matters on a machine whose +monitors run at different scale factors. Windows gives a window the scale factor of the monitor +nearest to it. A window out to the left would take the leftmost monitor's scale factor while +carrying a size measured in the primary's, and on a 150% primary beside a 100% monitor that made a +window 3840x2100 real pixels, taller than any monitor on the machine, with a page viewport no user +could have. Directly below the primary, the primary stays the nearest monitor and the size is +right. See `AutomationWindowPlacement.GetBoundsOffEveryMonitor`. + +`--debug` clears a `headless` setting for you: stepping through a test whose window you cannot see +is pointless. A setting that names a monitor is left alone, because that window is visible anyway. + +The variable applies only to a run under `--automation`, which is every e2e run and nothing else. +A Bloom you start yourself is unaffected, however the variable is set. 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 7b8e6c566e8c..7fb7c640626f 100644 --- a/src/BloomE2E/fixtures/launchBloom.ts +++ b/src/BloomE2E/fixtures/launchBloom.ts @@ -158,6 +158,26 @@ function samePath(a: string, b: string): boolean { const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); +/** + * The environment the Bloom we launch runs in. One variable decides where its windows go, + * BLOOM_AUTOMATION_MONITOR, and Bloom reads it itself (see AutomationWindowPlacement.cs): + * "headless" puts every window off every monitor, a monitor number puts them on that monitor, and + * anything else, the variable being unset included, leaves Bloom to place its windows as it always + * does. So the child inherits this process's environment untouched, with one exception. + * + * The exception is Playwright's --debug (which sets PWDEBUG): stepping through a test whose window + * nobody can see is pointless, so a debug session clears a "headless" setting, or the "0" that says + * the same thing, and gets a window. A setting that names a monitor is left alone, because that + * window IS visible. + */ +function environmentForBloom(): NodeJS.ProcessEnv { + const asked = process.env.BLOOM_AUTOMATION_MONITOR?.trim().toLowerCase(); + if (process.env.PWDEBUG && (asked === "headless" || asked === "0")) { + return { ...process.env, BLOOM_AUTOMATION_MONITOR: "" }; + } + return process.env; +} + /** What common/instanceInfo tells us about a running Bloom. Only the fields we use. */ interface IInstanceInfo { editableCollectionFolder?: string; @@ -362,12 +382,12 @@ 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", - ]); + // --automation: let this instance run alongside a Bloom the developer already has open, and + // let BLOOM_AUTOMATION_MONITOR say where its windows go (see environmentForBloom). + const args = [findCollectionFile(collectionDir), "--e2e", "--automation"]; + const bloomProcess: ChildProcess = execFile(exe, args, { + env: environmentForBloom(), + }); 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/BloomExe/AutomationWindowPlacement.cs b/src/BloomExe/AutomationWindowPlacement.cs new file mode 100644 index 000000000000..899e415a3b57 --- /dev/null +++ b/src/BloomExe/AutomationWindowPlacement.cs @@ -0,0 +1,267 @@ +using System; +using System.Drawing; +using System.Linq; +using System.Windows.Forms; + +namespace Bloom +{ + /// + /// What an automation run (--automation, e.g. the Playwright suites) does with the windows it + /// opens. One environment variable decides it, BLOOM_AUTOMATION_MONITOR: + /// + /// "2" (a 1-based monitor number, counted left to right) every window opens on that monitor + /// "headless", or "0" for no monitor at all every window opens off every monitor + /// absent, empty, or anything else Bloom places windows as it always does + /// + /// The variable applies ONLY under --automation, and that is a wider set of runs than the test + /// suites: ./go.sh passes --automation as well (scripts/watchBloomExe.mjs), so a Bloom started + /// to work in obeys the variable too. That is deliberate, and it is what the variable is for: + /// the developer sets it once and every Bloom an agent starts stays off the monitor they are + /// working on. Two things follow that are worth knowing before changing this code. + /// + /// First, "headless" therefore hides a ./go.sh Bloom the developer started THEMSELVES, and a + /// hidden Bloom looks exactly like one that failed to start: the launcher reports success and + /// the HTTP server answers, but no window exists anywhere. The log line DescribeChoice writes + /// on every automation start is what settles that, so leave it in place. + /// + /// Second, a Bloom the developer starts with no --automation flag at all is untouched, however + /// the variable is set. + /// + /// Nothing here changes whether an automation window takes the keyboard focus. It never does, + /// wherever it is (see Shell.ShowWithoutActivation and Shell.ReallyComeToFront). + /// + /// The variable is read on each call rather than cached. It costs nothing worth saving, and + /// nothing in Bloom writes it: the run's parent, normally the Playwright fixture, sets it + /// before Bloom starts, so every caller gets the same answer. That matters because the callers + /// have to agree. Shell places the window, and WebView2Browser turns off the occlusion check + /// for an off-screen one; if those two disagreed, every screenshot would come back blank. + /// + public static class AutomationWindowPlacement + { + /// The value of BLOOM_AUTOMATION_MONITOR that asks for off-screen windows. + public const string HeadlessSetting = "headless"; + + /// The number that says the same thing as HeadlessSetting: no monitor at all. + public const string NoMonitorSetting = "0"; + + public const string VariableName = "BLOOM_AUTOMATION_MONITOR"; + + /// Where an automation run puts its windows. + public enum Choice + { + /// + /// Bloom places its windows the way it normally does, so a run appears on the + /// developer's desktop. This is what an absent or unrecognized value means: nobody + /// asked for anything, so nothing is imposed. + /// + AsBloomNormallyWould, + + /// The variable named a monitor that exists. Every window goes on it. + OnTheChosenMonitor, + + /// The variable said "headless". Every window goes off every monitor. + OffEveryMonitor, + } + + /// + /// Read BLOOM_AUTOMATION_MONITOR and say what this run should do. Answers + /// AsBloomNormallyWould for a run that is not automation, whatever the variable says. + /// + public static Choice GetChoice() + { + if (!Program.StartupAutomation) + return Choice.AsBloomNormallyWould; + return Parse( + Environment.GetEnvironmentVariable(VariableName), + Screen.AllScreens.Length, + out _ + ); + } + + /// + /// Turn one raw value of BLOOM_AUTOMATION_MONITOR into a choice. Split out from GetChoice + /// so it can be tested without an environment variable or a real set of monitors. + /// + /// A value naming a monitor this machine does not have counts as unrecognized, and so does + /// a typo: both mean Bloom places its windows normally. That is deliberate. The developer + /// then SEES a Bloom window, which is the outcome that tells them the variable did not take + /// effect; a typo that silently hid the window would leave them with no way to notice. + /// + /// The raw value, or null when the variable is not set. + /// How many monitors this machine has. + /// + /// The monitor asked for, when the answer is OnTheChosenMonitor; 0 otherwise. + /// + public static Choice Parse(string setting, int screenCount, out int oneBasedMonitor) + { + oneBasedMonitor = 0; + var trimmed = setting?.Trim(); + if (string.IsNullOrEmpty(trimmed)) + return Choice.AsBloomNormallyWould; + if (trimmed.Equals(HeadlessSetting, StringComparison.OrdinalIgnoreCase)) + return Choice.OffEveryMonitor; + // "0" says the same thing as "headless": no monitor at all. It reads as "none" beside + // the numbers that name a monitor, and it is quicker to type. + if (trimmed == NoMonitorSetting) + return Choice.OffEveryMonitor; + if (int.TryParse(trimmed, out var index) && index >= 1 && index <= screenCount) + { + oneBasedMonitor = index; + return Choice.OnTheChosenMonitor; + } + return Choice.AsBloomNormallyWould; + } + + /// + /// True when this run keeps its windows off every monitor. Read this rather than the + /// variable: it is also false for a run that is not automation. + /// + public static bool IsOffEveryMonitor => GetChoice() == Choice.OffEveryMonitor; + + /// + /// This machine's monitors in the order the variable numbers them: left to right, so + /// monitor 1 is the leftmost. Two monitors at the same horizontal position, one above the + /// other, come out top first. + /// + /// The order is by position rather than the order Screen.AllScreens happens to return, + /// which is the order of the display drivers and means nothing a developer can see. A + /// developer reads a number off the picture of their monitors, so the number has to follow + /// the picture. + /// + /// This is still NOT the number Windows Settings prints on each monitor. Windows does not + /// document how the Settings app makes those labels, and neither the AllScreens order nor + /// the \\.\DISPLAY<n> device name nor the display-config path order reproduces them: on + /// one three-monitor machine Windows Settings said 1 (primary, centre), 2 (right) and + /// 3 (left), while left to right is 1 (left), 2 (primary, centre) and 3 (right). So Bloom + /// counts left to right, which a developer can work out from the arrangement they see, + /// and DescribeChoice writes the whole mapping to the log. + /// + public static Screen[] MonitorsLeftToRight() + { + return Screen + .AllScreens.OrderBy(screen => screen.Bounds.Left) + .ThenBy(screen => screen.Bounds.Top) + .ToArray(); + } + + /// + /// The monitor an automation run opens its windows on. Only meaningful when the choice is + /// OnTheChosenMonitor; falls back to the primary screen otherwise, so that callers wanting + /// a size rather than a position have one. + /// + public static Screen GetChosenMonitor() + { + if ( + Program.StartupAutomation + && Parse( + Environment.GetEnvironmentVariable(VariableName), + Screen.AllScreens.Length, + out var oneBasedMonitor + ) == Choice.OnTheChosenMonitor + ) + { + return MonitorsLeftToRight()[oneBasedMonitor - 1]; + } + return Screen.PrimaryScreen; + } + + /// + /// One line for the log saying what the variable said and what this run did with it, + /// naming every monitor by position and size. + /// + /// This exists because the number in the variable counts left to right, which is NOT the + /// number Windows Settings prints beside each display; see MonitorsLeftToRight. A developer + /// who reads a number off Windows Settings therefore gets a different monitor, and nothing + /// on screen says so. The log line is what lets them see which monitor Bloom actually + /// chose, and work out the number they want. + /// + public static string DescribeChoice() + { + var raw = Environment.GetEnvironmentVariable(VariableName); + var monitors = string.Join( + ", ", + MonitorsLeftToRight() + .Select( + (screen, zeroBased) => + $"{zeroBased + 1}=({screen.Bounds.X},{screen.Bounds.Y}) " + + $"{screen.Bounds.Width}x{screen.Bounds.Height}" + + (screen.Primary ? " primary" : "") + ) + ); + var what = GetChoice() switch + { + Choice.OffEveryMonitor => "every window goes off every monitor", + Choice.OnTheChosenMonitor => + $"every window goes on the monitor at {GetChosenMonitor().Bounds}", + _ => "Bloom places its windows as it always does", + }; + return $"{VariableName}={(raw == null ? "(not set)" : $"'{raw}'")}: {what}. " + + $"The monitors this process sees, numbered left to right as this variable " + + $"numbers them (which is NOT how Windows Settings numbers them): {monitors}."; + } + + /// + /// Where an off-every-monitor run puts a window: the size of the primary screen's working + /// area, positioned below every monitor and in line with the primary, so that not one + /// pixel of it is on any screen and it still gets the primary's scale factor. + /// + /// 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 GetBoundsOffEveryMonitor() + { + var workingArea = Screen.PrimaryScreen.WorkingArea; + var size = workingArea.Size; + + // The window goes straight DOWN from the primary monitor, not off to the left, and the + // reason is the DPI of the monitor Windows thinks the window is on. + // + // Windows gives a window the scale factor of the monitor nearest to it, and this + // process asked for a size in the primary monitor's own scaled pixels. Put the window + // out to the left and the nearest monitor is the leftmost one, whose scale factor is + // very likely not the primary's: on a machine whose primary runs at 150% and whose + // left monitor runs at 100%, a window meant to match the primary's 3840x2100 came out + // 3840x2100 REAL pixels, taller than any monitor on the machine, and the page inside + // it laid out at a viewport height no user could ever have. Keeping the window + // directly under the primary, aligned with its left edge, keeps the primary the + // nearest monitor on the layouts we have, so an off-screen window paints at the size + // a visible one would. + // + // "On the layouts we have" is the real limit here, and it is worth stating plainly. + // The primary is nearest only while no monitor sits below the primary in the same + // band of x. A machine with one monitor stacked under another, at a different scale + // factor, puts that lower monitor nearest instead, and the size comes out wrong in + // exactly the way described above. Getting it right for every layout means asking + // Windows for the scale factor of whichever monitor ends up nearest and scaling the + // requested size by the ratio, which is more than this code does today. See "No way + // to run the suite at a chosen monitor resolution and scale factor" in + // src/BloomE2E/AUTOMATION-DEBT.md. + // + // How far down: far enough that the window clears every monitor even after Windows + // scales it. This process's idea of the height can be out by the ratio of two scale + // factors, and Windows scales a monitor by at most 400%, so a window this process + // believes is H high covers at most 4H pixels of the desktop. Four heights below the + // lowest monitor therefore clears them all, and the 1000 pixels on top of that keep + // the two edges from meeting exactly. (An earlier version of this went left and left + // one window width of room; on a 160% primary that left 27 pixels of a 1000-pixel + // cushion, which is how the ratio came to be measured rather than guessed.) + // + // 32000 is as far down as a window may go: Windows still places a window there, and + // anything past about 32768 runs into the 16-bit coordinates that some of the older + // window messages still carry. On any real layout four heights is several thousand + // pixels, well inside that. A downward run of monitors more than about 30000 pixels + // tall 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 farDownWindowsAllows = 32000; + const int largestScaleFactorWindowsAllows = 4; + var lowestY = Screen.AllScreens.Max(screen => screen.Bounds.Bottom); + var y = Math.Min( + farDownWindowsAllows, + lowestY + (size.Height * largestScaleFactorWindowsAllows) + 1000 + ); + return new Rectangle(workingArea.X, y, size.Width, size.Height); + } + } +} diff --git a/src/BloomExe/Shell.cs b/src/BloomExe/Shell.cs index 54cf046b558e..fed313bf7b55 100644 --- a/src/BloomExe/Shell.cs +++ b/src/BloomExe/Shell.cs @@ -48,30 +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. + // not steal the user's keyboard focus when it is shown. That holds wherever the window + // is: on the developer's desktop, on a monitor of its own, or off every monitor. protected override bool ShowWithoutActivation => Program.StartupAutomation; - /// - /// The screen that an automation run (--automation) should open windows on: the one - /// chosen by the BLOOM_AUTOMATION_MONITOR environment variable (a 1-based index into - /// Screen.AllScreens), or the primary screen when the variable is absent or out of - /// range. Without this, Windows opens each test-launched window on whichever monitor - /// the user is currently working on. - /// - public static Screen GetAutomationScreen() - { - var setting = Environment.GetEnvironmentVariable("BLOOM_AUTOMATION_MONITOR"); - if ( - int.TryParse(setting, out var oneBasedIndex) - && oneBasedIndex >= 1 - && oneBasedIndex <= Screen.AllScreens.Length - ) - { - return Screen.AllScreens[oneBasedIndex - 1]; - } - return Screen.PrimaryScreen; - } - public Shell( Func projectViewFactory, CollectionSettings collectionSettings, @@ -91,6 +71,19 @@ AudioRecording audioRecording _controlKeyEvent = controlKeyEvent; _audioRecording = audioRecording; InitializeComponent(); + if (AutomationWindowPlacement.IsOffEveryMonitor) + { + // Keep the off-screen window out of the task bar, so such a run leaves no trace + // on the developer's desktop. + // + // This has to happen before the window handle exists, which is why it is here + // and not in Shell_Load with the rest of the headless placement. Assigning + // ShowInTaskbar on a form that is already showing makes Windows Forms recreate + // the form's handle, and every child handle with it, including the WebView2 + // host. The Edit tab survived that with a browser that no longer answered a + // jump to another page, so every e2e test that moves between pages hung. + ShowInTaskbar = false; + } Activated += (sender, args) => { // In at least one case (BL-15060) we seem to have gotten activated @@ -424,7 +417,9 @@ 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. + // from whatever they are doing while tests run, and a window placed off every + // monitor cannot come to the front at all: TopMost and BringToFront on it would + // take the foreground away for nothing. 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 @@ -446,17 +441,49 @@ private void Shell_Load(object sender, EventArgs e) { SuspendLayout(); + // Where an automation run puts its window is BLOOM_AUTOMATION_MONITOR's to + // decide (see AutomationWindowPlacement). A run that it says nothing about falls + // through to the ordinary cases below, window placement and all, so the + // developer sees the Bloom they would see without the variable. + var placement = AutomationWindowPlacement.GetChoice(); 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. - // Pin the window to the automation screen (see GetAutomationScreen). + // Say in the log which monitor this run chose and what the alternatives were. + // The number in the variable is not the number Windows Settings shows; see + // AutomationWindowPlacement.DescribeChoice. + Logger.WriteEvent(AutomationWindowPlacement.DescribeChoice()); + } + if (placement == AutomationWindowPlacement.Choice.OffEveryMonitor) + { + // The window goes off every monitor and out of the task bar, so a test can + // run while the developer works. It stays Normal (not minimized) and full + // size, because WebView2 only paints a window that is neither minimized nor + // hidden. See AutomationWindowPlacement.GetBoundsOffEveryMonitor. + StartPosition = FormStartPosition.Manual; + WindowState = FormWindowState.Normal; + Bounds = AutomationWindowPlacement.GetBoundsOffEveryMonitor(); + // ShowInTaskbar is set in the constructor, not here. See the comment there. + } + else if (placement == AutomationWindowPlacement.Choice.OnTheChosenMonitor) + { + // Open on the monitor the variable named, not on whichever one the developer + // is working on, and leave the saved window placement alone. StartPosition = FormStartPosition.Manual; WindowState = FormWindowState.Normal; - Bounds = GetAutomationScreen().WorkingArea; + Bounds = AutomationWindowPlacement.GetChosenMonitor().WorkingArea; // Maximizing keeps the window on the screen that contains its bounds. WindowState = FormWindowState.Maximized; } + else if (Program.StartupAutomation) + { + // An automation run the variable said nothing about: open exactly where a + // Bloom with no saved placement opens, and write nothing. The guard below + // keeps it from touching the developer's saved placement either way, which + // it must not do whatever the variable says: every Bloom of one build shares + // one user.config. + StartPosition = FormStartPosition.WindowsDefaultLocation; + WindowState = FormWindowState.Maximized; + } else if (Settings.Default.WindowSizeAndLocation == null) { StartPosition = FormStartPosition.WindowsDefaultLocation; @@ -470,7 +497,7 @@ private void Shell_Load(object sender, EventArgs e) // Bloom to open in the same place / size each time. if (Program.StartupAutomation) { - // Placement is already pinned above; leave the user's saved placement alone. + // Placement is settled above; leave the developer's saved placement alone. } else if (Settings.Default.MaximizeWindow == false) { @@ -530,6 +557,11 @@ private void Shell_ResizeEnd(object sender, EventArgs e) return; if (WindowState != FormWindowState.Normal) return; + // An automation run must never write the saved bounds. Where BLOOM_AUTOMATION_MONITOR + // put its window is somewhere the developer is not looking, off every monitor or on a + // monitor of the run's choosing, and saving that would move their next Bloom there. + if (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..c3962ec96cd2 100644 --- a/src/BloomExe/SplashScreen.cs +++ b/src/BloomExe/SplashScreen.cs @@ -29,18 +29,34 @@ public void FadeAndClose() } // During an automation run (--automation) the splash must not steal the user's - // keyboard focus when it is shown. + // keyboard focus when it is shown. That holds wherever the splash is. protected override bool ShowWithoutActivation => Program.StartupAutomation; private SplashScreen() { InitializeComponent(); - if (Program.StartupAutomation) + // The splash obeys BLOOM_AUTOMATION_MONITOR exactly as the main window does (see + // AutomationWindowPlacement). It is the second window every run opens, so a splash + // that ignored the variable would put a window on the developer's desktop however + // carefully the main one was placed. + var placement = AutomationWindowPlacement.GetChoice(); + if (placement == AutomationWindowPlacement.Choice.OffEveryMonitor) { - // An automation run must not open on whichever monitor the user is currently - // working on. Center the splash on the automation screen instead. + // Off every monitor with the main window, and out of the task bar with it. The + // splash normally has a task bar entry, and an off-screen window with one is + // worse than either: the developer gets a task bar button for a window they + // cannot bring into view. + StartPosition = FormStartPosition.Manual; + var offScreenArea = AutomationWindowPlacement.GetBoundsOffEveryMonitor(); + Location = new System.Drawing.Point(offScreenArea.Left, offScreenArea.Top); + ShowInTaskbar = false; + } + else if (placement == AutomationWindowPlacement.Choice.OnTheChosenMonitor) + { + // Center the splash on the monitor the variable named, rather than on whichever + // one the developer is working on. + var area = AutomationWindowPlacement.GetChosenMonitor().WorkingArea; StartPosition = FormStartPosition.Manual; - var area = Shell.GetAutomationScreen().WorkingArea; Location = new System.Drawing.Point( area.Left + (area.Width - Width) / 2, area.Top + (area.Height - Height) / 2 @@ -111,7 +127,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. + // from whatever they are doing while tests run, and a splash off every monitor + // cannot come to the front at all. 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 diff --git a/src/BloomExe/WebView2Browser.cs b/src/BloomExe/WebView2Browser.cs index e47eb7f19b2c..d70b68c34e8b 100644 --- a/src/BloomExe/WebView2Browser.cs +++ b/src/BloomExe/WebView2Browser.cs @@ -396,6 +396,15 @@ private async Task InitWebView() { additionalBrowserArgs += " --accept-lang=" + _uiLanguageOfThisRun; } + if (AutomationWindowPlacement.IsOffEveryMonitor) + { + // This run keeps Bloom's window far off-screen (see + // AutomationWindowPlacement.GetBoundsOffEveryMonitor), 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 567ec4e5b05c..8b144b12fc5d 100644 --- a/src/BloomExe/web/controllers/ProblemReportApi.cs +++ b/src/BloomExe/web/controllers/ProblemReportApi.cs @@ -1173,10 +1173,18 @@ private static void TryGetScreenshot(Control controlForScreenshotting) { ResetScreenshotFile(); } - else if (IsBloomProcessInForeground()) + else if ( + IsBloomProcessInForeground() + && !AutomationWindowPlacement.IsOffEveryMonitor + ) { // Bloom is the foreground app: a plain screen copy is cheaper // and avoids re-triggering any paint-related bugs. + // + // Not when the window is off every monitor, though. 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/AutomationWindowPlacementTests.cs b/src/BloomTests/AutomationWindowPlacementTests.cs new file mode 100644 index 000000000000..ea9bc4bac2f4 --- /dev/null +++ b/src/BloomTests/AutomationWindowPlacementTests.cs @@ -0,0 +1,249 @@ +using System; +using System.Linq; +using Bloom; +using NUnit.Framework; +using Choice = Bloom.AutomationWindowPlacement.Choice; + +namespace BloomTests +{ + /// + /// Covers how BLOOM_AUTOMATION_MONITOR is read. These call + /// AutomationWindowPlacement.Parse directly, so they need neither the environment variable + /// nor a machine with a particular set of monitors: the number of monitors is an argument. + /// + [TestFixture] + public class AutomationWindowPlacementTests + { + /// + /// ParseStartupPortArguments writes into Program statics that live for the rest of the + /// test run; re-parse empty args after each test to restore the defaults (the method + /// resets them all on entry). Same rationale as the ProgramTests TearDown. + /// + [TearDown] + public void TearDown() + { + Program.ParseStartupPortArguments(Array.Empty(), out _); + Environment.SetEnvironmentVariable(AutomationWindowPlacement.VariableName, null); + } + + [TestCase(null, TestName = "Parse_VariableNotSet_PlacesWindowsNormally")] + [TestCase("", TestName = "Parse_VariableEmpty_PlacesWindowsNormally")] + [TestCase(" ", TestName = "Parse_VariableAllSpaces_PlacesWindowsNormally")] + public void Parse_NothingAsked_PlacesWindowsNormally(string setting) + { + Assert.That( + AutomationWindowPlacement.Parse(setting, 3, out var monitor), + Is.EqualTo(Choice.AsBloomNormallyWould) + ); + Assert.That(monitor, Is.EqualTo(0)); + } + + [TestCase("headless")] + [TestCase("HEADLESS")] + [TestCase("Headless")] + [TestCase(" headless ")] + [TestCase("0", TestName = "Parse_ZeroMonitor_GoesOffEveryMonitor")] + [TestCase(" 0 ", TestName = "Parse_ZeroMonitorWithSpaces_GoesOffEveryMonitor")] + public void Parse_Headless_GoesOffEveryMonitor(string setting) + { + Assert.That( + AutomationWindowPlacement.Parse(setting, 3, out var monitor), + Is.EqualTo(Choice.OffEveryMonitor) + ); + Assert.That(monitor, Is.EqualTo(0), "No monitor was chosen, so none is reported."); + } + + [TestCase("1", 1)] + [TestCase("2", 2)] + [TestCase("3", 3)] + [TestCase(" 2 ", 2)] + public void Parse_MonitorThatExists_GoesOnThatMonitor(string setting, int expected) + { + Assert.That( + AutomationWindowPlacement.Parse(setting, 3, out var monitor), + Is.EqualTo(Choice.OnTheChosenMonitor) + ); + Assert.That(monitor, Is.EqualTo(expected)); + } + + /// + /// A monitor this machine does not have, a zero or negative index, and a typo all mean the + /// same thing: nobody asked for anything Bloom can honour, so it places its windows + /// normally and the developer sees the window. See the remarks on Parse. + /// + [TestCase("4", TestName = "Parse_MonitorBeyondTheLast_PlacesWindowsNormally")] + [TestCase("-1", TestName = "Parse_NegativeMonitor_PlacesWindowsNormally")] + [TestCase("headles", TestName = "Parse_HeadlessMisspelt_PlacesWindowsNormally")] + [TestCase("true", TestName = "Parse_Nonsense_PlacesWindowsNormally")] + [TestCase("2.5", TestName = "Parse_NotAWholeNumber_PlacesWindowsNormally")] + public void Parse_UnusableValue_PlacesWindowsNormally(string setting) + { + Assert.That( + AutomationWindowPlacement.Parse(setting, 3, out var monitor), + Is.EqualTo(Choice.AsBloomNormallyWould) + ); + Assert.That(monitor, Is.EqualTo(0)); + } + + [Test] + public void Parse_OnlyOneMonitor_TakesTheFirstAndRefusesTheSecond() + { + Assert.That( + AutomationWindowPlacement.Parse("1", 1, out _), + Is.EqualTo(Choice.OnTheChosenMonitor) + ); + Assert.That( + AutomationWindowPlacement.Parse("2", 1, out _), + Is.EqualTo(Choice.AsBloomNormallyWould), + "Monitor 2 does not exist on a one-monitor machine." + ); + } + + /// + /// The variable is only for automation runs. A developer who leaves it set in their shell + /// must still get an ordinary, visible Bloom when they start one themselves. + /// + [Test] + public void GetChoice_WithoutTheAutomationFlag_IgnoresTheVariable() + { + Environment.SetEnvironmentVariable( + AutomationWindowPlacement.VariableName, + AutomationWindowPlacement.HeadlessSetting + ); + + Program.ParseStartupPortArguments(Array.Empty(), out var errorMessage); + Assert.That(errorMessage, Is.Null); + Assert.That( + Program.StartupAutomation, + Is.False, + "Sanity check: this test needs a run that is NOT automation." + ); + + Assert.That( + AutomationWindowPlacement.GetChoice(), + Is.EqualTo(Choice.AsBloomNormallyWould) + ); + Assert.That(AutomationWindowPlacement.IsOffEveryMonitor, Is.False); + } + + [Test] + public void GetChoice_WithTheAutomationFlag_ObeysTheVariable() + { + Environment.SetEnvironmentVariable( + AutomationWindowPlacement.VariableName, + AutomationWindowPlacement.HeadlessSetting + ); + + Program.ParseStartupPortArguments(new[] { "--automation" }, out var errorMessage); + Assert.That(errorMessage, Is.Null); + Assert.That(Program.StartupAutomation, Is.True, "Sanity check."); + + Assert.That(AutomationWindowPlacement.GetChoice(), Is.EqualTo(Choice.OffEveryMonitor)); + Assert.That(AutomationWindowPlacement.IsOffEveryMonitor, Is.True); + } + + /// + /// The log line is the only place a developer can see which monitor a number means, because + /// the number is not the one Windows Settings shows. So it has to name every monitor, and it + /// has to say that the numbering differs. + /// + [Test] + public void DescribeChoice_NamesEveryMonitorAndWarnsAboutTheNumbering() + { + Environment.SetEnvironmentVariable( + AutomationWindowPlacement.VariableName, + AutomationWindowPlacement.HeadlessSetting + ); + Program.ParseStartupPortArguments(new[] { "--automation" }, out _); + + var line = AutomationWindowPlacement.DescribeChoice(); + + Assert.That(line, Does.Contain(AutomationWindowPlacement.VariableName)); + Assert.That(line, Does.Contain("off every monitor")); + Assert.That( + line, + Does.Contain("NOT how Windows Settings"), + "A developer reading a number off Windows Settings has to be warned." + ); + var screens = AutomationWindowPlacement.MonitorsLeftToRight(); + for (var oneBased = 1; oneBased <= screens.Length; oneBased++) + { + Assert.That( + line, + Does.Contain($"{oneBased}=({screens[oneBased - 1].Bounds.X},"), + $"Monitor {oneBased} is missing from the log line." + ); + } + } + + /// + /// Monitor 1 is the leftmost monitor, whatever order Screen.AllScreens returns. This runs + /// against however many monitors the machine has, so it says nothing on the CI runner's + /// single screen and everything on a developer's several. + /// + [Test] + public void MonitorsLeftToRight_AreOrderedByPosition() + { + var monitors = AutomationWindowPlacement.MonitorsLeftToRight(); + + Assert.That( + monitors.Length, + Is.EqualTo(System.Windows.Forms.Screen.AllScreens.Length), + "Every monitor has to be there; the order is all that changes." + ); + for (var next = 1; next < monitors.Length; next++) + { + Assert.That( + monitors[next].Bounds.Left, + Is.GreaterThanOrEqualTo(monitors[next - 1].Bounds.Left), + $"Monitor {next + 1} is left of monitor {next}." + ); + } + } + + /// + /// The window has to end up wholly off every monitor, and at a coordinate Windows will + /// honour. This runs against however many monitors the machine has, which is the point: + /// the answer has to hold on the CI runner's one screen and on a developer's several. + /// + [Test] + public void GetBoundsOffEveryMonitor_IsWhollyOffEveryMonitorAndWithinWindowsLimit() + { + var bounds = AutomationWindowPlacement.GetBoundsOffEveryMonitor(); + + Assert.That(bounds.Width, Is.GreaterThan(0), "Sanity check: a real size."); + Assert.That(bounds.Height, Is.GreaterThan(0), "Sanity check: a real size."); + Assert.That( + bounds.Top, + Is.LessThanOrEqualTo(32000), + "Windows does not honour a position further down than this." + ); + Assert.That( + bounds.Left, + Is.EqualTo(System.Windows.Forms.Screen.PrimaryScreen.WorkingArea.X), + "The window has to line up with the primary monitor, or Windows gives it another " + + "monitor's scale factor. See GetBoundsOffEveryMonitor." + ); + foreach (var screen in System.Windows.Forms.Screen.AllScreens) + { + Assert.That( + bounds.IntersectsWith(screen.Bounds), + Is.False, + $"The window overlaps the monitor at {screen.Bounds}." + ); + } + + // The desktop can make the window as much as four times as high as this process asked + // for, because Windows scales a monitor by at most 400%. So a clearance of one height + // is not enough: measure it, and require four. See GetBoundsOffEveryMonitor. + var lowestY = System.Windows.Forms.Screen.AllScreens.Max(screen => + screen.Bounds.Bottom + ); + Assert.That( + bounds.Top - lowestY, + Is.GreaterThanOrEqualTo(bounds.Height * 4), + "A window four times this high would still have to clear the lowest monitor." + ); + } + } +}