benwinding / quill-image-compress

A Quill rich text editor Module which compresses images uploaded to the editor
https://benwinding.github.io/quill-image-compress/src/demo.html
MIT License
123 stars 30 forks source link

Fixing handling when pasted mutliple file types #47

Open StaticBR opened 9 months ago

StaticBR commented 9 months ago

New Microsoft Products also paste multiple Data transfer items, like: grafik

This PR fixes this.

benwinding commented 9 months ago

Hi @StaticBR 👋 Not sure what this PR is doing, are you able to give me a recording of what exactly this fixes? Like what "New Microsoft Products" do this?

StaticBR commented 9 months ago

Hi @benwinding , sure. When copy /pasting from current Excel or Outlook the TransferredDataItem contain multiple mime-types of data. ( See Screenshot) Currently the plugin does in this case always paste an image instead of the available HTML. Which is bad for the usability. So I changed the logic: If there is any "text/html" in the TransferredDataItem[] present, ignore the images and use the html.

benwinding commented 9 months ago

What's different from the current behavior though? It seems to do exactly that, when pasting from other websites

Using the example site here -> https://benwinding.github.io/quill-image-compress/src/demo.html

https://github.com/benwinding/quill-image-compress/assets/11782590/5efaa357-e38a-4d33-a110-aee2e1fa00a3

StaticBR commented 9 months ago

Hi Ben, I'm so sorry there was an small error, I wanted to check the files not the images. That's what i get for thinking I can fix a bug blindly. Here you can see the difference when pasting from a local Excel : GIF 30 01 2024 11-20-58

Bests Ralf

StaticBR commented 9 months ago

@benwinding: It would be be nice, if you can make a Bugfix Release for this issue. Thanks in advance.

StaticBR commented 9 months ago

@benwinding sry to bother you again. Will you merge this change or do you have any concerns with it?

I'm asking, because i have a currently pending Bug, in a Software depending on this. Thanks.

benwinding commented 9 months ago

Hi StaticBR,

So it took me a while to understand this use case, but I think I've fixed it. Please try installing the new version and try the updated demo here:

quill-image-compress@1.2.31

Does it work now? (Apologies I can't merge your fix, as it broke the copy/paste in the browser)

StaticBR commented 9 months ago

Hi Ben, thanks for the Effort! I had a look at your changes and unfortunately the issue still persists. The problem is, if there are multiple mime types of the same data pasted. The plugin then forces Quill to always paste the image. Instead of the maybe available HTML. This is done preventing the paste with event.preventDefault(); and manually adding the image via a data url.

My solution was basically to say, if there is an pasted "text/html" compress nothing and let the browser just use the html. Sure if there is an image embedded within this html it will not be compressed. You said my solution "broke copy / paste in the browser", can you give me an example for that? Maybe I'm able to fix it.

I checked out you code and added a simple within the handleDataTransferList : if( items.some(f => f.type ==='text/html')){ return; } And the fix is working again. Here again is a screen recording, with some debugging infos: GIF 02 02 2024 19-26-56

benwinding commented 9 months ago

Hi @StaticBR, Thanks for adding more details 🙏

You said my solution "broke copy / paste in the browser", can you give me an example for that? Maybe I'm able to fix it.

Here's how your PR breaks copying an image (see video).

User copies an image in the browser, which the following fileTypes in the DataTransferItemList from the clipboard

fileTypes: [
  {type: 'text/html', kind: 'string'},
  {type: 'image/png', kind: 'file'},
] 

Your PR ignores compressing the image/png due to it having text/html in the array. This is an extremely common use-case, which is why I can't merge this as it is...

https://github.com/benwinding/quill-image-compress/assets/11782590/a13a395f-d5fb-497c-b135-2228c8cf0de5

Are you able to record exactly what program you're copying from? (video is preferred over gif)

image

Is this a cell in excel with the text "Critical"??? Don't know where this was copied from...

I'm on Mac and don't have any microsoft products to test... so if you can reproduce it with another website, or some mac compatible program that would be appreciated...

Would love to fix this, so give me as much detail as you can 🙏

StaticBR commented 9 months ago

Hey Ben,

sry for the long waiting time. But your last post has pointed me to a few more issues.

If you currently paste an html code from a website the plugin will currently leave the images completely untouched. So it will basically keep the original img.src of the image, even if this points to a remote URL.

I am not sure if that is whats people expecting. I surly was not. By solving this issue I think I can also solve your and my issues together.

So I added handling of pasted HTML to your plugin. The html will now be parsed all images will be extracted and processed.

Here is an example of pasting html with an image from a foreign Url: Paste foreign  Image

Even Pasting from Excel works now like a charm: Paste from Excel

Unfortunately I had to add a Breaking Change to this pull request: The option insertIntoEditor can no longer work if the plugin should be able to process html. Because of this I have replaced it with a uploadImage method. This allows us to also Upload images found within a pasted html.

Please have a look at this change and let me know if you are willing to merge this change.

Bests Ralf

StaticBR commented 8 months ago

FYI: I have added another Fix that prevented PNGs from retaining a transparent Background in some cases.