Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix copying webview image via context menu (#292567) #320234
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
base: main
Are you sure you want to change the base?
Fix copying webview image via context menu (#292567) #320234
Changes from all commits
341435aFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
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.
Addressed: setting the target now goes through a
setContextMenuImageTarget()helper that arms asetTimeoutto drop the reference aftercontextMenuImageTargetTtl, so the DOM node is released even if no copy occurs. Any prior timer is cleared on each set, and the copy handler routes its clear through the same helper.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.
Addressed: clearing now fully resets the state machine —
setContextMenuImageTarget(undefined)sets the target to undefined, the timestamp to 0, and the timer id to undefined, and the TTL timeout callback does the same. No stale timer id or timestamp is retained.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.
Addressed (conservatively): kept the short 20ms focus-retry that we verified works in practice, but added a best-effort
frameWindow.focus()before the wait to help reacquire focus, and kept the wait short to preserve the transient user activation. I avoided a larger rewrite of the retry mechanism since this path is the activation-sensitive part and is shared with the proven Markdown-preview behavior.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.
Addressed: the early return now logs via
console.error("Cannot copy image: the clipboard API is not available in this webview.") so the reason image-copy didn't happen is diagnosable.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.
Addressed: replaced the flat pixel cap with a memory-based limit — the canvas backing buffer is bounded to ~256 MB (
width * height * 4) and each dimension is capped at 16384. Images exceeding either fall back to re-fetching the original bytes.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.
Addressed:
ClipboardItemis now resolved from the content frame's realm (frameWindow.ClipboardItem) and used for both the guard and thenewcalls, so it no longer bails out or constructs against the outer realm.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.
Addressed: added a max-pixel guard (
16384 * 16384) before allocating the canvas. Images exceeding it (or with no natural dimensions) skip the canvas path and fall back to re-fetching the bytes, avoiding large-bitmap memory spikes/hangs.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.
The remembered target is already tightly scoped: it is reset on every
contextmenuevent and consumed (set toundefined) the moment a copy is serviced, and the image branch only runs when the frame selection is empty/collapsed. I deliberately did not clear it on context-menu dismissal: the host sendsset-context-menu-visible {visible:false}fromonDidHideContextMenu, which fires when the menu closes — including when the user clicks "Copy". The ordering of that hide message vs theexecCommand('copy')message is not guaranteed, so clearing on dismiss would race the copy and could break the feature. The only residual case is right-click-image → never copy → later bare Ctrl+C with no selection, which copies the image instead of nothing — a benign outcome that matches the analogous Markdown-preview behavior.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.
Follow-up: now also implemented a time-based expiry as suggested. The remembered image is only honored when the copy arrives within 5s of the context menu opening (
contextMenuImageTargetTtl), in addition to being consumed on use, reset on everycontextmenu, and gated on an empty selection. This scopes the state to the menu invocation without clearing on menu-dismiss, which would have raced the copy message.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.
The remembered target is already tightly scoped: it is reset on every
contextmenuevent and consumed (set toundefined) the moment a copy is serviced, and the image branch only runs when the frame selection is empty/collapsed. I deliberately did not clear it on context-menu dismissal: the host sendsset-context-menu-visible {visible:false}fromonDidHideContextMenu, which fires when the menu closes — including when the user clicks "Copy". The ordering of that hide message vs theexecCommand('copy')message is not guaranteed, so clearing on dismiss would race the copy and could break the feature. The only residual case is right-click-image → never copy → later bare Ctrl+C with no selection, which copies the image instead of nothing — a benign outcome that matches the analogous Markdown-preview behavior.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.
Follow-up: now also implemented a time-based expiry as suggested. The remembered image is only honored when the copy arrives within 5s of the context menu opening (
contextMenuImageTargetTtl), in addition to being consumed on use, reset on everycontextmenu, and gated on an empty selection. This scopes the state to the menu invocation without clearing on menu-dismiss, which would have raced the copy message.Uh oh!
There was an error while loading. Please reload this page.