Skip to content
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

Add ability to copy & paste images that are in uploading state. #17327

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Oct 25, 2024

Suggested merge commit message (convention)

Fix (image): Copying and pasting images in the uploading state is now possible. Closes #16967


Additional information

It looks like we forgot to dump images that contain uploadId to clipboard data. It resulted in blank img being exported to the HTML:

obraz

I added a converter that lookups in file repository and checks if there is matching uploadId entry containing data. If so, it stores base64 of loaded image in src of img element, which is already pretty well handled later on.

Easily reproducible on http://localhost:8125/ckeditor5-image/tests/manual/imageupload.html

Before

before-image-upload-fix-2024-10-25_12.58.54.mp4

After

after-image-upload-fix-2024-10-25_12.55.59.mp4

@arkflpc
Copy link
Contributor

arkflpc commented Oct 28, 2024

After upload is done, will the copied image have src updated to the target or stick to the data-url?

@Mati365
Copy link
Member Author

Mati365 commented Oct 28, 2024

@arkflpc It's updated to the target, as uploadId is no longer present in downcast after successful uploading of image.

arkflpc
arkflpc previously approved these changes Oct 28, 2024
Copy link
Contributor

@arkflpc arkflpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@niegowski , wdyt?

@juliaflejterska
Copy link

@Mati365 We've finished testing the PR with @kubaklatt - it fixes the issue and looks good to us 👍

Copy link
Contributor

@niegowski niegowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you trigger editor.getData() while an image is not fully uploaded, the editor data would contain the data: URL. For the user, it would look like the image is successfully uploaded but the URL would not point to the real asset on the server.
The proposed solution restarts the upload after the drop, but we should just keep the uploadId so the loader can match the given element and continue uploading.

@Mati365
Copy link
Member Author

Mati365 commented Nov 6, 2024

but we should just keep the uploadId

@niegowski we cannot, as it'll be no longer possible to paste such image to another instance of the editor. The second thing is that uploadId will be no longer valid after successful upload of original image, as it's removed from registry to avoid mem leak.

@Mati365
Copy link
Member Author

Mati365 commented Nov 8, 2024

@juliaflejterska / @charlttsie We changed a bit the behavior of this fix, so large data:base64 payload is no longer present in editor.getData and it's no longer possible to copy & paste such element from editor A, editor B (it'll work exactly like it is on production right now). Can you check it again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uploading image gets deleted when being dragged and dropped
4 participants