nusmodifications / nusmods

🏫 Official course planning platform for National University of Singapore.
https://nusmods.com
MIT License
578 stars 315 forks source link

Use document.execCommand("copy") instead of Flash #442

Closed rongjiecomputer closed 7 years ago

rongjiecomputer commented 7 years ago

All browser vendors (including Internet Explorer) will remove Flash completely by end of 2020.

Clipboard actions can be easily achieved with document.execCommand, but it might not work on ancient browsers. So the question is, how much nusmods cares about old browsers?

Reference:

ZhangYiJiang commented 7 years ago

Hi! Thanks for reporting this. We're aware of the issue. For reference, execCommand is actually fully supported by all browsers in NUSMod's current support matrix, so browser support is not the main problem.

The issue is that the current code does quite a bit more than just copying content to the clipboard - it also makes a request to a URL shortener to create a short URL. Unfortunately this has to be done synchronously due to security reasons, and browsers are also phasing out support for synchronous Ajax. That said, having tested the existing functionality a while back casually, the feature already appears broken on both Firefox and Chrome, so whatever changes we make now should be no worse than the existing solution - it's just a question of how best to salvage the feature.

rongjiecomputer commented 7 years ago

That said, having tested the existing functionality a while back casually, the feature already appears broken on both Firefox and Chrome

I assume you meant using sync Clipboard API in Promise?

w3c/selection-api#80 made Selection.addRange no-op when there is existing selection, so quite a lot of online demos stop working since Chrome 58 (https://www.chromestatus.com/features/6680566019653632).

<span class="copy" data-tooltip="Click to copy">user@example.com</span>
<script>
var copy = document.getElementsByClassName("copy");
var n = copy.length;

async function copyToClipboard(node) {
  var range = document.createRange();
  range.selectNode(node);
  window.getSelection().removeAllRanges(); // workaround for w3c/selection-api#80
  window.getSelection().addRange(range);
  try {
    var successful = document.execCommand('copy');
    var msg = successful ? 'successful' : 'unsuccessful';
    console.log('Copy email command was ' + msg);
  } catch(err) {
    console.log('Oops, unable to copy');
  }
}

for (var i = 0; i < n; i++) {
  copy[i].addEventListener("click", function(event) {
    copyToClipboard(this);
  });
}
</script>
ZhangYiJiang commented 7 years ago

No, I mean the existing Flash clipboard URL copy button on NUSMods. The behavior observed on testing is:

On Firefox, Flash is click-to-play, so it takes three clicks -

  1. First click activates Flash movie
  2. Second click starts to load short URL (nothing is copied to clipboard)
  3. If the URL is loaded, the third click successfully copies it to clipboard

On Chrome, the button doesn't work at all, no matter how many times you click the button. The synchronous Ajax is fired though, so the browser does freeze for about a second. Also, sometimes the movie crashes.

As such, the feature is pretty much completely broken. Its just a question of how much work we want to do to fix it, given the unusual constraints of having to make a network request to get the stuff that should go into the clipboard. We could make it a two click copy, like what you have to do in Firefox right now, and if you want to be fancy, trigger the load on hover to anticipate the user. Alternatively we could eager load the short URL by making a request on every change of the timetable, but that may put too much load on the shortener server, especially since the feature does not appear to be frequently used.

Anyway, I'm not entirely sure what you mean by the piece of code you have there. The function shouldn't need to be async since await is not used in it - in fact, I'm not sure if execCommand will work at all when it is async.

ZhangYiJiang commented 7 years ago

https://github.com/nusmodifications/nusmods/commit/9032fbb26d44ec28116e5322f80979006d931ef4 should fix this. I should have committed it to a branch, but I accidentally pushed to master instead, oh well.

Anyway, we're going with the two step copy solution here -

  1. User clicks on the text box, and the short URL is retrieved
  2. The copy button appears, allowing the user to copy the URL. This is mainly for the benefit of mobile users, since desktop users can just Ctrl/Cmd+C immediately

Can I get a quick 👍 from @yangshun or @li-kai (not sure who's familiar with the short URL / copy to clipboard feature)

yangshun commented 7 years ago

Actually we can stop development/maintenance on V2 entirely. V3 should launch by 2020. I'm fine with the approach as long as it works (:

li-kai commented 7 years ago

2020... 😢