Use click event to determine modifier keys (#22988)

Previously, editor elements had to listen for mouse_up events to
determine when a click had completed. This meant that they only had
access to modifier keys that were pressed during the mouse_up event.

This led to some incorrect user experiences, such as executing a
ctrl+click if the user pressed ctrl after pressing the mouse button, but
before releasing it.

This change adds a click event handler to EditorElement, and adds a
modifier() method to the ClickEvent, which only includes the modifier
keys that were pressed during both mouse down and mouse up. The code for
handling link clicks has been moved into the click event handler, so
that it's only triggered when the non-multi-cursor modifier was held for
both the mouse down and mouse up events.

Closes #12752, #16074, #17892 (the latter two seem to be duplicates of
the former!)

Release Notes:

- Fixed a bug where pressing ctrl/cmd (or other modifiers) after mouse
down but before mouse up still triggered ctrl/cmd+click behavior (e.g.
"go to definition")

crates/editor/src/editor.rs

@@ -81,9 +81,9 @@ use gpui::{
     AsyncWindowContext, AvailableSpace, Bounds, ClipboardEntry, ClipboardItem, Context,
     DispatchPhase, ElementId, Entity, EntityInputHandler, EventEmitter, FocusHandle, FocusOutEvent,
     Focusable, FontId, FontWeight, Global, HighlightStyle, Hsla, InteractiveText, KeyContext,
-    MouseButton, PaintQuad, ParentElement, Pixels, Render, SharedString, Size, Styled, StyledText,
-    Subscription, Task, TextStyle, TextStyleRefinement, UTF16Selection, UnderlineStyle,
-    UniformListScrollHandle, WeakEntity, WeakFocusHandle, Window,
+    MouseButton, MouseDownEvent, PaintQuad, ParentElement, Pixels, Render, SharedString, Size,
+    Styled, StyledText, Subscription, Task, TextStyle, TextStyleRefinement, UTF16Selection,
+    UnderlineStyle, UniformListScrollHandle, WeakEntity, WeakFocusHandle, Window,
 };
 use highlight_matching_bracket::refresh_matching_bracket_highlights;
 use hover_popover::{hide_hover, HoverState};
@@ -681,6 +681,7 @@ pub struct Editor {
     leader_peer_id: Option<PeerId>,
     remote_id: Option<ViewId>,
     hover_state: HoverState,
+    pending_mouse_down: Option<Rc<RefCell<Option<MouseDownEvent>>>>,
     gutter_hovered: bool,
     hovered_link_state: Option<HoveredLinkState>,
     inline_completion_provider: Option<RegisteredInlineCompletionProvider>,
@@ -1375,6 +1376,7 @@ impl Editor {
             leader_peer_id: None,
             remote_id: None,
             hover_state: Default::default(),
+            pending_mouse_down: None,
             hovered_link_state: Default::default(),
             inline_completion_provider: None,
             active_inline_completion: None,

crates/editor/src/element.rs

@@ -736,6 +736,10 @@ impl EditorElement {
     fn mouse_up(
         editor: &mut Editor,
         event: &MouseUpEvent,
+        #[cfg_attr(
+            not(any(target_os = "linux", target_os = "freebsd")),
+            allow(unused_variables)
+        )]
         position_map: &PositionMap,
         text_hitbox: &Hitbox,
         window: &mut Window,
@@ -748,18 +752,7 @@ impl EditorElement {
             editor.select(SelectPhase::End, window, cx);
         }
 
-        let multi_cursor_setting = EditorSettings::get_global(cx).multi_cursor_modifier;
-        let multi_cursor_modifier = match multi_cursor_setting {
-            MultiCursorModifier::Alt => event.modifiers.secondary(),
-            MultiCursorModifier::CmdOrCtrl => event.modifiers.alt,
-        };
-
-        if !pending_nonempty_selections && multi_cursor_modifier && text_hitbox.is_hovered(window) {
-            let point = position_map.point_for_position(text_hitbox.bounds, event.position);
-            editor.handle_click_hovered_link(point, event.modifiers, window, cx);
-
-            cx.stop_propagation();
-        } else if end_selection && pending_nonempty_selections {
+        if end_selection && pending_nonempty_selections {
             cx.stop_propagation();
         } else if cfg!(any(target_os = "linux", target_os = "freebsd"))
             && event.button == MouseButton::Middle
@@ -791,6 +784,30 @@ impl EditorElement {
         }
     }
 
+    fn click(
+        editor: &mut Editor,
+        event: &ClickEvent,
+        position_map: &PositionMap,
+        text_hitbox: &Hitbox,
+        window: &mut Window,
+        cx: &mut Context<Editor>,
+    ) {
+        let pending_nonempty_selections = editor.has_pending_nonempty_selection();
+
+        let multi_cursor_setting = EditorSettings::get_global(cx).multi_cursor_modifier;
+        let multi_cursor_modifier = match multi_cursor_setting {
+            MultiCursorModifier::Alt => event.modifiers().secondary(),
+            MultiCursorModifier::CmdOrCtrl => event.modifiers().alt,
+        };
+
+        if !pending_nonempty_selections && multi_cursor_modifier && text_hitbox.is_hovered(window) {
+            let point = position_map.point_for_position(text_hitbox.bounds, event.up.position);
+            editor.handle_click_hovered_link(point, event.modifiers(), window, cx);
+
+            cx.stop_propagation();
+        }
+    }
+
     fn mouse_dragged(
         editor: &mut Editor,
         event: &MouseMoveEvent,
@@ -5305,6 +5322,13 @@ impl EditorElement {
                 if phase == DispatchPhase::Bubble {
                     match event.button {
                         MouseButton::Left => editor.update(cx, |editor, cx| {
+                            let pending_mouse_down = editor
+                                .pending_mouse_down
+                                .get_or_insert_with(Default::default)
+                                .clone();
+
+                            *pending_mouse_down.borrow_mut() = Some(event.clone());
+
                             Self::mouse_left_down(
                                 editor,
                                 event,
@@ -5356,6 +5380,43 @@ impl EditorElement {
                 }
             }
         });
+
+        window.on_mouse_event({
+            let editor = self.editor.clone();
+            let position_map = layout.position_map.clone();
+            let text_hitbox = layout.text_hitbox.clone();
+
+            let mut captured_mouse_down = None;
+
+            move |event: &MouseUpEvent, phase, window, cx| match phase {
+                // Clear the pending mouse down during the capture phase,
+                // so that it happens even if another event handler stops
+                // propagation.
+                DispatchPhase::Capture => editor.update(cx, |editor, _cx| {
+                    let pending_mouse_down = editor
+                        .pending_mouse_down
+                        .get_or_insert_with(Default::default)
+                        .clone();
+
+                    let mut pending_mouse_down = pending_mouse_down.borrow_mut();
+                    if pending_mouse_down.is_some() && text_hitbox.is_hovered(window) {
+                        captured_mouse_down = pending_mouse_down.take();
+                        window.refresh();
+                    }
+                }),
+                // Fire click handlers during the bubble phase.
+                DispatchPhase::Bubble => editor.update(cx, |editor, cx| {
+                    if let Some(mouse_down) = captured_mouse_down.take() {
+                        let event = ClickEvent {
+                            down: mouse_down,
+                            up: event.clone(),
+                        };
+                        Self::click(editor, &event, &position_map, &text_hitbox, window, cx);
+                    }
+                }),
+            }
+        });
+
         window.on_mouse_event({
             let position_map = layout.position_map.clone();
             let editor = self.editor.clone();

crates/gpui/src/interactive.rs

@@ -147,6 +147,20 @@ pub struct ClickEvent {
     pub up: MouseUpEvent,
 }
 
+impl ClickEvent {
+    /// Returns the modifiers that were held down during both the
+    /// mouse down and mouse up events
+    pub fn modifiers(&self) -> Modifiers {
+        Modifiers {
+            control: self.up.modifiers.control && self.down.modifiers.control,
+            alt: self.up.modifiers.alt && self.down.modifiers.alt,
+            shift: self.up.modifiers.shift && self.down.modifiers.shift,
+            platform: self.up.modifiers.platform && self.down.modifiers.platform,
+            function: self.up.modifiers.function && self.down.modifiers.function,
+        }
+    }
+}
+
 /// An enum representing the mouse button that was pressed.
 #[derive(Hash, PartialEq, Eq, Copy, Clone, Debug)]
 pub enum MouseButton {

crates/picker/src/picker.rs

@@ -604,7 +604,7 @@ impl<D: PickerDelegate> Picker<D> {
             .id(("item", ix))
             .cursor_pointer()
             .on_click(cx.listener(move |this, event: &ClickEvent, window, cx| {
-                this.handle_click(ix, event.down.modifiers.secondary(), window, cx)
+                this.handle_click(ix, event.modifiers().secondary(), window, cx)
             }))
             // As of this writing, GPUI intercepts `ctrl-[mouse-event]`s on macOS
             // and produces right mouse button events. This matches platforms norms

crates/project_panel/src/project_panel.rs

@@ -3707,7 +3707,7 @@ impl ProjectPanel {
                     }
                     cx.stop_propagation();
 
-                    if let Some(selection) = this.selection.filter(|_| event.down.modifiers.shift) {
+                    if let Some(selection) = this.selection.filter(|_| event.modifiers().shift) {
                         let current_selection = this.index_for_selection(selection);
                         let clicked_entry = SelectedEntry {
                             entry_id,
@@ -3741,7 +3741,7 @@ impl ProjectPanel {
                             this.selection = Some(clicked_entry);
                             this.marked_entries.insert(clicked_entry);
                         }
-                    } else if event.down.modifiers.secondary() {
+                    } else if event.modifiers().secondary() {
                         if event.down.click_count > 1 {
                             this.split_entry(entry_id, cx);
                         } else {
@@ -3752,7 +3752,7 @@ impl ProjectPanel {
                         }
                     } else if kind.is_dir() {
                         this.marked_entries.clear();
-                        if event.down.modifiers.alt {
+                        if event.modifiers().alt {
                             this.toggle_expand_all(entry_id, window, cx);
                         } else {
                             this.toggle_expanded(entry_id, window, cx);