Make “copy to clipboard” button Permissions API-aware #1901
Conversation
|
CI is exiting before the tests run... I am not sure why yet. Will check it out later this week. |
|
Looks like the |
|
Ah, so locally I had to ensure |
|
Sounds good! |
b9d90e6 to
429407b
Compare
|
I believe Selenium is failing because we are not accepting the clipboard permissions prompt that appears in Chromium-based browsers. This failure is helpful, as it has made me realize that we need to handle the case where the the user has not provided clipboard permissions to Gollum from their browser. I am a little bit confused, though, why my new test case passes and only the JS errors Capybara test fails. So I must still be missing something. |
That is quite odd. 😕 EDIT: sorry, the link I posted seems to be relevant only to a specif JS test API. |
429407b to
39898e5
Compare
|
Just making a note that I'll be expanding the scope of this PR to include a change we talked about in #1937:
|
63a4d78 to
ccd2215
Compare
|
@benjaminwil What's the current status of this PR? |
I was actually hoping to pick this up again and get it ready for review this weekend! So very timely of you to comment on it now. |
|
That's great. Thanks for the update! |
bff1349 to
cb93a2d
Compare
cb93a2d to
607dacd
Compare
|
@bartkamphorst Thanks for catching that error in Firefox. This opened a small can of worms for me, and I'm just adding this comment to explain that can of worms so (hopefully) no one has to waste time digging through the MDN, Firefox bug reports, and ClipboardJS GitHub issues the next time we touch this feature. It turns out that Firefox's support for the clipboard via the Permissions API is nonexistent, but we can still use I also tried transitioning to use the ClipboardJS, but I ran into a different issue that stems from ClipboardJS using Since |
607dacd to
038d2b0
Compare
I agree. Just fixed up the |
|
Will also add that I manually tested this in Chrome, Firefox, and Safari without issue. |
There was a problem hiding this comment.
Thanks @benjaminwil ! Sorry this was such a pain to resolve. I do wonder if we could do without an explicit check for userAgent == Firefox but see my remark about that in the review.
Could you explain how the button is now navigable with the keyboard? When I "tab through" the items, I go from the Search input, to Upload to Rename and then to some image in the page, but never to Overview, History, Edit or New or to any of the copy to clipboard buttons.
| if (navigator.permissions && navigator.permissions.query) { | ||
| return await navigator.permissions.query({name: "clipboard-write"}) | ||
| .then((permissionStatus) => permissionStatus.state === "denied") | ||
| .catch((error) => console.error(error)); |
There was a problem hiding this comment.
Should we notify the user of the error in the UI?
There was a problem hiding this comment.
My view would be that that's not necessary: if users are expecting copy/paste functionality on websites and not getting it, that won't be experienced as a problem with gollum specifically.
There was a problem hiding this comment.
I have no comments in addition to those of @bartkamphorst. Thanks for the improvement and the extensive tests, @benjaminwil!
|
Wow, it’s been so long since I started this and it appears I’ve bit off more than I can easily chew for one PR. I don’t want to block any releases. Perhaps I should close this and PR these fixes more one-at-a-time. |
|
Weirdly, keyboard navigation is behaving much differently in Chrome versus Firefox for all menu items. Maybe this is worth looking into separately. |
|
The fix for keyboard navigation could definitely be in a different PR, but otherwise it would be great to have the improved code buttons in the next release! And please don't worry about holding up that release: I had an unexpectedly very busy last four months or so, so I didn't have to work on the new release anyway, until very recently! |
I agree. And this behavior is the same on master (on Firefox) so it's not introduced by your changes. My only concern for this PR is whether we should be checking explicitly for whether |
Modern browsers implement a permissions API that disallows websites from writing to and reading the system clipboard without permission. To learn more see the MDN's article about this[1]. This commit provides a basic check for clipboard access, so we don't spam the browser or its console with errors accidentally. We also don't want to run any of this code if no codeblocks are present. So we can check for codeblocks, as well, and potentially exit early. While I was refactoring, I also found it cumbersome to use a JS event to find an element and its sibling element. So I've change the `event` argument of the `copyToClipboard` function to be the codeblock ID we generate, which lets us easily get the button for the codeblock as well as the codeblock itself without a big event object. [1]: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Interact_with_the_clipboard
The button is now positioned, styled, and animated using Primer utilties.
038d2b0 to
0165758
Compare
|
@dometto @bartkamphorst Thanks for your reviews so far. I've updated the PR description and title to reflect the reduced scope of functionality it provides. Let me know if you have further comments. |
|
Thanks for working on the new approach to clipboard permissions @benjaminwil! 🎆 Happy we can still release this with 6.0, which is now really, really close. :) |
See the code comment in the commit body. I was able to manually test that Firefox emits no errors and copies to clipboard from a Firefox profile with the default settings.
0165758 to
dff5f13
Compare
|
Thanks again for the reviews. I've pushed up changes and recompiled the static assets. Should be ready to go. 👍 |
Fixes #1885.
This pull request makes the "copy to clipboard" button on wiki pages usable with modern browsers that implement the Permissions API for the "clipboard-write" permission as it exists today. And it generally improves the "copy to clipboard" functionality of code blocks on wiki pages:
Users who don't use pointing devices can use the "copy to clipboard" using the keyboard only.Note: Before this pull request, the feature was not keyboard-accessible, and it still isn't after this pull request. There is discussion about this in the pull request comments.