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

Feat: Add Ability to Copy Images and Image URLs into the Clipboard #768

Merged
merged 14 commits into from
Aug 4, 2021

Conversation

George-lewis
Copy link
Contributor

@George-lewis George-lewis commented Jul 31, 2021

Hello maintainers of HoverZoom, I raise this PR to add some long requested functionality: The ability to copy images and their links into the clipboard.

This should address the following issues: #459, #750, and #765

I have set the default keys to C for "Copy" and L for "Copy Link".

The functionality is fairly straightforward, you hover over an image and press one of the buttons to either copy the image into your clipboard, or the direct link to that image or video.

I tried to add entries to all the English locales I could think of, please let me know if I missed any.

Unfortunately there's an unavoidable caveat to the copy image functionality. Some sites have their CORS set up in such a way (I have found a few) that while you can hoverzoom an image on a particular page, you cannot interact with it in the script. My current code will emit an error when calling .toBlob, and it will complain about attempting to export a tainted canvas. Similarly you cannot fetch the source URL either.

Let me know what you think. Thanks.

Copy link
Owner

@extesy extesy left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request, @George-lewis! It looks good to me overall, just one question about using alternative to copying canvas.

js/hoverzoom.js Outdated
}

// Else render to a canvas to convert to a png
const canvas = document.createElement("canvas");
Copy link
Owner

Choose a reason for hiding this comment

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

Is it possible to use imgDetails.url to copy the actual image file like https://web.dev/async-clipboard/ article did? It might resolve some of your problems with canvas.

Copy link
Contributor Author

@George-lewis George-lewis Aug 1, 2021

Choose a reason for hiding this comment

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

I'm not too familiar with all the stuff going on with the global variables in this file. Is imgDetails.url the same as hz.hzImg.find('img').get(0).src?

I'm having trouble finding docs for this, but I get the impression that Chrome doesn't support copying non-png images.
I think this source file documents all of the supported types in the clipboard, but I'm not sure.

In any case when I try to copy a jpeg the browser says: Uncaught (in promise) DOMException: Type image/jpeg not supported on write. Feel free to confirm this for yourself, you could modify toBlob to always do a fetch, and then try to copy a jpeg.

I really tried to find a way around CORS here, but unfortunately I just couldn't. The powers that be decided that you can embed an image in a page, but not fetch nor export it from a canvas. If you have any ideas here I'm all for it.

Anyway this is why I'm using the canvas, it's the recommended way to convert images to png in the browser. From your article:

To write an image to the clipboard, you need the image as a blob. One way to do this is by requesting the image from a server using fetch(), then calling blob() on the response.

Requesting an image from the server may not be desirable or possible for a variety of reasons. Fortunately, you can also draw the image to a canvas and call the canvas' toBlob() method.

Thoughts?

Copy link
Owner

@extesy extesy Aug 1, 2021

Choose a reason for hiding this comment

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

imgDetails gets populated with image details (url, width, height) when it's zoomed. hz.hzImg.find('img').get(0).src is kinda the same thing but it tries to parse the details of the artificial hzImg node that the extension creates for the full image.

As for the article's phrase "requesting an image from the server may not be desirable or possible for a variety of reasons" - it does not apply in our case because we always load the full image from the server, that's kinda the point of this extension. You already do this for png files:

        if (isPng) {
            return fetch(img.src).then(res => res.blob())
        }

What happens if you remove "if (isPng)"? I haven't tried myself but I've seen images saved with incorrect extensions (png vs jpg) and browsers can still render them correctly even though the content doesn't match the extension.

Copy link
Contributor Author

@George-lewis George-lewis Aug 1, 2021

Choose a reason for hiding this comment

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

What happens if you remove "if (isPng)"?

Removing it results in trying to copy non-png images to the clipboard, which produces the error mentioned in my last comment. Unless there's something strange about my Chrome, you cannot write other image formats to the clipboard.

If it's not inconvenient for you I think you should try running this in your browser:

const url = "https://i.imgur.com/pg4bkej.jpeg";
fetch(url)
    .then(res => res.blob())
    .then(blob => new ClipboardItem({[blob.type]: blob}))
    .then(item => {
        setTimeout(() => {
            navigator.clipboard.write([item])
        }, 3000);
    });

You'll need to run it on a page that allows you to make the request to that origin (pasting into the console of a new tab should work). Make sure you click on the browser window before the timeout executes, otherwise clipboard access will be denied: DOMException: Document is not focused.

Let me know what happens for you. I get DOMException: Type image/jpeg not supported on write.

Edit: I see your point about mismatched file extensions. The thing about file names is that they're really more of a convention than anything else. Anyway, if you hardcode the mime type Chrome will complain: DOMException: Type image/png does not match the blob's type image/jpeg

Copy link
Owner

Choose a reason for hiding this comment

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

This worked for me:

const url = "https://i.imgur.com/pg4bkej.jpeg";
fetch(url)
    .then(res => res.blob())
    .then(blob => blob.arrayBuffer())
    .then(data => new ClipboardItem({["image/png"]: new Blob([data], {"type": "image/png"})}))
    .then(item => {
        setTimeout(() => {
            navigator.clipboard.write([item])
        }, 3000);
    });

Copy link
Contributor Author

@George-lewis George-lewis Aug 2, 2021

Choose a reason for hiding this comment

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

I've made a few discoveries:

  1. Despite documentation existing for clipboard.setImageData, it does not actually exist in FireFox as far as I can tell.
    • This is unfortunate because the documentation explicitly says it supports jpegs and pngs
  2. As you noted, ClipboardItem is disabled by default in FireFox, however users can opt to enable it via a configuration flag. A FireFox install with this flag set would pass our feature check, however would encounter an issue during copying, because FireFox doesn't accept our hacked blob. When trying to copy: DOMException: Unable to decode blob for 'image/png' as image. I'm guessing that Chrome tries to decode the blob, realizes it's a jpeg, and reencodes it or something, but FireFox notices the mismatch and stops.

I feel like that leaves us a few options:

  1. Do not officially support FireFox for this at all. Users can still enable the flag if they want, but copying non-png images will fail.
  2. Ask users to enable the flag if they want the feature, and go back to using canvas so that it works fully on all browsers

Copy link
Owner

Choose a reason for hiding this comment

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

Given the low total number of Firefox users, I think it's ok to not support it for now. Can you please also hide these options completely on Firefox instead of showing them but disabling functionality? Something like if (navigator.userAgent.toLowerCase().indexOf('firefox') > -1) should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the low total number of Firefox users

😄 Real jab at FireFox there. Especially considering they have lost 20% / 50 million users over the last year.

By the way, I don't see options.html file in this PR. How are you exposing the options?

The extension dynamically creates the HTML here: https://github.com/extesy/hoverzoom/blob/master/js/options.js#L32
It was sufficient to add the option name to the array, and then the text into the locale files.

Can you please also hide these options completely on Firefox

Sure. I'll take the easy way out here and add an explicit check to the code that generates the options page. I can also add an explicit check to the code that does the copying? Though it will partially work as-is

Copy link
Contributor Author

@George-lewis George-lewis Aug 3, 2021

Choose a reason for hiding this comment

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

Latest commit should do the trick. It uses navigator.userAgentData, which is still technically experimental, but is supported in all Chromium browsers so we're good-to-go. It was the best way I could find to check if a browser is Chromium-based.

Copy link
Owner

Choose a reason for hiding this comment

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

Looks almost perfect, just a comment about shortcuts processing.

_locales/ca/messages.json Outdated Show resolved Hide resolved
@extesy
Copy link
Owner

extesy commented Aug 3, 2021

By the way, I don't see options.html file in this PR. How are you exposing the options?

js/hoverzoom.js Outdated Show resolved Hide resolved
@George-lewis
Copy link
Contributor Author

George-lewis commented Aug 3, 2021

Before merging:

  • remove feature check
    • made redundant by chromium check
  • reorder check for options page

I'll do it later


Done

@extesy extesy merged commit 9bf7dd4 into extesy:master Aug 4, 2021
@extesy
Copy link
Owner

extesy commented Aug 4, 2021

Thank you for the contribution, @George-lewis!

@GrosPoulet
Copy link
Contributor

Hi @George-lewis, welcome aboard ⛵
I added a default action key for Image Lock feature (#746) created by @EhsanKia : L (76)
I choosed the letter L for Lock, but you are using the same letter for Copy Image URL action key.
=> do you mind if i change your action key to U (85) for URL ?

@George-lewis
Copy link
Contributor Author

George-lewis commented Aug 6, 2021

Hi @George-lewis, welcome aboard ⛵
I added a default action key for Image Lock feature (#746) created by @EhsanKia : L (76)
I choosed the letter L for Lock, but you are using the same letter for Copy Image URL action key.
=> do you mind if i change your action key to U (85) for URL ?

Thanks! Go right ahead. U sounds like a fine replacement

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.

3 participants