diff --git a/packages/tool-server/src/tools/describe/platforms/android/index.ts b/packages/tool-server/src/tools/describe/platforms/android/index.ts index 03ef11823..1fae1781b 100644 --- a/packages/tool-server/src/tools/describe/platforms/android/index.ts +++ b/packages/tool-server/src/tools/describe/platforms/android/index.ts @@ -3,7 +3,11 @@ import type { Registry, ToolDependency } from "@argent/registry"; import type { DescribeTreeData } from "../../contract"; import { adbExecOutBinary, isAndroidTv } from "../../../../utils/adb"; import { resolveDevice } from "../../../../utils/device-info"; -import { getAndroidScreenSize } from "../../../../utils/android-screen"; +import { + getAndroidScreenSize, + orientScreenSize, + parseDumpRotation, +} from "../../../../utils/android-screen"; import { parseUiAutomatorDump } from "./uiautomator-parser"; import { androidDevtoolsRef, @@ -113,6 +117,12 @@ export async function describeAndroid( } ); } - const tree = parseUiAutomatorDump(raw, size.width, size.height); + // `wm size` is not rotation-aware, but the dump says which rotation it was + // taken at. Orienting the divisor here is what keeps a rotated device's frames + // in the same upright space the android-devtools path already produces — and + // stops the right-hand half of a landscape screen being pruned away as + // off-screen (#609). + const oriented = orientScreenSize(size, parseDumpRotation(raw)); + const tree = parseUiAutomatorDump(raw, oriented.width, oriented.height); return { tree, source: "uiautomator", hint }; } diff --git a/packages/tool-server/src/tools/screenshot-diff/index.ts b/packages/tool-server/src/tools/screenshot-diff/index.ts index eada18494..219985cee 100644 --- a/packages/tool-server/src/tools/screenshot-diff/index.ts +++ b/packages/tool-server/src/tools/screenshot-diff/index.ts @@ -5,6 +5,7 @@ import path from "path"; import { z } from "zod"; import { FAILURE_CODES, FailureError } from "@argent/registry"; import type { + DeviceInfo, FileInputSpec, ServiceRef, ToolContext, @@ -14,6 +15,7 @@ import type { import { simulatorServerRef, type SimulatorServerApi } from "../../blueprints/simulator-server"; import { resolveDevice } from "../../utils/device-info"; import { httpScreenshot } from "../../utils/simulator-client"; +import { captureScreenshotUpright } from "../../utils/rotation-aware-capture"; import { requireArtifacts, type ArtifactHandle } from "../../artifacts"; import { diffPngFiles } from "./screenshot-diff"; @@ -48,7 +50,11 @@ const zodSchema = z rotation: z .enum(["Portrait", "LandscapeLeft", "LandscapeRight", "PortraitUpsideDown"]) .optional() - .describe("Orientation override for live baseline/current captures."), + .describe( + "Orientation override for live baseline/current captures. Rarely needed: an Android capture " + + "already follows the device's rotation. Setting it pins a fixed rotation, which can make a " + + "live capture disagree with a saved baseline taken at a different device rotation." + ), outputDir: z .string() .min(1) @@ -197,6 +203,7 @@ async function resolveInputPaths( const baselinePath = params.captureBaseline ? await captureLiveInput({ api: requireSimulatorServer(services), + device: resolveDevice(params.udid), outputDir, name: "baseline", rotation: params.rotation, @@ -208,6 +215,7 @@ async function resolveInputPaths( const currentPath = params.captureCurrent ? await captureLiveInput({ api: requireSimulatorServer(services), + device: resolveDevice(params.udid), outputDir, name: "current", rotation: params.rotation, @@ -279,6 +287,10 @@ async function captureLiveInput(params: { // Resolved and validated by requireSimulatorServer at the call site, so it is // never undefined here. api: SimulatorServerApi; + // Needed so a live capture picks up the device's rotation the same way the + // `screenshot` tool does. Without it a rotated-Android `captureCurrent` would + // come back sideways and diff at ~100% against an upright saved baseline. + device: DeviceInfo; outputDir: string; name: "baseline" | "current"; rotation?: Params["rotation"]; @@ -294,9 +306,23 @@ async function captureLiveInput(params: { // baseline saved at any scale. Full-res is preserved wherever it works (iOS). let capture: Awaited>; try { - capture = await params.captureScreenshot(params.api, params.rotation, params.signal, 1.0); + capture = await captureScreenshotUpright( + params.api, + params.device, + params.rotation, + params.signal, + 1.0, + params.captureScreenshot + ); } catch { - capture = await params.captureScreenshot(params.api, params.rotation, params.signal); + capture = await captureScreenshotUpright( + params.api, + params.device, + params.rotation, + params.signal, + undefined, + params.captureScreenshot + ); } const suffix = crypto.randomBytes(4).toString("hex"); const destination = path.join(params.outputDir, `${params.name}-${suffix}.live.png`); diff --git a/packages/tool-server/src/tools/screenshot/index.ts b/packages/tool-server/src/tools/screenshot/index.ts index 7ee753085..261430380 100644 --- a/packages/tool-server/src/tools/screenshot/index.ts +++ b/packages/tool-server/src/tools/screenshot/index.ts @@ -7,7 +7,8 @@ import type { Registry, ToolCapability, ToolDefinition } from "@argent/registry" import { simulatorServerRef, type SimulatorServerApi } from "../../blueprints/simulator-server"; import { chromiumCdpRef, type ChromiumCdpApi } from "../../blueprints/chromium-cdp"; import { resolveDevice } from "../../utils/device-info"; -import { getScreenshotScale, httpScreenshot } from "../../utils/simulator-client"; +import { getScreenshotScale } from "../../utils/simulator-client"; +import { captureScreenshotUpright } from "../../utils/rotation-aware-capture"; import { isTvOsSimulator } from "../../utils/ios-devices"; import { simctlArgsForUdid } from "../../utils/ios-device-sets"; import { captureVegaScreenshotPng } from "../../utils/vega-screen"; @@ -25,7 +26,11 @@ const zodSchema = z.object({ .enum(["Portrait", "LandscapeLeft", "LandscapeRight", "PortraitUpsideDown"]) .optional() .describe( - "Orientation override for the screenshot (rotates the captured image after Page.captureScreenshot on Chromium)." + "Orientation override. Rarely needed: on Android the capture already follows the device's " + + "current rotation, so it is upright without this. Setting it replaces that with a fixed " + + "rotation, which on a rotated device produces an image whose geometry no longer matches " + + "`describe` frames or gesture coordinates. On Chromium it rotates the captured image after " + + "Page.captureScreenshot." ), scale: z .number() @@ -130,6 +135,7 @@ export function createScreenshotTool(registry: Registry): ToolDefinition1 / width>1 because the screenW used for the - * divisor was pre-rotation. One extra `adb shell` per `describe` is cheap - * compared to the uiautomator dump exec-out it sits next to. + * describes after a rotation (rotation completes in <500 ms). One extra + * `adb shell` per `describe` is cheap compared to the uiautomator dump exec-out + * it sits next to. */ export async function getAndroidScreenSize(serial: string): Promise { const out = await adbShell(serial, "wm size", { timeoutMs: 5_000 }); @@ -45,3 +52,40 @@ export async function getAndroidScreenSize(serial: string): Promise`. + * + * Taking it from the XML rather than asking the device again is deliberate. It + * costs no extra round-trip, it cannot disagree with the bounds it is used to + * normalize (a rotation between the two calls would), and — most importantly — + * the legacy path runs precisely when the android-devtools helper could not be + * reached, which is when the device is least likely to answer more adb probes. + * + * Returns null when absent, which keeps pre-rotation-attribute dumps working. + */ +export function parseDumpRotation(rawOutput: string): 0 | 1 | 2 | 3 | null { + const value = Number(/]*\brotation="([0-3])"/.exec(rawOutput)?.[1]); + if (value === 0 || value === 1 || value === 2 || value === 3) return value; + return null; +} + +/** + * Swap width and height when the device is on its side, so the result describes + * the display as it is currently laid out. + * + * uiautomator reports node bounds against the rotated display, so on a rotated + * device the unrotated `wm size` is the wrong divisor in both axes. The visible + * consequence was not merely squashed frames: `isVisibleRect` drops any node + * whose left edge is past the screen width, so with a 1080-wide divisor against + * a 2424-wide display, everything on the right-hand half of the screen + * disappeared from the tree entirely. + */ +export function orientScreenSize( + size: AndroidScreenSize, + rotation: 0 | 1 | 2 | 3 | null +): AndroidScreenSize { + if (rotation !== 1 && rotation !== 3) return size; + return { width: size.height, height: size.width }; +} diff --git a/packages/tool-server/src/utils/device-orientation.ts b/packages/tool-server/src/utils/device-orientation.ts new file mode 100644 index 000000000..65c7a6786 --- /dev/null +++ b/packages/tool-server/src/utils/device-orientation.ts @@ -0,0 +1,147 @@ +import { promises as fs } from "node:fs"; +import { adbShell } from "./adb"; + +/** + * The orientation names the simulator-server screenshot API accepts. + * + * Read these as *compositing transforms*, not as physical orientations. The + * distinction matters: a report on #609 established that on iOS the same names + * are 180° inverted from the physical result, so nothing here may be derived + * from what the words mean. + */ +export type OrientationName = + | "Portrait" + | "LandscapeLeft" + | "LandscapeRight" + | "PortraitUpsideDown"; + +/** Android's surface rotation, as reported by the platform (`Surface.ROTATION_*`). */ +export type SurfaceRotation = 0 | 1 | 2 | 3; + +/** + * Surface rotation → the name that makes simulator-server hand back an upright + * capture. + * + * DERIVED BY MEASUREMENT, NOT BY NAME. `adb exec-out screencap` is + * rotation-aware, so it is ground truth; each candidate name was captured at + * full scale and scored against it as mean absolute difference over a 96×54 + * grayscale grid, both as-is and rotated 180° to catch an inverted mapping: + * + * rotation 1 LandscapeLeft MAD 2.19 (vs 6.54 for its 180° twin) + * rotation 3 LandscapeRight MAD 2.19 (vs 6.34) + * rotation 2 PortraitUpsideDown MAD 2.67 (vs 13.13) + * rotation 0 Portrait identity — an unrotated capture is already upright + * + * Note this table is the INVERSE of simulator-server's own convention, which + * maps 90° → LandscapeRight (`src/device_controller.rs`) and rotates the frame + * at decode time from the scrcpy header's `display_orientation` + * (`src/device_controller/android_device/video.rs`, + * `src/media_handler/decoder/video_toolbox.rs`). We are compensating downstream + * for what that layer already did, so this constant encodes the behaviour of a + * particular simulator-server build rather than a property of Android. + * + * That is why a unit test pinning this table is not sufficient on its own: it + * could never notice simulator-server changing underneath us. `captureLooksUpright` + * below is the check that would, and it is applied to the real capture. + */ +export const SURFACE_ROTATION_TO_NAME: Readonly> = { + 0: "Portrait", + 1: "LandscapeLeft", + 2: "PortraitUpsideDown", + 3: "LandscapeRight", +}; + +function isSurfaceRotation(value: number): value is SurfaceRotation { + return value === 0 || value === 1 || value === 2 || value === 3; +} + +/** + * Read the device's current surface rotation. + * + * Returns `null` — never throws — when the rotation cannot be established. A + * failed query must degrade to "unknown", which callers treat as "behave + * exactly as before". It must never fall back to a guess: a wrong orientation + * produces a confidently wrong image, which is worse than the sideways one this + * is fixing. + * + * Two independent readings are tried because `dumpsys` output is not a stable + * API. `|| true` keeps a `grep` miss (exit 1) from making `adbShell` throw. + */ +export async function readAndroidSurfaceRotation(serial: string): Promise { + const probes: { cmd: string; pattern: RegExp }[] = [ + { + cmd: "dumpsys display | grep mCurrentOrientation || true", + pattern: /mCurrentOrientation=([0-3])/, + }, + { cmd: "dumpsys window displays || true", pattern: /\bmRotation=([0-3])/ }, + ]; + + for (const { cmd, pattern } of probes) { + try { + const out = await adbShell(serial, cmd, { timeoutMs: 5_000 }); + const value = Number(pattern.exec(out)?.[1]); + if (Number.isInteger(value) && isSurfaceRotation(value)) return value; + } catch { + // Try the next probe; exhausting them yields null. + } + } + return null; +} + +/** + * The rotation to request for an upright capture, or `undefined` to send no + * rotation at all. + * + * `undefined` is returned for an unrotated device rather than `"Portrait"` so + * the request body is byte-identical to what it was before this existed — the + * overwhelmingly common case stays provably unchanged. + */ +export function captureRotationForSurface( + rotation: SurfaceRotation | null +): OrientationName | undefined { + if (rotation === null || rotation === 0) return undefined; + return SURFACE_ROTATION_TO_NAME[rotation]; +} + +/** Width and height from a PNG's IHDR chunk, or null if this isn't a PNG. */ +export async function readPngSize(path: string): Promise<{ width: number; height: number } | null> { + let handle: Awaited> | undefined; + try { + handle = await fs.open(path, "r"); + const buf = Buffer.alloc(24); + const { bytesRead } = await handle.read(buf, 0, 24, 0); + if (bytesRead < 24) return null; + // 8-byte signature, then a length+type header, then IHDR's width/height. + if (buf.readUInt32BE(0) !== 0x89504e47) return null; + const width = buf.readUInt32BE(16); + const height = buf.readUInt32BE(20); + if (width <= 0 || height <= 0) return null; + return { width, height }; + } catch { + return null; + } finally { + await handle?.close().catch(() => {}); + } +} + +/** + * Does a capture taken with `requested` actually have the shape that rotation + * implies? + * + * This is the guard that a table test cannot be. The mapping above compensates + * for a rotation simulator-server applies at decode time; if that layer changes, + * the compensation silently becomes a 180° error or a no-op. Comparing the + * delivered PNG's aspect against the orientation we asked for catches the case + * where the request did not do what this module assumes. + * + * Returns true when unknown — an unreadable PNG is not evidence of a problem. + */ +export function captureLooksUpright( + requested: OrientationName, + size: { width: number; height: number } | null +): boolean { + if (!size || size.width === size.height) return true; + const wantsLandscape = requested === "LandscapeLeft" || requested === "LandscapeRight"; + const isLandscape = size.width > size.height; + return wantsLandscape === isLandscape; +} diff --git a/packages/tool-server/src/utils/rotation-aware-capture.ts b/packages/tool-server/src/utils/rotation-aware-capture.ts new file mode 100644 index 000000000..d0c2f408b --- /dev/null +++ b/packages/tool-server/src/utils/rotation-aware-capture.ts @@ -0,0 +1,76 @@ +import type { SimulatorServerApi } from "../blueprints/simulator-server"; +import type { DeviceInfo } from "@argent/registry"; +import { + captureLooksUpright, + captureRotationForSurface, + readAndroidSurfaceRotation, + readPngSize, +} from "./device-orientation"; +import { httpScreenshot } from "./simulator-client"; + +/** + * Capture a screenshot that is the right way up on a rotated device. + * + * Background (#609): on a rotated Android device the capture came back + * portrait-framed with the content lying sideways, while `describe` and the + * gesture tools were already reporting and accepting *upright* coordinates — + * uiautomator measures against the rotated display. So the screenshot was the + * one surface out of step, and an agent reading it saw an image whose geometry + * disagreed with every coordinate it was given. + * + * simulator-server already knows how to hand back an upright frame; it just has + * to be told which rotation to apply. Nothing tracks orientation here — the + * rotation is queried from the device each time. A cached value could only ever + * be staler than a ~40 ms probe, and serving a stale orientation is precisely + * the failure this fixes. + * + * Why query over adb rather than reuse the android-devtools helper, which also + * reports rotation: resolving that service *installs and starts* it (APK + * install, `am instrument`, a 30 s ready timeout). `screenshot` is `alwaysLoad` + * and fires automatically after more than a dozen tools, so making it able to + * install an APK is not acceptable. Gating on "use it only if already running" + * would be worse still — the capture would come out upright or sideways + * depending on whether something happened to call `describe` first. + * + * iOS is deliberately untouched. There the whole surface (describe frames, + * gesture input, capture) is consistently in the portrait-native space, so + * rotating only the capture would break the agreement rather than restore it. + */ +export async function captureScreenshotUpright( + api: SimulatorServerApi, + device: DeviceInfo, + requestedRotation: string | undefined, + signal?: AbortSignal, + scale?: number, + capture: typeof httpScreenshot = httpScreenshot +): Promise<{ url: string; path: string }> { + // An explicit rotation from the caller always wins, and no other platform + // takes this path, so both cases are byte-identical to the previous behaviour. + if (requestedRotation !== undefined || device.platform !== "android") { + return capture(api, requestedRotation, signal, scale); + } + + const surface = await readAndroidSurfaceRotation(device.id); + const rotation = captureRotationForSurface(surface); + // Unrotated, or the rotation could not be read: send no rotation at all, exactly + // as before. An unreadable rotation must never become a guess. + if (!rotation) return capture(api, undefined, signal, scale); + + const result = await capture(api, rotation, signal, scale); + + // The mapping from surface rotation to rotation name compensates for a + // rotation simulator-server itself applies while decoding the video stream. If + // that layer changes, the compensation quietly becomes a 180° error or a + // no-op, and no test of our own constant could notice. Checking the delivered + // image's aspect against the rotation we asked for is the check that would. + if (!captureLooksUpright(rotation, await readPngSize(result.path))) { + console.warn( + `[screenshot] ${device.id}: requested ${rotation} for surface rotation ${surface}, ` + + `but the capture came back with the opposite aspect. Falling back to an ` + + `unrotated capture — simulator-server's rotation handling may have changed.` + ); + return capture(api, undefined, signal, scale); + } + + return result; +} diff --git a/packages/tool-server/test/android-rotated-uiautomator.test.ts b/packages/tool-server/test/android-rotated-uiautomator.test.ts new file mode 100644 index 000000000..e16006deb --- /dev/null +++ b/packages/tool-server/test/android-rotated-uiautomator.test.ts @@ -0,0 +1,122 @@ +import { describe, it, expect } from "vitest"; +import { orientScreenSize, parseDumpRotation } from "../src/utils/android-screen"; +import { parseUiAutomatorDump } from "../src/tools/describe/platforms/android/uiautomator-parser"; + +/** + * Issue #609, legacy describe path. `wm size` reports the UNROTATED size — + * measured on a landscape Pixel_9 (API 36) whose display was really 2424x1080, + * `wm size` still answered "Physical size: 1080x2424" with no Override line. + * + * uiautomator, by contrast, reports bounds against the rotated display. Dividing + * one by the other did not merely squash the frames: nodes whose left edge lies + * past the (too small) screen width are treated as off-screen and pruned, so the + * right-hand half of a landscape screen vanished from the tree entirely. + * + * The dump states its own rotation, so the divisor can be oriented with no extra + * round-trip — which matters because this path runs exactly when the + * android-devtools helper could not be reached. + */ + +/** Shaped like a real landscape dump: 2424x1080, with content past x=1080. */ +const LANDSCAPE_DUMP = ` + + + + + +`; + +const PORTRAIT_DUMP = LANDSCAPE_DUMP.replace('rotation="1"', 'rotation="0"'); + +/** Every label anywhere in the tree. uiautomator `text` surfaces as `label`. */ +function labels(node: unknown): string[] { + const out: string[] = []; + const walk = (n: Record) => { + if (typeof n.label === "string" && n.label) out.push(n.label); + for (const c of (n.children as Record[] | undefined) ?? []) walk(c); + }; + walk(node as Record); + return out; +} + +function findFrame(node: unknown, text: string): { x: number; width: number } | undefined { + let found: { x: number; width: number } | undefined; + const walk = (n: Record) => { + if (n.label === text && n.frame) found = n.frame as { x: number; width: number }; + for (const c of (n.children as Record[] | undefined) ?? []) walk(c); + }; + walk(node as Record); + return found; +} + +describe("parseDumpRotation", () => { + it("reads the rotation the dump was taken at", () => { + expect(parseDumpRotation(LANDSCAPE_DUMP)).toBe(1); + expect(parseDumpRotation(PORTRAIT_DUMP)).toBe(0); + }); + + it("reads every rotation the platform can report", () => { + for (const r of [0, 1, 2, 3]) { + expect(parseDumpRotation(``)).toBe(r); + } + }); + + it("returns null when the attribute is absent, so older dumps still work", () => { + expect(parseDumpRotation("")).toBeNull(); + }); + + it("returns null for an unparseable value rather than guessing", () => { + expect(parseDumpRotation('')).toBeNull(); + }); +}); + +describe("orientScreenSize", () => { + const portrait = { width: 1080, height: 2424 }; + + it("swaps the axes when the device is on its side", () => { + expect(orientScreenSize(portrait, 1)).toEqual({ width: 2424, height: 1080 }); + expect(orientScreenSize(portrait, 3)).toEqual({ width: 2424, height: 1080 }); + }); + + it("leaves an upright or upside-down device alone", () => { + expect(orientScreenSize(portrait, 0)).toEqual(portrait); + expect(orientScreenSize(portrait, 2)).toEqual(portrait); + }); + + it("leaves the size alone when the rotation is unknown", () => { + expect(orientScreenSize(portrait, null)).toEqual(portrait); + }); +}); + +describe("a rotated dump normalized against the oriented size", () => { + const wmSize = { width: 1080, height: 2424 }; // what `wm size` really answers + + it("drops the right-hand half of the screen when the size is not oriented", () => { + // This is the bug, stated as a test: with the unrotated divisor the node at + // x=1900 is past screenW=1080 and is pruned as off-screen, so it is missing + // from the tree entirely — not merely clamped. + const tree = parseUiAutomatorDump(LANDSCAPE_DUMP, wmSize.width, wmSize.height); + expect(labels(tree)).toContain("Left edge"); + expect(labels(tree)).not.toContain("Right edge"); + }); + + it("keeps both edges once the size is oriented by the dump's own rotation", () => { + const oriented = orientScreenSize(wmSize, parseDumpRotation(LANDSCAPE_DUMP)); + const tree = parseUiAutomatorDump(LANDSCAPE_DUMP, oriented.width, oriented.height); + expect(labels(tree)).toEqual(expect.arrayContaining(["Left edge", "Right edge"])); + }); + + it("puts the right-hand node where it actually is on screen", () => { + const oriented = orientScreenSize(wmSize, parseDumpRotation(LANDSCAPE_DUMP)); + const tree = parseUiAutomatorDump(LANDSCAPE_DUMP, oriented.width, oriented.height); + const frame = findFrame(tree, "Right edge")!; + // 1900/2424 ≈ 0.784 — on the right-hand side, which is the whole point. + expect(frame.x).toBeCloseTo(1900 / 2424, 2); + expect(frame.width).toBeCloseTo(480 / 2424, 2); + }); + + it("leaves an unrotated dump exactly as it was", () => { + const oriented = orientScreenSize(wmSize, parseDumpRotation(PORTRAIT_DUMP)); + expect(oriented).toEqual(wmSize); + }); +}); diff --git a/packages/tool-server/test/device-orientation.test.ts b/packages/tool-server/test/device-orientation.test.ts new file mode 100644 index 000000000..c4d6375b5 --- /dev/null +++ b/packages/tool-server/test/device-orientation.test.ts @@ -0,0 +1,194 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { promises as fs } from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; + +/** + * Issue #609: on a rotated Android device the screenshot came back + * portrait-framed with the content lying sideways, while `describe` frames and + * gesture coordinates were already upright. The capture was the one surface out + * of step. + * + * The rotation is queried per capture rather than tracked, so these tests are + * about reading it honestly and — crucially — about refusing to guess when it + * cannot be read. A wrong orientation yields a confidently wrong image, which is + * worse than the sideways one being fixed. + */ + +const adbShell = vi.hoisted(() => vi.fn(async (_serial: string, _cmd: string) => "")); +vi.mock("../src/utils/adb", () => ({ adbShell })); + +import { + SURFACE_ROTATION_TO_NAME, + captureLooksUpright, + captureRotationForSurface, + readAndroidSurfaceRotation, + readPngSize, +} from "../src/utils/device-orientation"; + +/** Verbatim shape of the two probes, captured from a Pixel_9 (API 36). */ +const DUMPSYS_DISPLAY = (r: number) => ` mCurrentOrientation=${r}\n`; +const DUMPSYS_WINDOW = (r: number) => + `Display: mDisplayId=0\n init=1080x2424 420dpi\n mRotation=${r}\n mCurrentRotation=ROTATION_90\n`; + +beforeEach(() => { + vi.clearAllMocks(); + adbShell.mockResolvedValue(""); +}); + +describe("the surface-rotation → rotation-name table", () => { + // Pinned by measurement against `adb exec-out screencap`, which IS + // rotation-aware: each candidate name was scored as mean absolute difference + // against the screencap reference and against that reference rotated 180°, so + // an inverted mapping could not pass. The correct name won by ~3x in every row + // (e.g. rotation 1: LandscapeLeft 2.19 vs LandscapeRight 6.54). + // + // This table is the INVERSE of simulator-server's own 90°→LandscapeRight + // convention because we are compensating for a rotation it applies while + // decoding. That is exactly why this test cannot be the only guard — see + // `captureLooksUpright`, which checks the real capture instead. + it("maps each rotation to the name that yields an upright capture", () => { + expect(SURFACE_ROTATION_TO_NAME).toEqual({ + 0: "Portrait", + 1: "LandscapeLeft", + 2: "PortraitUpsideDown", + 3: "LandscapeRight", + }); + }); + + it("sends no rotation at all for an unrotated device", () => { + // Not "Portrait": keeping it undefined leaves the request body byte-identical + // to what it was before any of this existed, so the common case is provably + // unchanged. + expect(captureRotationForSurface(0)).toBeUndefined(); + }); + + it("sends no rotation when the rotation could not be read", () => { + expect(captureRotationForSurface(null)).toBeUndefined(); + }); + + it("requests a rotation for each side the device can be on", () => { + expect(captureRotationForSurface(1)).toBe("LandscapeLeft"); + expect(captureRotationForSurface(2)).toBe("PortraitUpsideDown"); + expect(captureRotationForSurface(3)).toBe("LandscapeRight"); + }); +}); + +describe("reading the rotation off the device", () => { + it("reads the primary probe", async () => { + adbShell.mockResolvedValueOnce(DUMPSYS_DISPLAY(1)); + expect(await readAndroidSurfaceRotation("emulator-5554")).toBe(1); + expect(adbShell).toHaveBeenCalledTimes(1); + }); + + it("reads every rotation the platform can report", async () => { + for (const r of [0, 1, 2, 3]) { + adbShell.mockResolvedValueOnce(DUMPSYS_DISPLAY(r)); + expect(await readAndroidSurfaceRotation("emulator-5554")).toBe(r); + } + }); + + it("falls back to the second probe when the first says nothing", async () => { + // `dumpsys` output is not a stable API, so a single regex is a single point + // of failure across vendors and versions. + adbShell.mockResolvedValueOnce("").mockResolvedValueOnce(DUMPSYS_WINDOW(3)); + expect(await readAndroidSurfaceRotation("emulator-5554")).toBe(3); + expect(adbShell).toHaveBeenCalledTimes(2); + }); + + it("keeps a grep miss from throwing", async () => { + // The probe appends `|| true` precisely so that grep exiting 1 — the normal + // outcome when the line is absent — is not an error. + expect(adbShell.mock.calls).toHaveLength(0); + await readAndroidSurfaceRotation("emulator-5554"); + for (const [, cmd] of adbShell.mock.calls) expect(cmd).toContain("|| true"); + }); + + it("returns null rather than guessing when both probes fail", async () => { + adbShell.mockRejectedValue(new Error("device offline")); + expect(await readAndroidSurfaceRotation("emulator-5554")).toBeNull(); + }); + + it("returns null on unparseable output", async () => { + adbShell.mockResolvedValue("mCurrentOrientation=banana"); + expect(await readAndroidSurfaceRotation("emulator-5554")).toBeNull(); + }); + + it("ignores an out-of-range rotation", async () => { + // Never coerce something unrecognised into a rotation — a wrong one is worse + // than none. + adbShell.mockResolvedValue("mCurrentOrientation=7"); + expect(await readAndroidSurfaceRotation("emulator-5554")).toBeNull(); + }); +}); + +describe("the aspect guard on the delivered capture", () => { + // The mapping compensates for what simulator-server does at decode time. If + // that changes, the compensation silently becomes a 180° error or a no-op and + // no test of our own constant could see it. This checks the actual image. + it("accepts a landscape image for a landscape rotation", () => { + expect(captureLooksUpright("LandscapeLeft", { width: 2424, height: 1080 })).toBe(true); + expect(captureLooksUpright("LandscapeRight", { width: 2424, height: 1080 })).toBe(true); + }); + + it("rejects a portrait image delivered for a landscape rotation", () => { + expect(captureLooksUpright("LandscapeLeft", { width: 1080, height: 2424 })).toBe(false); + }); + + it("accepts a portrait image for an upside-down portrait rotation", () => { + expect(captureLooksUpright("PortraitUpsideDown", { width: 1080, height: 2424 })).toBe(true); + }); + + it("does not treat an unreadable image as evidence of a problem", () => { + expect(captureLooksUpright("LandscapeLeft", null)).toBe(true); + }); + + it("passes a square image, which carries no aspect information", () => { + expect(captureLooksUpright("LandscapeLeft", { width: 512, height: 512 })).toBe(true); + }); +}); + +describe("readPngSize", () => { + let dir: string; + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), "argent-png-size-")); + }); + afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }); + }); + + /** A minimal valid PNG header — signature, then IHDR length/type/w/h. */ + function pngHeader(width: number, height: number): Buffer { + const buf = Buffer.alloc(24); + buf.writeUInt32BE(0x89504e47, 0); + buf.writeUInt32BE(0x0d0a1a0a, 4); + buf.writeUInt32BE(13, 8); + buf.write("IHDR", 12, "ascii"); + buf.writeUInt32BE(width, 16); + buf.writeUInt32BE(height, 20); + return buf; + } + + it("reads the dimensions out of the IHDR chunk", async () => { + const file = path.join(dir, "a.png"); + await fs.writeFile(file, pngHeader(2424, 1080)); + expect(await readPngSize(file)).toEqual({ width: 2424, height: 1080 }); + }); + + it("returns null for a file that is not a PNG", async () => { + const file = path.join(dir, "b.png"); + await fs.writeFile(file, Buffer.alloc(24)); + expect(await readPngSize(file)).toBeNull(); + }); + + it("returns null for a truncated file rather than throwing", async () => { + const file = path.join(dir, "c.png"); + await fs.writeFile(file, Buffer.alloc(8)); + expect(await readPngSize(file)).toBeNull(); + }); + + it("returns null for a missing file rather than throwing", async () => { + expect(await readPngSize(path.join(dir, "nope.png"))).toBeNull(); + }); +}); diff --git a/packages/tool-server/test/rotation-aware-capture.test.ts b/packages/tool-server/test/rotation-aware-capture.test.ts new file mode 100644 index 000000000..f9d7281bb --- /dev/null +++ b/packages/tool-server/test/rotation-aware-capture.test.ts @@ -0,0 +1,142 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import type { DeviceInfo } from "@argent/registry"; + +/** + * Issue #609: the capture is the surface that has to move, not the coordinates. + * On Android `describe` frames and gesture input are already upright, so the + * capture is made to follow the device rotation to match them. iOS is left + * alone: there the whole surface is consistently portrait-native, and rotating + * only the capture would break an agreement rather than restore one. + */ + +const readAndroidSurfaceRotation = vi.hoisted(() => + vi.fn(async (_serial: string): Promise<0 | 1 | 2 | 3 | null> => null) +); +const readPngSize = vi.hoisted(() => + vi.fn(async (_p: string): Promise<{ width: number; height: number } | null> => null) +); + +vi.mock("../src/utils/device-orientation", async (importOriginal) => { + // The mapping table and the aspect guard stay REAL so the assertions below + // exercise the actual constant rather than a restatement of it. + const actual = await importOriginal(); + return { ...actual, readAndroidSurfaceRotation, readPngSize }; +}); + +import { captureScreenshotUpright } from "../src/utils/rotation-aware-capture"; + +const ANDROID: DeviceInfo = { id: "emulator-5554", platform: "android", kind: "emulator" }; +const IOS: DeviceInfo = { id: "BC0026C7-AAE0-490E", platform: "ios", kind: "simulator" }; + +const api = {} as never; + +/** Records the rotation each capture was asked for. */ +function recorder(size: { width: number; height: number } = { width: 2424, height: 1080 }) { + const rotations: (string | undefined)[] = []; + const capture = vi.fn(async (_api: unknown, rotation?: string) => { + rotations.push(rotation); + return { url: "http://x/y.png", path: "/tmp/y.png" }; + }); + readPngSize.mockResolvedValue(size); + return { rotations, capture: capture as never }; +} + +beforeEach(() => { + vi.clearAllMocks(); + readAndroidSurfaceRotation.mockResolvedValue(null); + readPngSize.mockResolvedValue(null); +}); + +describe("Android capture follows the device rotation", () => { + it("requests the matching rotation for a landscape device", async () => { + readAndroidSurfaceRotation.mockResolvedValue(1); + const { rotations, capture } = recorder(); + + await captureScreenshotUpright(api, ANDROID, undefined, undefined, undefined, capture); + + expect(rotations).toEqual(["LandscapeLeft"]); + }); + + it("sends no rotation for an unrotated device", async () => { + // The overwhelmingly common case must produce the exact request it always + // did — not `rotation: "Portrait"`. + readAndroidSurfaceRotation.mockResolvedValue(0); + const { rotations, capture } = recorder({ width: 1080, height: 2424 }); + + await captureScreenshotUpright(api, ANDROID, undefined, undefined, undefined, capture); + + expect(rotations).toEqual([undefined]); + }); + + it("sends no rotation when the device would not say", async () => { + readAndroidSurfaceRotation.mockResolvedValue(null); + const { rotations, capture } = recorder({ width: 1080, height: 2424 }); + + await captureScreenshotUpright(api, ANDROID, undefined, undefined, undefined, capture); + + expect(rotations).toEqual([undefined]); + }); + + it("lets an explicit rotation win, and does not probe at all", async () => { + const { rotations, capture } = recorder(); + + await captureScreenshotUpright( + api, + ANDROID, + "PortraitUpsideDown", + undefined, + undefined, + capture + ); + + expect(rotations).toEqual(["PortraitUpsideDown"]); + expect(readAndroidSurfaceRotation).not.toHaveBeenCalled(); + }); + + it("passes the scale through unchanged", async () => { + readAndroidSurfaceRotation.mockResolvedValue(1); + const { capture } = recorder(); + + await captureScreenshotUpright(api, ANDROID, undefined, undefined, 1.0, capture); + + expect(capture).toHaveBeenCalledWith(api, "LandscapeLeft", undefined, 1.0); + }); +}); + +describe("iOS is deliberately untouched", () => { + it("never probes and never adds a rotation", async () => { + const { rotations, capture } = recorder(); + + await captureScreenshotUpright(api, IOS, undefined, undefined, undefined, capture); + + expect(rotations).toEqual([undefined]); + expect(readAndroidSurfaceRotation).not.toHaveBeenCalled(); + }); +}); + +describe("the aspect guard", () => { + it("falls back to an unrotated capture when the image comes back the wrong shape", async () => { + // Simulates simulator-server's rotation handling changing underneath us: we + // ask for landscape and get a portrait-shaped PNG. Shipping that would be a + // confidently wrong image, so the unrotated capture is preferred. + readAndroidSurfaceRotation.mockResolvedValue(1); + const { rotations, capture } = recorder({ width: 1080, height: 2424 }); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + await captureScreenshotUpright(api, ANDROID, undefined, undefined, undefined, capture); + + expect(rotations).toEqual(["LandscapeLeft", undefined]); + expect(warn).toHaveBeenCalled(); + warn.mockRestore(); + }); + + it("does not second-guess a capture whose size cannot be read", async () => { + readAndroidSurfaceRotation.mockResolvedValue(1); + const { rotations, capture } = recorder(); + readPngSize.mockResolvedValue(null); + + await captureScreenshotUpright(api, ANDROID, undefined, undefined, undefined, capture); + + expect(rotations).toEqual(["LandscapeLeft"]); + }); +});