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 clipboard handling #178

Closed
wants to merge 19 commits into from
Closed

web clipboard handling #178

wants to merge 19 commits into from

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented Apr 18, 2023

Fixes #113

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Apr 22, 2023

Because the clipboard permissions are not implemented by all browsers, this is not an ideal...

More info: https://developer.mozilla.org/en-US/docs/Web/API/Permissions_API#browser_compatibility

On google chrome, as noted "supported on above link, I correctly got the permission request and my minimal playground for copy is working (shown in console) 🎉.

Buut having a permission request doesn't seem ideal, I feel like it's the wrong tool for the job ? I'm not sure about the exact process...

For me a clipboard paste event should be handled by the browser, and forward characters to the canvas, winit, bevy, bevy_egui, egui....

I'm guessing prevent_defaults() is somehow in the way ? Any insight is welcome.

examples/clipboard.rs Outdated Show resolved Hide resolved
examples/clipboard.rs Outdated Show resolved Hide resolved
examples/clipboard.rs Outdated Show resolved Hide resolved
@Vrixyz
Copy link
Contributor Author

Vrixyz commented May 7, 2023

9e85a90 tries to implement eframe's way of using web-sys API to paste (following #113 (comment)) ; but that doesn't work 😢

@Vrixyz
Copy link
Contributor Author

Vrixyz commented May 16, 2023

current state as of 69c447d ; there is a weird bug causing the data pasted to be doubled, not sure why.

paste_progress_bevy_egui.mp4

@@ -0,0 +1,13 @@
[alias]
run-wasm = ["run", "--release", "--package", "run-wasm", "--"]
Copy link
Contributor Author

@Vrixyz Vrixyz May 22, 2023

Choose a reason for hiding this comment

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

I think cargo run-wasm is quite handy to easily test a wasm version ; I can make another PR if interested

@Vrixyz
Copy link
Contributor Author

Vrixyz commented May 22, 2023

I tested my PR with the exampe ui on wasm and could copy paste from and to another application 🎉!

I'm removing the "draft" mode, I think that's a fair attempt at adding that feature, I have a few weaknesses to point:

  • should we remove completely the fully managed local "string" clipboard ? in wasm non web my implementation would fail 💀
    • I guess we could make some kind of decoration API with a bevy_egui_extra adding clipboard_web features...
  • eframe uses another egui API to support Paste, it's probably more correct because it's from official egui ; I didn't go this route because I fear other obstacles, I'd like to get a minimal clipboard support before imporving on top of that.
  • a bit unwrap-heavy, and not reading channel send events, maybe other clippy warnings

@Vrixyz Vrixyz marked this pull request as ready for review May 22, 2023 14:15
@Vrixyz Vrixyz changed the title WIP: clipboard handling web clipboard handling May 22, 2023
@Vrixyz Vrixyz requested a review from paul-hansen May 22, 2023 14:17
src/systems.rs Outdated
if let Some(contents) = input_resources.egui_clipboard.get_contents() {
focused_input.events.push(egui::Event::Text(contents))
{
if command && pressed {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚠️ on wasm mac, this is wrong, because it maps to ctrl key, but it should map to command key...

(to be clear, this is not a but introduced in this PR)

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, good catch. This makes me wonder how we can detect running on MacOS in wasm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eframe uses events for all those, deferring the responsibility to the browser: https://github.com/emilk/egui/blob/307565efa55158cfa6b82d2e8fdc4c4914b954ed/crates/eframe/src/web/events.rs#L184

Copy link
Owner

@vladbat00 vladbat00 May 28, 2023

Choose a reason for hiding this comment

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

It feels like we should adopt the same approach. Let's keep the whole block enabled only for non-wasm targets, whereas for wasm we'll just read from channels (which in turn will get messages on web_sys events).

Copy link
Owner

Choose a reason for hiding this comment

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

I think this is important to fix, as atm on MacOS, users need to do Ctrl+C to copy and Cmd+V to paste. I'd also be happy if we added the Select All (Ctrl/Cmd+A) support for WASM, to avoid the same inconsistency issue. That's extending the scope of this PR a bit, but hotkey consistency feels important here.

Copy link
Contributor Author

@Vrixyz Vrixyz May 31, 2023

Choose a reason for hiding this comment

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

I feel like it means we would bypass winit/bevy key inputs in wasm :/ could this possibly fixed upstream 🤔 ?

for the record, ctrl-A works (kinda) on wasm; on azerty it's mapped to ctrl-Q though, and on mac to ctrl where we'd want it to be cmd.

In all hindsight, I'd suggest a simpler approach where we detect if we run on mac or not through https://docs.rs/web-sys/latest/web_sys/struct.Navigator.html#method.platform, and adapt our meta key from there in case of cfg!(target_os = "wasm32") to consolidate following code: https://github.com/mvlabat/bevy_egui/blob/c51fc5640e2e431fa2169c98e6d252914c614904/src/systems.rs#L104-L110

Is it ok for you?

src/lib.rs Outdated Show resolved Hide resolved
src/web_clipboard.rs Outdated Show resolved Hide resolved
src/web_clipboard.rs Outdated Show resolved Hide resolved
src/web_clipboard.rs Outdated Show resolved Hide resolved
src/web_clipboard.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@@ -53,8 +53,7 @@ impl<'w, 's> InputEvents<'w, 's> {
#[allow(missing_docs)]
#[derive(SystemParam)]
pub struct InputResources<'w, 's> {
#[cfg(feature = "manage_clipboard")]
pub egui_clipboard: Res<'w, crate::EguiClipboard>,
pub egui_clipboard: ResMut<'w, crate::EguiClipboard>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #178 (comment) ; to be noted we can have mut only for wasm if we want.

Cargo.toml Outdated Show resolved Hide resolved
info!("Not implemented.");
}
}
info!("{:?}", event.clipboard_data())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

are these logs useful ? I suspect they would be noisy for end user.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think all the infos! are maybe a bit too much

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed most and/or changed for errors/warn

@Vrixyz
Copy link
Contributor Author

Vrixyz commented May 31, 2023

After more testing, I believe in wasm+mac, LWin and RWin is not properly forwarded from winit to bevy (to bevy_egui), I hope the winit keyboard input refactoring would fix it but I didn't look into it.

This results in cmd-A being unusable :( ; I can re-enable ctrl-A to avoid losing functionality, but as you said that creates a difference with cut/copy.

In my opinion, as a mac user, using ctrl feels very wrong, so I'd advocate to merge this PR with a fallback to ctrl-A while waiting or working on a fix upstream.

@Vrixyz
Copy link
Contributor Author

Vrixyz commented May 31, 2023

also, prevent_default=false (which I had to do to get copy/paste events) has other implications: cmd+left arrow triggers a "previous page", which eframe suppress via a prevent_default=true in a few other specific cases too.

I created an issue to winit to follow this prevent_default behaviour: rust-windowing/winit#2831

Cargo.toml Show resolved Hide resolved
@ConnorBP
Copy link

Any updates on this?

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Nov 18, 2023

Any updates on this?

Blockers listed on the description are still relevant ; I'm working on bevyengine/bevy#10702.

@extrawurst
Copy link

now that the winit update is merged into bevy can this go forward?

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Jan 5, 2024

I believe so ; I'm still coordinating the follow up of winit update at bevyengine/bevy#11052 ; so I'm happy to let this adopted by someone else wants to ; otherwise I'll pick it up again in a.. few month hopefully

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Jan 13, 2024

My understanding has improved a bit.

My observations are base on an old version on bevy_egui, in order to be consistent with the state of this PR. this PR will need to be rebased anyway, and re-evaluated with winit changes. My intuition tells me that it should not too hard to update that PR.

What problems this PR has

bevy_egui sends physical keys to egui, which is a problem on non qwerty keyboards, especially for "azerty select-all":

I thought the reason I wasn't receiving input from cmd-a was a bug from winit, but it was a wrong observation:

  • as bevy_egui sends the physical keys, physical key "A" will be sent when I hit the "Q" key, so to trigger select-all, I should do cmd-Q. but cmd-Q in mac is used to close an application. I remapped that to nothing because I don't want to hit that by mistake 😅 .
  • Theoretically it should work, but winit doesn't send any event when typing cmd-Q. This might be a bug from upstream or the browser, but it's very niche as the correct use case should be to input cmd-A.

I think the correct approach is to fix bevy_egui to send the logical key rather than the physical key.

The winit update follow up (bevyengine/bevy#11052) will help with detecting the correct "Logical key" with the task list "Add more info in KeyboardInput".

Given current state of bevy_egui, I'm thinking this does not create more inconsistencies in the API than currently the case on main, that being said, at this point I'm ok with ironing out the last bits before merging 👍

What would help greatly, for this PR and also future improvements, would be:

  • a list of pertinent tests to do
  • and better yet, automate them.

@v-kat
Copy link
Contributor

v-kat commented Jan 30, 2024

Could you move forward with #236 I'd love to have this feature when 0.13 comes out. Thanks for all the winit work!

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Feb 3, 2024

Could you move forward with #236 I'd love to have this feature when 0.13 comes out. Thanks for all the winit work!

I would love to.

I'm starting to work on it: Vrixyz#3

As mentioned in #178 (comment) ; we still have the problem of bevy_egui using physical keys rather than logical keys on bevy main.

Those problems might not actually apply to bevy 0.12, and I now believe we could have merged that PR for bevy 0.12 if tested more thoroughly, but now I'll focus on making that working for bevy 0.13.

I did a talk on that subject for the curious, which might give more context: https://www.youtube.com/live/i3wF71XJ-24?si=zfFV8xoY9JuQKmOt&t=3090

@v-kat
Copy link
Contributor

v-kat commented Mar 17, 2024

I might do a PR with a cleaned version of https://github.com/v-kat/bevy_egui/tree/wasm_mobile_keyboard it also seems to have some overlap but I dunno if it covers everything (I'm mainly doing a hacky fix for the mobile web keyboard and could try to rip out some of the copy paste code it has).

@vladbat00 vladbat00 mentioned this pull request Mar 17, 2024
@vladbat00
Copy link
Owner

@Vrixyz tremendous work! I opened another PR where I added some refactoring on top and the ability to unsubscribe from events. I'll leave it for review till the evening and merge+release it later today.

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Mar 18, 2024

Great thanks for championing it to mergeability ! I'll let you close this PR at your will.

@vladbat00
Copy link
Owner

@v-kat please do open a PR! I've just merged this one, so you can dissect the mobile web keyboard changes from the clipboard ones.

@vladbat00 vladbat00 closed this Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

copy to clipboard in wasm app
6 participants