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

Images copied from the browser are pasted as inline links rather than handled via onImageFilePaste #397

Closed
frankieyan opened this issue Aug 16, 2023 · 5 comments · Fixed by #434
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@frankieyan
Copy link
Member

frankieyan commented Aug 16, 2023

Not sure if this is truly a bug, but when you copy an image from the browser and paste it into the composer, it shows up as an inline image that is sourced from the original URL, instead of being uploaded (via onImageFilePaste).

This is somewhat of a problem if the URL requires authentication, such as when copied from GitHub. It's also a bit inconsistent compared to other instances of inline images where they are always uploaded to our servers. The same image copied from a browser but pasted to other desktop apps (e.g. Signal, Whatsapp) often end up being uploaded as well.

For our users, this may not be very transparent because the pasted image will likely still be viewable by them, and they'd only realize a problem if they inspect the URL of the image.

Reproduction

image

Additional information

Clipboard data from https://evercoder.github.io/clipboard-inspector/
image

It's possible that this conflicts with the solution we have in place to prevent tabular data copied from Numbers from pasting an image: #290

@frankieyan frankieyan added the question Further information is requested label Aug 16, 2023
@rfgamaral
Copy link
Member

Not sure if this is truly a bug, but when you copy an image from the browser and paste it into the composer, it shows up as an inline image that is sourced from the original URL, instead of being uploaded (via onImageFilePaste).

Yes, I believe this should be considered a bug. Explicitly copied images should always be handled by uploader (if there is one).

It's possible that this conflicts with the solution we have in place to prevent tabular data copied from Numbers from pasting an image: #290

Indeed, this is the problem, and it's a tricky one. To have our cake and eat it too, we need to figure out a better way to distinguish the user intention when copying something to the clipboard.

Looking at multiple clipboard data examples, I can think of a few ways to work around this, but they are all band aids that could be causing some other issue (like this one) in the future. The proper fix would be to identify edge cases (like Numbers adding an image file to the clipboard), and work around only those edge cases. But for Numbers, specifically, I can't think of a way to identify that edge case.

@rfgamaral rfgamaral added bug Something isn't working good first issue Good for newcomers and removed question Further information is requested labels Aug 17, 2023
@rfgamaral
Copy link
Member

A more pragmatic solution to this problem could be to remove the solution that we have in place to prevent tabular data copied from Numbers from pasting an image, which would prioritize pasting images (if one is available), thus forcing users to use the specific "paste as plain-text" keyboard shortcut if they want to paste text.

This may not be what users expected, because they expect things to work intelligently, in a way that the system "guesses" what they want (I expect that too, but I'm not sure it will be possible here), but it would be a way to have everything working, which right now we don't have.

@frankieyan Thoughts?

@frankieyan
Copy link
Member Author

A more pragmatic solution to this problem could be to remove the solution that we have in place to prevent tabular data copied from Numbers from pasting an image, which would prioritize pasting images (if one is available), thus forcing users to use the specific "paste as plain-text" keyboard shortcut if they want to paste text.

I agree with this. We don't have any feedback from users that pasting from Numbers is something that happens often enough to warrant images being deprioritized, and I think having onImageFilePaste handle pasted images consistently is more important here.

@rfgamaral
Copy link
Member

@frankieyan In that case, and since I don't have access to Numbers, do you mind doing the following:

  • Remove these lines and launch the local Storybook
  • Copy a table from Numbers, and...
    • Paste it to the rich-text editor with Cmd+V
      • Was the image pasted?
    • Paste it to the rich-text editor with Cmd+Shift+V
      • Was the table text pasted correctly?

If possible, screen record these tests so I can double-check everything is working as expected.

@frankieyan
Copy link
Member Author

@frankieyan In that case, and since I don't have access to Numbers, do you mind doing the following:

  • Remove these lines and launch the local Storybook
  • Copy a table from Numbers, and...
    • Paste it to the rich-text editor with Cmd+V
      • Was the image pasted?
    • Paste it to the rich-text editor with Cmd+Shift+V
      • Was the table text pasted correctly?

If possible, screen record these tests so I can double-check everything is working as expected.

Sorry Ricardo, I haven't found the time to come back to this issue yet, as we have a big release coming up tomorrow. I should be able to test this out closer to the end of the week!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants