You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
factor the page's actual window.devicePixelRatio into screenshot downscaling
keep CDP clip dimensions in CSS pixels while bounding the resulting bitmap dimensions
add a regression test for the real defaultViewport: null path with a forced 2x device scale factor
Why
Screenshot source boxes and CDP clip dimensions are expressed in CSS pixels, but the returned bitmap is scaled by the page's device pixel ratio. The previous calculation compared the CSS width directly with screenshotMaxWidth, so a 2x page could return an image twice the configured bound.
The new calculation derives the clip scale from box × devicePixelRatio, making the limit apply to the image pixels sent to the model.
Validation
npm run check-format
npm run build
npm run test -- tests/tools/screenshot.test.ts --test-name-pattern='honors screenshotMaxWidth at device scale factors above 1'
npm run test -- tests/tools/screenshot.test.ts --test-skip-pattern='with full page resulting in a large screenshot'
The new regression fails on main with a 200px-wide image and passes with an exact 100px result after this change. I also attempted the full suite with retries; this local environment still times out in unrelated daemon/extension E2E tests and hits the existing Page.captureScreenshot: Page is too large case, which reproduces on pristine main.
Rebased again onto the latest main (0b4390d); the current head is c801c83. git range-diff reports the PR commit as unchanged. The two new upstream commits include the repository-wide verifyFilesSchema migration, which touches src/tools/screenshot.ts in a separate hunk; the rebase preserved both that new object-schema form and the HiDPI calculation without conflict. The introduced HiDPI regression test and the related no-viewport downscaling test pass on the current head. For transparency, the full screenshot file still hits Page.captureScreenshot: Page is too large in the pre-existing large full-page case; the identical command reproduced the identical failure on a detached clean origin/main, so no unrelated code change was made. The Memory leak tests, Check code before submitting, and Compile and run tests workflows are again action_required before creating jobs. Could a maintainer approve those runs and review the rebased PR when convenient?
Diff looks right to me. The emitted bitmap is box * scale * dpr, so folding the ratio into the bound is the right place to fix it, and at a ratio of 1 the calculation is unchanged.
I pulled both versions of computeDownscaleClip() out and ran them over a matrix of box sizes, ratios and bounds. Nothing came back above the bound, nothing main clips stopped being clipped, and the sub-pixel guard fires on identical inputs either way since the ratio cancels out of it.
The new test also can't pass on main, which returns 200 where it asserts 100.
Still behind main, and the test jobs need a maintainer to approve the run.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2531
What changed
window.devicePixelRatiointo screenshot downscalingdefaultViewport: nullpath with a forced 2x device scale factorWhy
Screenshot source boxes and CDP clip dimensions are expressed in CSS pixels, but the returned bitmap is scaled by the page's device pixel ratio. The previous calculation compared the CSS width directly with
screenshotMaxWidth, so a 2x page could return an image twice the configured bound.The new calculation derives the clip scale from
box × devicePixelRatio, making the limit apply to the image pixels sent to the model.Validation
npm run check-formatnpm run buildnpm run test -- tests/tools/screenshot.test.ts --test-name-pattern='honors screenshotMaxWidth at device scale factors above 1'npm run test -- tests/tools/screenshot.test.ts --test-skip-pattern='with full page resulting in a large screenshot'The new regression fails on
mainwith a 200px-wide image and passes with an exact 100px result after this change. I also attempted the full suite with retries; this local environment still times out in unrelated daemon/extension E2E tests and hits the existingPage.captureScreenshot: Page is too largecase, which reproduces on pristinemain.