-
Notifications
You must be signed in to change notification settings - Fork 30.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(markdown-preview/copy-image): invalid error with blob when download image. #233497
Conversation
…ad external image url.
@microsoft-github-policy-service agree |
@@ -11,6 +11,7 @@ import { SettingsManager, getData } from './settings'; | |||
import throttle = require('lodash.throttle'); | |||
import morphdom from 'morphdom'; | |||
import type { ToWebviewMessage } from '../types/previewMessaging'; | |||
import { isURL } from '../src/util/is-url'; |
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.
Code here not access the parent directory. Instead only load from preview-src
'image/png': new Promise((resolve) => { | ||
const imageSrc = image.getAttribute('data-src') || 'src-not-found'; | ||
|
||
if (isURL(imageSrc)) { |
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.
Shouldn't this try the image and then only write text if that fails?
const imageSrc = image.getAttribute('data-src') || 'src-not-found'; | ||
|
||
if (isURL(imageSrc)) { | ||
await navigator.clipboard.writeText(imageSrc); |
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.
Instead of writing text, maybe use apis to select the image and then use execCommand('copy')
tocopy it. It still won't write out image data but it will better match what browser do. Most of them write out text/html
too for example
@@ -16,6 +16,10 @@ export class CopyImageCommand implements Command { | |||
|
|||
public execute(args: { id: string; resource: string }) { | |||
const source = vscode.Uri.parse(args.resource); | |||
this._webviewManager.findPreview(source)?.copyImage(args.id); | |||
const imagePreview = this._webviewManager.findPreview(source); |
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 revert this
})]); | ||
}); | ||
|
||
await navigator.clipboard.write([new ClipboardItem({ 'image/png': blobImage })]); |
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.
I'd revert this too. You want to call write
early so that browsers know there is pending image data. Now it will instead wait for the blob to be created and only then write the clipboard. It's possible that clipboard changes between those steps
When you copied an image with an external link, it gave a CORS error.
Since it wasn't possible to fix the CORS issue directly to resolve this problem, instead of copying an image from an external source, it now copies the link to your clipboard so you can access the image. For local images, it still copies normally like before.
before:
Screen.Recording.2024-11-10.at.13.20.31.mov
after:
Screen.Recording.2024-11-10.at.13.22.08.mov
issue: #205624