Fix Mac text selection keyboard shortcuts in CEF text fields#3928
Fix Mac text selection keyboard shortcuts in CEF text fields#3928Mayank7Bisnewar wants to merge 1 commit intoGraphiteEditor:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug that prevented standard macOS text selection keyboard shortcuts from functioning correctly within CEF text fields. The solution involved a multi-faceted approach: removing a problematic workaround that blocked shortcut registration, implementing a mechanism to prevent an infinite event loop between CEF and Winit, and enhancing key mapping to correctly interpret complex multi-modifier shortcuts. These changes collectively restore expected text field behavior for Mac users. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a clever fix for macOS keyboard shortcuts in CEF text fields by using a thread-local flag to prevent infinite event loops. The changes are well-contained and the explanation is clear. I've suggested one improvement for panic safety using an RAII guard, which is a common Rust pattern for ensuring resource cleanup.
| #[cfg(target_os = "macos")] | ||
| crate::window::mac::app::IS_CEF_WORK.with(|c| c.set(true)); | ||
|
|
||
| cef::do_message_loop_work(); | ||
|
|
||
| #[cfg(target_os = "macos")] | ||
| crate::window::mac::app::IS_CEF_WORK.with(|c| c.set(false)); |
There was a problem hiding this comment.
To make this more robust against potential panics within cef::do_message_loop_work(), it's better to use an RAII guard to manage the IS_CEF_WORK flag. This ensures the flag is always reset to false, even if an unwind occurs, preventing the application from getting into a state where all keyboard events are ignored.
#[cfg(target_os = "macos")]
let _guard = {
struct CefWorkGuard;
impl Drop for CefWorkGuard {
fn drop(&mut self) {
crate::window::mac::app::IS_CEF_WORK.with(|c| c.set(false));
}
}
crate::window::mac::app::IS_CEF_WORK.with(|c| c.set(true));
CefWorkGuard
};
cef::do_message_loop_work();![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="desktop/src/cef/input/keymap.rs">
<violation number="1" location="desktop/src/cef/input/keymap.rs:15">
P1: Mac-specific navigation Unicode mappings are applied globally, causing non-mac key events to become character-bearing/CHAR events and risking cross-platform input regressions.</violation>
</file>
<file name="desktop/src/cef/input.rs">
<violation number="1" location="desktop/src/cef/input.rs:112">
P2: Mitigation comment is stale and now contradicts implementation, increasing regression risk in fragile macOS keyboard handling.</violation>
</file>
<file name="desktop/src/window/mac/app.rs">
<violation number="1" location="desktop/src/window/mac/app.rs:17">
P1: The new `IS_CEF_WORK` early return in `send_event` drops all NSEvents during CEF loop work, not just the intended synthesized key events.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| NamedKey::Enter => '\r', | ||
| NamedKey::Backspace => '\x08', | ||
| NamedKey::Escape => '\x1b', | ||
| NamedKey::ArrowUp => '\u{F700}', |
There was a problem hiding this comment.
P1: Mac-specific navigation Unicode mappings are applied globally, causing non-mac key events to become character-bearing/CHAR events and risking cross-platform input regressions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/src/cef/input/keymap.rs, line 15:
<comment>Mac-specific navigation Unicode mappings are applied globally, causing non-mac key events to become character-bearing/CHAR events and risking cross-platform input regressions.</comment>
<file context>
@@ -12,6 +12,15 @@ impl ToCharRepresentation for Key {
NamedKey::Enter => '\r',
NamedKey::Backspace => '\x08',
NamedKey::Escape => '\x1b',
+ NamedKey::ArrowUp => '\u{F700}',
+ NamedKey::ArrowDown => '\u{F701}',
+ NamedKey::ArrowLeft => '\u{F702}',
</file context>
| if IS_CEF_WORK.with(|c| c.get()) { | ||
| // CEF synthesized an NSEvent for an unhandled key press during message loop work. | ||
| // Drop it to avoid duplicate menu triggers and infinite keyboard event loops with winit. | ||
| return; | ||
| } |
There was a problem hiding this comment.
P1: The new IS_CEF_WORK early return in send_event drops all NSEvents during CEF loop work, not just the intended synthesized key events.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/src/window/mac/app.rs, line 17:
<comment>The new `IS_CEF_WORK` early return in `send_event` drops all NSEvents during CEF loop work, not just the intended synthesized key events.</comment>
<file context>
@@ -10,6 +14,12 @@ define_class!(
impl GraphiteApplication {
#[unsafe(method(sendEvent:))]
fn send_event(&self, event: &NSEvent) {
+ if IS_CEF_WORK.with(|c| c.get()) {
+ // CEF synthesized an NSEvent for an unhandled key press during message loop work.
+ // Drop it to avoid duplicate menu triggers and infinite keyboard event loops with winit.
</file context>
| if IS_CEF_WORK.with(|c| c.get()) { | |
| // CEF synthesized an NSEvent for an unhandled key press during message loop work. | |
| // Drop it to avoid duplicate menu triggers and infinite keyboard event loops with winit. | |
| return; | |
| } | |
| if IS_CEF_WORK.with(|c| c.get()) && event.r#type() == NSEventType::KeyDown { | |
| // CEF synthesized an NSEvent for an unhandled key press during message loop work. | |
| // Drop only key events to avoid suppressing unrelated app/window input. | |
| return; | |
| } |
| { | ||
| key_event.unmodified_character = event.key_without_modifiers.to_char_representation() as u16; | ||
| } | ||
| key_event.unmodified_character = event.key_without_modifiers.to_char_representation() as u16; |
There was a problem hiding this comment.
P2: Mitigation comment is stale and now contradicts implementation, increasing regression risk in fragile macOS keyboard handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/src/cef/input.rs, line 112:
<comment>Mitigation comment is stale and now contradicts implementation, increasing regression risk in fragile macOS keyboard handling.</comment>
<file context>
@@ -109,10 +109,7 @@ pub(crate) fn handle_window_event(browser: &Browser, input_state: &mut InputStat
- {
- key_event.unmodified_character = event.key_without_modifiers.to_char_representation() as u16;
- }
+ key_event.unmodified_character = event.key_without_modifiers.to_char_representation() as u16;
#[cfg(target_os = "macos")] // See https://www.magpcss.org/ceforum/viewtopic.php?start=10&t=11650
</file context>
|
Clearly not tested (well), doesn't compile: And this will lead to the menu bar lighting up when pressing Also linked the wrong issue. |
2 participants
Fixes #3507
This fixes the issue where standard macOS keyboard selection shortcuts (like
Cmd+AandShift+Option+Up) weren't working properly inside CEF text fields on desktop.While looking into this, I found that the
unmodified_character = 0workaround in input.rs was preventing CEF from properly registering those text shortcuts on Mac. However, removing it caused an infinite loop where CEF would push unhandled syntheticNSEventsback to app.rs which routed them straight back into Winit (causing duplicate menu events and hanging).To fix the whole flow:
unmodified_character = 0mitigation in input.rs so Cmd+A registers.IS_CEF_WORKthread-local flag in app.rs and singlethreaded.rs to detect and safely drop those synthetic CEF events before they loop back into Winit.0xF700) for the navigational named keys in keymap.rs so Chromium's nativeinterpretKeyEvents:can correctly process complex multi-modifier shortcuts likeShift+Option+Up.Tested locally and everything works just like it does on the web version now!
(Note: I tracked down the CEF/Winit infinite loop architecture and developed this fix with the help of an AI coding assistant.)