-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(tracing): clip canvas
contents from screenshots
#33119
base: main
Are you sure you want to change the base?
Changes from 3 commits
79a5407
6d0f0be
e19f0ee
8687e55
82ad4ec
b3bd73b
8c35ade
bf5f718
23822dc
c70f372
ccdfeee
3389fdd
45d1b54
2dfa90f
309bea0
5aae14e
455be13
ea4116e
5d4d73d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,7 +78,7 @@ export class SnapshotRenderer { | |
return this._snapshots[this._index].viewport; | ||
} | ||
|
||
render(): RenderedFrameSnapshot { | ||
render(swScope: string, traceURL: string): RenderedFrameSnapshot { | ||
const result: string[] = []; | ||
const visit = (n: NodeSnapshot, snapshotIndex: number, parentTag: string | undefined, parentAttrs: [string, string][] | undefined) => { | ||
// Text node. | ||
|
@@ -154,12 +154,16 @@ export class SnapshotRenderer { | |
const html = lruCache(this, () => { | ||
visit(snapshot.html, this._index, undefined, undefined); | ||
|
||
const screenshotURL = new URL(`./screenshot/${snapshot.pageId}`, swScope); | ||
screenshotURL.searchParams.set('trace', traceURL); | ||
screenshotURL.searchParams.set('name', this.snapshotName!); | ||
|
||
const html = result.join(''); | ||
// Hide the document in order to prevent flickering. We will unhide once script has processed shadow. | ||
const prefix = snapshot.doctype ? `<!DOCTYPE ${snapshot.doctype}>` : ''; | ||
return prefix + [ | ||
'<style>*,*::before,*::after { visibility: hidden }</style>', | ||
`<script>${snapshotScript(this._callId, this.snapshotName)}</script>` | ||
`<script>${snapshotScript(screenshotURL.toString(), this._callId, this.snapshotName)}</script>` | ||
].join('') + html; | ||
}); | ||
|
||
|
@@ -242,8 +246,8 @@ function snapshotNodes(snapshot: FrameSnapshot): NodeSnapshot[] { | |
return (snapshot as any)._nodes; | ||
} | ||
|
||
function snapshotScript(...targetIds: (string | undefined)[]) { | ||
function applyPlaywrightAttributes(unwrapPopoutUrl: (url: string) => string, ...targetIds: (string | undefined)[]) { | ||
function snapshotScript(screenshotURL: string | undefined, ...targetIds: (string | undefined)[]) { | ||
function applyPlaywrightAttributes(unwrapPopoutUrl: (url: string) => string, screenshotURL: string | undefined, ...targetIds: (string | undefined)[]) { | ||
const kPointerWarningTitle = 'Recorded click position in absolute coordinates did not' + | ||
' match the center of the clicked element. This is likely due to a difference between' + | ||
' the test runner and the trace viewer operating systems.'; | ||
|
@@ -299,6 +303,25 @@ function snapshotScript(...targetIds: (string | undefined)[]) { | |
} | ||
} | ||
|
||
const canvases = root.querySelectorAll('canvas'); | ||
if (canvases.length > 0 && screenshotURL) { | ||
const img = new Image(); | ||
img.onload = () => { | ||
for (const canvas of canvases) { | ||
const context = canvas.getContext('2d')!; | ||
|
||
const boundingRect = canvas.getBoundingClientRect(); | ||
const xStart = boundingRect.left / window.innerWidth; | ||
const yStart = boundingRect.top / window.innerHeight; | ||
const xEnd = boundingRect.right / window.innerWidth; | ||
const yEnd = boundingRect.bottom / window.innerHeight; | ||
|
||
context.drawImage(img, xStart * img.width, yStart * img.height, (xEnd - xStart) * img.width, (yEnd - yStart) * img.height, 0, 0, canvas.width, canvas.height); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. implemented all of it |
||
} | ||
}; | ||
img.src = screenshotURL; | ||
} | ||
|
||
{ | ||
const body = root.querySelector(`body[__playwright_custom_elements__]`); | ||
if (body && window.customElements) { | ||
|
@@ -401,7 +424,7 @@ function snapshotScript(...targetIds: (string | undefined)[]) { | |
window.addEventListener('DOMContentLoaded', onDOMContentLoaded); | ||
} | ||
|
||
return `\n(${applyPlaywrightAttributes.toString()})(${unwrapPopoutUrl.toString()}${targetIds.map(id => `, "${id}"`).join('')})`; | ||
return `\n(${applyPlaywrightAttributes.toString()})(${unwrapPopoutUrl.toString()}, ${JSON.stringify(screenshotURL)}${targetIds.map(id => `, "${id}"`).join('')})`; | ||
} | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1439,6 +1439,15 @@ test.skip('should allow showing screenshots instead of snapshots', async ({ runA | |
await expect(screenshot).toBeVisible(); | ||
}); | ||
|
||
test('canvas clipping', async ({ runAndTrace, page, server }) => { | ||
const traceViewer = await runAndTrace(async () => { | ||
await page.goto(server.PREFIX + '/screenshots/canvas.html'); | ||
await page.waitForTimeout(1000); // ensure we could take a screenshot | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I predict this will be flaky 😄 We usually do a few rafs in the hopes it will redraw, but even that does not always help. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's a raf? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
}); | ||
|
||
await traceViewer.page.pause(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this test isn't gonna stay, obviously. but you can use it to play around with the feature: @dgozman what's the best way of testing this - should we trace a page with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that would be a flaky test, unfortunately. Perhaps just check some test-specific logging, like we do in recorder tests: if (isUnderTest) console.error(`drawn 15;23-184;566 screenshot rect on the canvas`); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks, good idea! implemented |
||
}); | ||
|
||
test.skip('should handle case where neither snapshots nor screenshots exist', async ({ runAndTrace, page, server }) => { | ||
const traceViewer = await runAndTrace(async () => { | ||
await page.goto(server.PREFIX + '/one-style.html'); | ||
|
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.
Please handle
img.onerror
as well.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.
done