ckeditor
Add support for uploading images included in pasted HTML content
Suggested merge commit message (convention)
Feature (image): Add support for uploading images included in pasted HTML content. Closes: #5152
Hey! Thanks for the contribution. Before jumping into reviewing the code in depth, it would be great to get some explanation of what was introduced. The #5152 has a very long discussion of different scenarios.
If you could provide:
- Which scenario are you trying to cover, and are most interested in?
- What decisions have you made and why (e.g.,
image.setAttribute( 'crossorigin', 'anonymous' );looks interesting). - Info on the lack of configuration, as far as I can see, this is enabled by default, which could be problematic for some users, as it may lead to some automatic copyright abuses. Curious about your thoughts.
- Fail scenarios, is there any path in which this won't work, what is a recovery path when an image is not downloadable?
Hey @Witoso!
The main scenario we're interested in is avoiding hotlinking images. We are migrating to CKEditor from Froala, which had built in support for uploading images to a server, rather than hotlinking images, which is generally considered bad practice.
This PR adds support for:
- Pasting HTML that contains one or more
<img>tags and automatically uploading them using theFileRepository. This includes images copied using the "Copy image" option in your browser, since that copies the image as both a file and an HTML<img>tag. - Pasting the URL of an image and automatically uploading it using the
FileRepository.
Some remote servers may include CORS headers that prevent the image from being downloaded, so the crossorigin="anonymous" attribute is added to attempt to circumvent that. This appears to work in my manual testing so far.
You're correct in thinking this is enabled by default. I think it would make sense for this to be the default functionality, due to hotlinking images being generally considered a bad practice as mentioned before. If it would be considered a breaking change, or if there are any other backwards compatibility concerns, I'd understand if enabling it via a config option was preferred.
As for copyright concerns, as far as I'm aware copyright is based on the domain the image is displayed on rather than the domain it's hosted on - but I could be wrong here.
The PR may require some additional error handling for images that can't be downloaded, as I haven't tested this scenario yet.
The PR may require some additional error handling for images that can't be downloaded, as I haven't tested this scenario yet.
After some more testing it looks like any issues fetching the image are handled by existing error handling logic for the loader, since I'm just passing the fetchImage Promise into the loader. The same error handling logic that's used if a local file can't be fetched for some reason will apply to remote images.
If an image can't be fetched, the <img> element will be removed from the pasted content.
I think both act of hot linking, and the download action could be illegal (depending on country, jurisdiction, fair use, etc., not a legal advice :wink:). So I think we could get this argument out of the way.
I think giving users an option is most likely a must-have, we need to discuss internally what's the default, etc. I will get back to you.
Some remote servers may include CORS headers that prevent the image from being downloaded, so the
crossorigin="anonymous"attribute is added to attempt to circumvent that.
Gotcha, most likely needs to be removed afterward when you replace the src, as it's no longer needed.
I think giving users an option is most likely a must-have, we need to discuss internally what's the default, etc. I will get back to you.
No worries, makes sense.
Gotcha, most likely needs to be removed afterward when you replace the
src, as it's no longer needed.
The attribute is only set on the img element that's created in order to download the image and convert it to a Blob, not the actual img element that's inserted into the editor content, so this shouldn't be a concern.
As we discussed it, we should keep by default the current behavior, and the configuration option should be added, I guess to the AutoImage, that will switch the hot linking to the upload. @felixpackard we will take it on ourselves.
There has been no activity on this PR for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the contribution, leave a comment or reaction under this PR.
We've closed your PR due to inactivity. While time has passed, the core of your contribution might still be relevant. If you're able, consider reopening a similar PR.