sudodoki / copy-to-clipboard

Copy stuff into clipboard from your browser using JS
http://sudodoki.github.io/copy-to-clipboard/example/
MIT License
1.33k stars 130 forks source link

onClick() callback prevents text from being copied #98

Open MaffooBristol opened 4 years ago

MaffooBristol commented 4 years ago

MacOS Mojave 10.14.6 Tested on both Chrome 81 and Safari 13 copy-to-clipboard 3.3.1

This seems like a pretty fundamentally critical bug.

When I use the function on a button's onClick event, it works fine. However when I specify an onCopy() too, it just doesn't copy anything. It doesn't matter what's inside the callback function, or whether it's an arrow function, or whether the function is defined outside of the handler, or anything else really.

Works:

<button
  onClick={() => {
    copy(Math.random(), {});
  }}
>
  Copy text
</button>

Doesn't work:

<button
  onClick={() => {
    copy(Math.random(), {
      onCopy: () => {},
    });
  }}
>
  Copy text
</button>
MaffooBristol commented 4 years ago

Additionally, I think it would be better if the function returned a promise

sudodoki commented 4 years ago

wonder whether that's because of preventDefault in onCopy branching logic https://github.com/sudodoki/copy-to-clipboard/blob/master/index.js#L67 I will try to look into it later, but first thought is that this is odd

sudodoki commented 4 years ago

@MaffooBristol

Additionally, I think it would be better if the function returned a promise

we had callback support at some point, but that wasn't too useful given we are doing sync stuff all the time (prompt is blocking as well). Given a newer api that is promise based, we had some initial discussion on the level 'well, it would be cool to support it and the probably wrapping everying into promise interface', but no work was done as nobody had time for it

MaffooBristol commented 4 years ago

Cheers for looking into it. For now I'll just make the assumption that the copying was successful but it's not ideal going forwards

And re: the promise, it's not a deal breaker, just a nice to have 😄

ronaldgj commented 4 years ago

Same problem with me! When I remove the e.preventDefault() it works!

mike652638 commented 4 years ago

Same problem here, tested with Firefox Browser Developer Edition 79.0b9(x64) ! When I remove/comment the e.preventDefault() line from index.js#L67, it works as expected then. This seems to be a bug.

BTW, why do we need the e.preventDefault() line please ? Can this line be safely removed ? @sudodoki

sudodoki commented 4 years ago

if you have onCopy handler that might be super useful to change format / etc. you don't want default handler to actually fire. The argument passed to onCopy can be used to actually do setData(format, data). By default copyToClipboard is sync, so no need for 'callback'.

So basically, providing onCopy you are opting-out of default copying mechanism of your browser and do all the work. If you only need this to provide format, consider using format option