feat(trace-viewer): render iframe canvas in trace viewer #33809
Conversation
This comment has been minimized.
This comment has been minimized.
ruifigueira
force-pushed
the
feat/tv-iframe-canvas
branch
from
9e96691 to
ddc34a7
Compare
This comment has been minimized.
This comment has been minimized.
| @@ -249,6 +272,10 @@ function snapshotScript(...targetIds: (string | undefined)[]) { | |||
| const targetElements: Element[] = []; | |||
| const canvasElements: HTMLCanvasElement[] = []; | |||
|
|
|||
| let topFrameWindow: Window = window; | |||
| while (topFrameWindow !== topFrameWindow.parent && !topFrameWindow.location.pathname.match(/\/page@[a-z0-9]+$/)) | |||
There was a problem hiding this comment.
What happens if you remove the second condition here? I would have assumed that walking up window.parent should lead you to the top frame without any other conditions.
There was a problem hiding this comment.
It may not be the actual top window. In fact, it's only the top window when we open the snapshot in a new tab. In trace viewer / UI mode, it must be the snapshot root window, which is an iframe:
Maybe topFrameWindow should be called topSnapshotWindow instead.
There was a problem hiding this comment.
Gave it another look. My gut feeling says that we should try to perform as much heavy lifting at snapshot time as possible. We already caught a bug about that in the past: #33575
Also left a question to verify my understanding of the code.
| viewport, | ||
| frames: new WeakMap(), | ||
| }; | ||
| window['__playwright_canvas_render_info__'] = canvasRenderInfo; |
There was a problem hiding this comment.
after reading through this, my impression is that there's a non-trivial order of execution here, where snapshotScript is being executed for all frames in the frame, and they all read each other's window.__playwright_canvas_render_info__ states. Is this understanding right?
There was a problem hiding this comment.
Yes, you're right. Each parent window, before rendering each iframe, extracts the __playwright_canvas_render_info__ value and keeps in that variable. It then removes the attribute and renders the element normally, which will eventually trigger recursively the same process inside the iframe.
There was a problem hiding this comment.
Nice, that makes sense. Could you add a comment about that somewhere, for future readers?
There was a problem hiding this comment.
Also, would __playwright_frame_bounding_rects__ be a more fitting name?
There was a problem hiding this comment.
What about this suggestion I gave on #33809 (comment) ?
Maybe topFrameWindow should be called topSnapshotWindow instead.
| const value = JSON.stringify({ | ||
| left: boundingRect.left / window.innerWidth, |
There was a problem hiding this comment.
By moving this division to the rendering stage, we make an implicit assumption that window.innerWidth will be the same at snapshot time and at rendering time. I'm not sure that's true. Do you think we could perform all the heavy lifting at snapshot time instead? That way, we can be more sure that the bounding rect actually refers to the right pixels in the screenshot.
There was a problem hiding this comment.
I thought of keeping that at snapshot time, but the problem is that most likely we'll hit on XSS issues. I moved those computations to rendering stage because at that point we'll no longer have XSS issues (all rendered frames are under the same domain).
Nevertheless, the end result should be the same, because I capture window.innerWidth for each frame and save it in its __playwright_bounding_rect__ attribute and that's the value I'm using to compute the ratio
There was a problem hiding this comment.
Nevertheless, the end result should be the same, because I capture window.innerWidth for each frame and save it in its playwright_bounding_rect attribute.
That makes total sense to me, and resolves my worry about the render-time differences. nice!
There was a problem hiding this comment.
What I said was not 100% correct: actually the window.inner{Width,Height} is not from iframe's __playwright_bounding_rect__ , it was already being captured at snapshot time:
and those are the dimensions I use at rendering time. That's why I changed the snapshotScript signature to receive the viewport structure.
Test results for "tests 1"2 flaky37248 passed, 650 skipped Merge workflow run. |
Closes: #33779