Repository navigation
fix: calculate pixel ratio based on actual screenshot data in case of oopif - #1331
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
✅ Testplane browser-env run succeed
|
✅ Testplane E2E run succeed
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48c022db88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
commit: |
KuznetsovRoman
left a comment
KuznetsovRoman
left a comment
There was a problem hiding this comment.
I suggest adding some kind of comment to "css pixels" x "png pixels" comparison so at least it would be clear trade-off.
Besides, /ok
e79f7ce to
1f4d00b
Compare
1f4d00b to
064716e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 064716e69a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const pixelRatio = imageSize.width / viewportSizeInCss.width; | ||
| if (Math.abs(imageSize.height - viewportSizeInCss.height * pixelRatio) > 1) { | ||
| throw new Error( |
There was a problem hiding this comment.
When screenshotMode is fullpage (or auto detects a full-page bitmap), Camera.captureViewportImage stores the document-sized calibrated area in uncroppedSize before cropping the returned image to the viewport. Comparing that height with the CSS viewport here therefore always exceeds the tolerance on scrollable pages and throws the generic consistency error, so mobile-emulated viewport and element captures fail without reaching the DPR retry. The validation needs dimensions representing the viewport before user margins, rather than the raw full-page dimensions.
Useful? React with 👍 / 👎.
What's done?
This is a second iteration of out-of-process-iframe fix. In the first fix, we relied on the fact that after screenshot pixel ratio should be correct. However, it turned out that there are race conditions possible when a page has iframe appearing in between capturing screenshot and getting pixelRatio value.
The new logic computes pixel ratio by dividing physical pixel screenshot width by css viewport width. This one should be 100% reliable.