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

web: Implement pasting text using Clipboard API #16692

Merged
merged 5 commits into from
Jun 18, 2024

Conversation

kjarosh
Copy link
Member

@kjarosh kjarosh commented Jun 12, 2024

Fixes #16678.

Ruffle does not have direct clipboard access on web, so the current paste implementation from the context menu does not work. This patch intercepts the paste context menu callback, and uses the Clipboard API to ask the browser for the clipboard and update it before calling the callback. When the Clipboard API is not available, a modal informing the user about cut, copy, paste shortcuts is displayed.

Firefox Chromium
Clipboard modal
A modal is displayed when the Clipboard API is not supported

@kjarosh kjarosh added the A-web Area: Web & Extensions label Jun 12, 2024
@torokati44
Copy link
Member

torokati44 commented Jun 12, 2024

As for the first commit, cf.: torokati44@df91395

I'm not saying that you should use this verbatim, but the comments therein might be worth considering.
EDIT: Welp, yeah... https://github.com/ruffle-rs/ruffle/actions/runs/9480734248/job/26122062635?pr=16692

@torokati44
Copy link
Member

A modal is displayed when the Clipboard API is not supported

Does a plain alert have too much of "bad, crufty, old web" vibe?

@kjarosh
Copy link
Member Author

kjarosh commented Jun 12, 2024

Does a plain alert have too much of "bad, crufty, old web" vibe?

TBH I created a modal for pure consistency with other UI elements (also, I took inspiration from other sites which show a similar message), but now that you mentioned, it does have a bit of that vibe, yes 😄

@torokati44
Copy link
Member

torokati44 commented Jun 12, 2024

TBH if there's no compelling technical reason against it, I'd still prefer it I think. This "plain black text on white background" thing, just to tell the user a short message, is not the nicest IMHO.
EDIT: But I can accept this too, if more people like it than not.

@kjarosh
Copy link
Member Author

kjarosh commented Jun 12, 2024

A version with alert:

Just let me know which one to include, I'll update this PR accordingly.

The downside of using alert is that it's fairly intrusive and freezes temporarily the whole page where Ruffle is embedded. It also does not support setting a custom title and HTML inside.

@Dinnerbone
Copy link
Contributor

I'm fairly against alert for something so verbose, personally

@kjarosh kjarosh added the waiting-on-review Waiting on review from a Ruffle team member label Jun 12, 2024
Copy link
Member

@n0samu n0samu left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! I definitely prefer a modal over an alert as well. One small thing though, we might want to show the modal if the user denies the clipboard permission so they know there's an alternative way to paste without giving the site that extra permission they might not be comfortable with.

@kjarosh
Copy link
Member Author

kjarosh commented Jun 12, 2024

Added a second version of the clipboard modal which is shown when user denies access to the clipboard:

IMO This modal is non-intrusive, as clicking anywhere closes it, so it's not gonna annoy the user, even if they deny access intentionally or miss-click, but may give useful information for some.

@torokati44
Copy link
Member

FWIW I have accepted the overruling of my preference toward alert.

@evilpie
Copy link
Collaborator

evilpie commented Jun 18, 2024

We could also add some text to the context menu when e.g. pasting fails, but that only gives us a limited amount of space.

As a follow up: It would be nice if all our modals would use color-scheme: light dark; so that dark mode users aren't flash banged.

kjarosh and others added 5 commits June 18, 2024 23:57
This reverts commit 5b7c48e.

Co-authored-by: TÖRÖK Attila <torokati44@gmail.com>
The custom RUSTFLAGS from CI overrides .cargo/config.toml.
The current implementation was synchronous only, so that when
an async callback was used, the menu was being disposed prematurely.
This patch ensures that in case of asynchronous callbacks,
the menu will be disposed after they finish.
This modal informs the user that they can use shortcuts
for copy, cut, and paste instead of using the context menu.
This modal is meant to be displayed when the browser
does not support reading the clipboard,
or the user denies permission to the clipboard.
Ruffle does not have direct clipboard access on web, so the current
paste implementation from the context menu does not work.
This patch intercepts the paste context menu callback,
and uses the Clipboard API to ask the browser for the clipboard
and update it before calling the callback.
When the Clipboard API is not available or the user denies
clipboard permission, a modal informing the user about
cut, copy, paste shortcuts is displayed.
@kjarosh kjarosh enabled auto-merge (rebase) June 18, 2024 21:58
@kjarosh kjarosh removed the waiting-on-review Waiting on review from a Ruffle team member label Jun 18, 2024
@kjarosh kjarosh merged commit b36a6b7 into ruffle-rs:master Jun 18, 2024
17 checks passed
@kjarosh kjarosh deleted the web-paste branch June 18, 2024 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web Area: Web & Extensions newsworthy
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

web: Cannot paste from context menu
6 participants