-
Notifications
You must be signed in to change notification settings - Fork 247
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
Conversation
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 |
9e85a90 tries to implement |
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", "--"] |
There was a problem hiding this comment.
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
I tested my PR with the exampe I'm removing the "draft" mode, I think that's a fair attempt at adding that feature, I have a few weaknesses to point:
|
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(to be clear, this is not a but introduced in this PR)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- ctrl/cmd detection: this is how eframe does it, it checks the web_sys events for each keys... : https://github.com/emilk/egui/blob/307565efa55158cfa6b82d2e8fdc4c4914b954ed/crates/eframe/src/web/input.rs#LL211C25-L211C25
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:
Lines 104 to 110 in c51fc56
let mac_cmd = if cfg!(target_os = "macos") { | |
win | |
} else { | |
false | |
}; | |
let command = if cfg!(target_os = "macos") { win } else { ctrl }; | |
Is it ok for you?
@@ -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>, |
There was a problem hiding this comment.
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.
src/web_clipboard.rs
Outdated
info!("Not implemented."); | ||
} | ||
} | ||
info!("{:?}", event.clipboard_data()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
After more testing, I believe in wasm+mac, 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 |
also, I created an issue to winit to follow this prevent_default behaviour: rust-windowing/winit#2831 |
Any updates on this? |
Blockers listed on the description are still relevant ; I'm working on bevyengine/bevy#10702. |
now that the winit update is merged into bevy can this go forward? |
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 |
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 hasbevy_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:
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:
|
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 |
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). |
@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. |
Great thanks for championing it to mergeability ! I'll let you close this PR at your will. |
@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. |
Fixes #113