angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.33k stars 6.73k forks source link

Clipboard code example in Material 9.0.0 docs does not work on Firefox #18449

Open amdw opened 4 years ago

amdw commented 4 years ago

Documentation Feedback

The following code example from the Clipboard documentation for Angular Material 9.0.0 does not work on Firefox:

https://github.com/angular/components/blame/a67cef6cf048166ebd1934637dbe0136c2dd345c/src/cdk/clipboard/clipboard.md#L43

The problem seems to be that Firefox does not allow Javascript applications to write to the clipboard except synchronously inside a click handler (unless special permissions have been granted):

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Interact_with_the_clipboard#Writing_to_the_clipboard

Therefore, the fact that the attempt function is called asynchronously using setTimeout causes the call to fail, with a console warning message saying the copy was "denied because it was not called from inside a short running user-generated event handler".

It's unclear to me why the example code is written with the copy attempts done asynchronously. Running the attempts inline seems to work fine, even for a fairly substantial amount of data:

let clipboardData = 'something to copy';
const pending = this.clipboard.beginCopy(clipboardData);
for (let attempt = 0; attempt < 3; attempt++) {
  const result = pending.copy();
  if (result) {
    console.log('Copied on attempt ' + (attempt+1));
    break;
  }
}
pending.destroy();

Affected documentation page: https://material.angular.io/cdk/clipboard/overview

amdw commented 4 years ago

I should also mention that I have not tried the template directives such as cdkCopyToClipboard.

If these are implemented like the code example internally, then they will presumably also not work on Firefox, in which case this would be a code bug rather than just a documentation issue.

crisbeto commented 4 years ago

The example seems to be working for me against Firefox 73.

amdw commented 4 years ago

I created a small application that successfully reproduces the problem on Firefox 72.0.2 (the current version in Fedora 31).

https://github.com/amdw/angular-clipboard-repro

Clicking the first button (which has a click handler that matches the documentation example code) fails to copy any text to the clipboard, and you get a warning message in the JavaScript console:

document.execCommand('cut'/'copy') was denied because it was not called from inside a short running user-generated event handler.

Clicking the button on the right works as expected: the text is copied to the clipboard and there is no warning in the console - just the "Copied on attempt" message, which I've yet to see display any attempt count other than 1.

amdw commented 4 years ago

Firefox 73.0 was just rolled out to Fedora Workstation 31, and I can confirm that the problem is still reproducible using my example app on that version.

jackwootton commented 4 years ago

Looks like this is happening in Mozilla Firefox 73.0.1 (64-bit), Ubuntu 19.10. It was reported by a user of www.jsonschema.net. See https://github.com/jackwootton/json-schema/issues/68

crisbeto commented 4 years ago

I'm able to reproduce it now as well. The weird thing is that it complains about setTimeout, but not for Promise.resolve().then.

amdw commented 4 years ago

Thanks for the pull request. However as @crisbeto noted there, moving the initial attempt call to be synchronous only helps in the (common) case where the copy succeeds on the first attempt.

If retries are required, they presumably have no chance of succeeding on Firefox, because they are still occurring via setTimeout.

In some ways this is worse than the original code, because it is a rarely-exercised and hard-to-test code path that is broken.

Please can this issue be reopened until it can be confirmed that either the first call can never fail on Firefox, or that there is no way to make retries work properly on Firefox.

If either of these is true then it may not be possible to improve the code further, but documentation should be added to explain the situation.

If neither of these is true, then the code is still broken, just in a much more subtle and hard-to-detect way than before.

mmalerba commented 4 years ago

@crisbeto is there anything more we can do here? or can we file an issue with firefox?

crisbeto commented 4 years ago

What I did was to align the code snippet with what we're doing in the CdkCopyToClipboard directive, but I don't know whether we can do much more since I don't have all the context around why the timeout is there to begin with (I left a note about it in the PR). I don't think it makes sense to open an issue for Firefox, because the behavior looks intentional.

amdw commented 4 years ago

Thank you. This is precisely the concern I raised on my second comment on this issue. Now that the documentation code example and the CdkCopyToClipboard directive are aligned, this is no longer purely a documentation issue: the directive code itself is in question as well.

I agree with what @crisbeto says - we need to understand why setTimeout was used in the first place. In other words, what's wrong with the simple for loop in my original comment on this issue?

Can we ask whoever wrote the directive code in the first place? It seems to have been introduced in the following commit, but this was authored by @crisbeto and committed by @mmalerba, so presumably one of you can explain why it was done that way? Am I misunderstanding something here?

https://github.com/angular/components/commit/a67cef6cf048166ebd1934637dbe0136c2dd345c

Appreciate your help!