-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(canvas snapshots): position mismatch in headless mode #33575
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Why width and not height? If anything, I'd assume that width might differ due to different scrollbars, but height would likely be correct. Do you have a real repro that shows the discrepancy in width and/or height?
I'd skip the test in this case. |
I was wondering the same. In the repro the customer shared, the height differed between headed and headless, but the width did not. If we know that the width will also differ because of scrollbars, then maybe this whole approach doesn't work and we need to capture dimensions together with the snapshot itself. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that PR description is now wrong.
if (!boundingRectAttribute) | ||
continue; | ||
|
||
const boundingRect = JSON.parse(boundingRectAttribute) as { left: number, top: number, right: number, bottom: number }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try/catch parsing just in case.
|
||
const partiallyUncaptured = xEnd > 1 || yEnd > 1; | ||
const fullyUncaptured = xStart > 1 || yStart > 1; | ||
drawCheckerboard(context, canvas); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep not drawing the checkerboard for fullyUncaptured
canvases, to avoid any performance penalties.
Test results for "tests 1"1 flaky36885 passed, 650 skipped Merge workflow run. |
The code for canvas rendering currently assumes that the screenshot it crops from has the same aspect ratio as the frame it's displaying in. That assumption is wrong. The fix is record the canvas position at snapshot time, so that it's relative to the screnshot aspect ratio.
Closes #33562.