fix(webview): support input for elements inside cross-origin <iframe>#41463
fix(webview): support input for elements inside cross-origin <iframe>#41463dcrousso wants to merge 2 commits into
<iframe>#41463Conversation
2bac9db to
4d7c0b8
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| return { target, frame: null, doc: null, x, y }; | ||
| const frame = target as HTMLIFrameElement | HTMLFrameElement; | ||
| const frameRect = frame.getBoundingClientRect(); | ||
| const frameStyle = frame.ownerDocument.defaultView!.getComputedStyle(frame); |
There was a problem hiding this comment.
This (and in other places) looks like a cross origin reach?
There was a problem hiding this comment.
the ownerDocument is the ancestor of the <iframe> (as opposed to the contentDocument which is inside the <iframe>)
this is a replacement for a direct window.getComputedStyle(frame) which may not work since the window could be the wrong object if at this point the context is already inside an <iframe>
| if (active && (active.localName === 'iframe' || active.localName === 'frame')) { | ||
| let inner: Element | null = null; | ||
| try { | ||
| const doc = (active as HTMLIFrameElement | HTMLFrameElement).contentDocument; |
There was a problem hiding this comment.
I still don't think the bulk of this change makes sense. Each frame has its own injected script with its own document and window object, available on the injected script itself. They are only ones, you can't switch between them in the injected script.
There was a problem hiding this comment.
you're right that each <iframe> get its own injected script instance and that there's no way to just magically move stuff between them
the injected script will attempt drill down as deep as possible for the desired operation (e.g. positionInIFrame will elementFromPoint the desired coordinates, activeIFrame will look at the activeElement, etc.). if the result is an <iframe> (i.e. we've drilled down as deep as we can go) then include a handle for that as part of the reply alongside all of the other data computed so far (i.e. the local x, y, target, etc.)
the server will then look at the result to see if it's included the <iframe> and if so, it will then use the the protocol DOM.Node.contentDocument to cross the boundary and use that as the new execution context to repeat the above
(note that the code referenced in this comment was only to avoid the round-trip back to the server when dealing with same-origin <iframe>)
4d7c0b8 to
de15944
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
de15944 to
467ba5f
Compare
This comment has been minimized.
This comment has been minimized.
467ba5f to
f0b3c02
Compare
This comment has been minimized.
This comment has been minimized.
f0b3c02 to
bde5642
Compare
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.
This comment has been minimized.
This comment has been minimized.
bde5642 to
f81977b
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f81977b to
0e68b37
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0e68b37 to
ea8814c
Compare
|
|
||
| // Descend through open shadow roots so synthetic events land on the actual | ||
| // element under the pointer rather than on the shadow host. | ||
| private _deepElementFromPoint(x: number, y: number): Element | null { |
There was a problem hiding this comment.
Looks like a readability regression - why removing a fine utility method instead of calling it.
There was a problem hiding this comment.
yeah i pushed before i committed undoing this change. my apologies
| } | ||
|
|
||
| private async _dispatchWebViewInput(progress: Progress, method: string, params: any): Promise<void> { | ||
| if (method === 'mouseMove') { |
There was a problem hiding this comment.
You don't want to go from the strongly typed called, into dispatched and then start sniffing dispatched code. Stay in the strongly typed world and only start dispatching when crossing the injected script boundary, like previously.
|
|
||
| private async _dispatchWebViewInput(progress: Progress, method: string, params: any): Promise<void> { | ||
| if (method === 'mouseMove') { | ||
| await this._dispatchMouseMove(progress, params); |
There was a problem hiding this comment.
It is now quite hard to follow the mouse move dispatching - it has special handling in 3 layers
| // Mouse move maintains hover state across frames: as the pointer crosses an <iframe> | ||
| // boundary it leaves the elements in one frame and enters elements in another. | ||
| private async _dispatchMouseMove(progress: Progress, params: any): Promise<void> { | ||
| const path = await this._pointerPath(progress, params.x as number, params.y as number); |
There was a problem hiding this comment.
This code should be in the RawMouseImpl, straightforward.
| const childFrame = await this._childFrame(progress, iframe); | ||
| if (!childFrame) | ||
| break; | ||
| ({ x, y } = await progress.race(position.evaluate(result => ({ x: result.x, y: result.y })))); |
There was a problem hiding this comment.
do not reassign method input params, keep things readable.
| try { | ||
| return await progress.race(this.getContentFrame(iframe)); | ||
| } finally { | ||
| iframe.dispose(); |
There was a problem hiding this comment.
Either don't pass the ownership, or call childFrameAndDispose for clarity.
| if (framesToHover.includes(hoveredFrame) || !frames.includes(hoveredFrame)) | ||
| continue; | ||
| const context = await progress.race(hoveredFrame.mainContext()); | ||
| await progress.race(context.evaluate(() => (globalThis as any).__pwWebViewInput.clearHover())); |
There was a problem hiding this comment.
This feels wrong, what if I just move the cursor 5 pixels to the right, without existing and elements? Should be no mouseout events, etc ?
There was a problem hiding this comment.
if we havent moved out of any <iframe> then we wont hit this as every value in _lastHoveredFrames will be present in framesToHover
ill add a comment to clarify
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
each `<iframe>` has its own mouse position as such, moving the pointer into or out of an `<iframe>` did not dispatch `mouseleave`/`mouseout`/etc. instead, track what `<iframe>` are hovered on the server so that movement is coordinated globally
ea8814c to
9bba18a
Compare
Test results for "MCP"3 failed 7706 passed, 1235 skipped Merge workflow run. |
Test results for "tests 1"3 flaky49515 passed, 1164 skipped Merge workflow run. |
No description provided.