Improve code context menu layout position esp visual stability (#22102)

* Now decides whether the menu is above or below the target position
before rendering it. This causes its position to no longer vary
depending on the length of completions

* When the text area is height constrained (< 12) lines, now chooses the
side which has the most space. Before it would always display above if
height constrained below.

* Misc code cleanups

Release Notes:

- Improved completions menu layout to be more stable and use available
space better.

crates/editor/src/code_context_menus.rs

@@ -4,8 +4,8 @@ use std::{cell::Cell, cmp::Reverse, ops::Range, rc::Rc};
 use fuzzy::{StringMatch, StringMatchCandidate};
 use gpui::{
     div, px, uniform_list, AnyElement, BackgroundExecutor, Div, FontWeight, ListSizingBehavior,
-    Model, Pixels, ScrollStrategy, SharedString, StrikethroughStyle, StyledText,
+    Model, ScrollStrategy, SharedString, StrikethroughStyle, StyledText, UniformListScrollHandle,
-    UniformListScrollHandle, ViewContext, WeakView,
+    ViewContext, WeakView,
 };
 use language::Buffer;
 use language::{CodeLabel, Documentation};
@@ -106,22 +106,24 @@ impl CodeContextMenu {
         }
     }
 
+    pub fn origin(&self, cursor_position: DisplayPoint) -> ContextMenuOrigin {
+        match self {
+            CodeContextMenu::Completions(menu) => menu.origin(cursor_position),
+            CodeContextMenu::CodeActions(menu) => menu.origin(cursor_position),
+        }
+    }
     pub fn render(
         &self,
-        cursor_position: DisplayPoint,
         style: &EditorStyle,
-        max_height: Pixels,
+        max_height_in_lines: u32,
         workspace: Option<WeakView<Workspace>>,
         cx: &mut ViewContext<Editor>,
-    ) -> (ContextMenuOrigin, AnyElement) {
+    ) -> AnyElement {
         match self {
-            CodeContextMenu::Completions(menu) => (
+            CodeContextMenu::Completions(menu) => {
-                ContextMenuOrigin::EditorPoint(cursor_position),
+                menu.render(style, max_height_in_lines, workspace, cx)
-                menu.render(style, max_height, workspace, cx),
-            ),
-            CodeContextMenu::CodeActions(menu) => {
-                menu.render(cursor_position, style, max_height, cx)
             }
+            CodeContextMenu::CodeActions(menu) => menu.render(style, max_height_in_lines, cx),
         }
     }
 }
@@ -322,13 +324,19 @@ impl CompletionsMenu {
         !self.matches.is_empty()
     }
 
+    fn origin(&self, cursor_position: DisplayPoint) -> ContextMenuOrigin {
+        ContextMenuOrigin::EditorPoint(cursor_position)
+    }
+
     fn render(
         &self,
         style: &EditorStyle,
-        max_height: Pixels,
+        max_height_in_lines: u32,
         workspace: Option<WeakView<Workspace>>,
         cx: &mut ViewContext<Editor>,
     ) -> AnyElement {
+        let max_height = max_height_in_lines as f32 * cx.line_height();
+
         let completions = self.completions.borrow_mut();
         let show_completion_documentation = self.show_completion_documentation;
         let widest_completion_ix = self
@@ -496,7 +504,7 @@ impl CompletionsMenu {
             },
         )
         .occlude()
-        .max_h(max_height)
+        .max_h(max_height_in_lines as f32 * cx.line_height())
         .track_scroll(self.scroll_handle.clone())
         .with_width_from_item(widest_completion_ix)
         .with_sizing_behavior(ListSizingBehavior::Infer);
@@ -779,13 +787,20 @@ impl CodeActionsMenu {
         !self.actions.is_empty()
     }
 
+    fn origin(&self, cursor_position: DisplayPoint) -> ContextMenuOrigin {
+        if let Some(row) = self.deployed_from_indicator {
+            ContextMenuOrigin::GutterIndicator(row)
+        } else {
+            ContextMenuOrigin::EditorPoint(cursor_position)
+        }
+    }
+
     fn render(
         &self,
-        cursor_position: DisplayPoint,
         _style: &EditorStyle,
-        max_height: Pixels,
+        max_height_in_lines: u32,
         cx: &mut ViewContext<Editor>,
-    ) -> (ContextMenuOrigin, AnyElement) {
+    ) -> AnyElement {
         let actions = self.actions.clone();
         let selected_item = self.selected_item;
         let list = uniform_list(
@@ -857,7 +872,7 @@ impl CodeActionsMenu {
             },
         )
         .occlude()
-        .max_h(max_height)
+        .max_h(max_height_in_lines as f32 * cx.line_height())
         .track_scroll(self.scroll_handle.clone())
         .with_width_from_item(
             self.actions
@@ -873,14 +888,6 @@ impl CodeActionsMenu {
         )
         .with_sizing_behavior(ListSizingBehavior::Infer);
 
-        let element = Popover::new().child(list).into_any_element();
+        Popover::new().child(list).into_any_element()
-
-        let cursor_position = if let Some(row) = self.deployed_from_indicator {
-            ContextMenuOrigin::GutterIndicator(row)
-        } else {
-            ContextMenuOrigin::EditorPoint(cursor_position)
-        };
-
-        (cursor_position, element)
     }
 }

crates/editor/src/editor.rs

@@ -1382,18 +1382,16 @@ impl Editor {
         if self.pending_rename.is_some() {
             key_context.add("renaming");
         }
-        if self.context_menu_visible() {
+        match self.context_menu.borrow().as_ref() {
-            match self.context_menu.borrow().as_ref() {
+            Some(CodeContextMenu::Completions(_)) => {
-                Some(CodeContextMenu::Completions(_)) => {
+                key_context.add("menu");
-                    key_context.add("menu");
+                key_context.add("showing_completions")
-                    key_context.add("showing_completions")
-                }
-                Some(CodeContextMenu::CodeActions(_)) => {
-                    key_context.add("menu");
-                    key_context.add("showing_code_actions")
-                }
-                None => {}
             }
+            Some(CodeContextMenu::CodeActions(_)) => {
+                key_context.add("menu");
+                key_context.add("showing_code_actions")
+            }
+            None => {}
         }
 
         // Disable vim contexts when a sub-editor (e.g. rename/inline assistant) is focused.
@@ -4999,6 +4997,7 @@ impl Editor {
             }))
     }
 
+    #[cfg(feature = "test-support")]
     pub fn context_menu_visible(&self) -> bool {
         self.context_menu
             .borrow()
@@ -5006,21 +5005,30 @@ impl Editor {
             .map_or(false, |menu| menu.visible())
     }
 
+    fn context_menu_origin(&self, cursor_position: DisplayPoint) -> Option<ContextMenuOrigin> {
+        self.context_menu
+            .borrow()
+            .as_ref()
+            .map(|menu| menu.origin(cursor_position))
+    }
+
     fn render_context_menu(
         &self,
-        cursor_position: DisplayPoint,
         style: &EditorStyle,
-        max_height: Pixels,
+        max_height_in_lines: u32,
         cx: &mut ViewContext<Editor>,
-    ) -> Option<(ContextMenuOrigin, AnyElement)> {
+    ) -> Option<AnyElement> {
-        self.context_menu.borrow().as_ref().map(|menu| {
+        self.context_menu.borrow().as_ref().and_then(|menu| {
-            menu.render(
+            if menu.visible() {
-                cursor_position,
+                Some(menu.render(
-                style,
+                    style,
-                max_height,
+                    max_height_in_lines,
-                self.workspace.as_ref().map(|(w, _)| w.clone()),
+                    self.workspace.as_ref().map(|(w, _)| w.clone()),
-                cx,
+                    cx,
-            )
+                ))
+            } else {
+                None
+            }
         })
     }
 

crates/editor/src/element.rs

@@ -70,8 +70,8 @@ use std::{
 };
 use sum_tree::Bias;
 use theme::{ActiveTheme, Appearance, PlayerColor};
-use ui::prelude::*;
 use ui::{h_flex, ButtonLike, ButtonStyle, ContextMenu, Tooltip};
+use ui::{prelude::*, POPOVER_Y_PADDING};
 use unicode_segmentation::UnicodeSegmentation;
 use util::RangeExt;
 use util::ResultExt;
@@ -2771,7 +2771,6 @@ impl EditorElement {
     fn layout_context_menu(
         &self,
         line_height: Pixels,
-        hitbox: &Hitbox,
         text_hitbox: &Hitbox,
         content_origin: gpui::Point<Pixels>,
         start_row: DisplayRow,
@@ -2780,56 +2779,98 @@ impl EditorElement {
         newest_selection_head: DisplayPoint,
         gutter_overshoot: Pixels,
         cx: &mut WindowContext,
-    ) -> bool {
+    ) {
-        let max_height = cmp::min(
+        let Some(context_menu_origin) = self
-            12. * line_height,
+            .editor
-            cmp::max(3. * line_height, (hitbox.size.height - line_height) / 2.),
+            .read(cx)
-        );
+            .context_menu_origin(newest_selection_head)
-        let Some((position, mut context_menu)) = self.editor.update(cx, |editor, cx| {
+        else {
-            if editor.context_menu_visible() {
+            return;
-                editor.render_context_menu(newest_selection_head, &self.style, max_height, cx)
-            } else {
-                None
-            }
-        }) else {
-            return false;
         };
-
+        let target_offset = match context_menu_origin {
-        let context_menu_size = context_menu.layout_as_root(AvailableSpace::min_size(), cx);
+            crate::ContextMenuOrigin::EditorPoint(display_point) => {
-
+                let cursor_row_layout =
-        let (x, y) = match position {
+                    &line_layouts[display_point.row().minus(start_row) as usize];
-            crate::ContextMenuOrigin::EditorPoint(point) => {
+                gpui::Point {
-                let cursor_row_layout = &line_layouts[point.row().minus(start_row) as usize];
+                    x: cursor_row_layout.x_for_index(display_point.column() as usize)
-                let x = cursor_row_layout.x_for_index(point.column() as usize)
+                        - scroll_pixel_position.x,
-                    - scroll_pixel_position.x;
+                    y: display_point.row().next_row().as_f32() * line_height
-                let y = point.row().next_row().as_f32() * line_height - scroll_pixel_position.y;
+                        - scroll_pixel_position.y,
-                (x, y)
+                }
             }
             crate::ContextMenuOrigin::GutterIndicator(row) => {
                 // Context menu was spawned via a click on a gutter. Ensure it's a bit closer to the indicator than just a plain first column of the
                 // text field.
-                let x = -gutter_overshoot;
+                gpui::Point {
-                let y = row.next_row().as_f32() * line_height - scroll_pixel_position.y;
+                    x: -gutter_overshoot,
-                (x, y)
+                    y: row.next_row().as_f32() * line_height - scroll_pixel_position.y,
+                }
             }
         };
 
-        let mut list_origin = content_origin + point(x, y);
+        // If the context menu's max height won't fit below, then flip it above the line and display
-        let list_width = context_menu_size.width;
+        // it in reverse order. If the available space above is less than below.
-        let list_height = context_menu_size.height;
+        let unconstrained_max_height = line_height * 12. + POPOVER_Y_PADDING;
+        let min_height = line_height * 3. + POPOVER_Y_PADDING;
+        let target_position = content_origin + target_offset;
+        let y_overflows_below = target_position.y + unconstrained_max_height > text_hitbox.bottom();
+        let bottom_y_when_flipped = target_position.y - line_height;
+        let available_above = bottom_y_when_flipped - text_hitbox.top();
+        let available_below = text_hitbox.bottom() - target_position.y;
+        let mut y_is_flipped = y_overflows_below && available_above > available_below;
+        let mut max_height = cmp::min(
+            unconstrained_max_height,
+            if y_is_flipped {
+                available_above
+            } else {
+                available_below
+            },
+        );
 
-        // Snap the right edge of the list to the right edge of the window if
+        // If less than 3 lines fit within the text bounds, instead fit within the window.
-        // its horizontal bounds overflow.
+        if max_height < 3. * line_height {
-        if list_origin.x + list_width > cx.viewport_size().width {
+            let available_above = bottom_y_when_flipped;
-            list_origin.x = (cx.viewport_size().width - list_width).max(Pixels::ZERO);
+            let available_below = cx.viewport_size().height - target_position.y;
+            if available_below > 3. * line_height {
+                y_is_flipped = false;
+                max_height = min_height;
+            } else if available_above > 3. * line_height {
+                y_is_flipped = true;
+                max_height = min_height;
+            } else if available_above > available_below {
+                y_is_flipped = true;
+                max_height = available_above;
+            } else {
+                y_is_flipped = false;
+                max_height = available_below;
+            }
         }
 
-        if list_origin.y + list_height > text_hitbox.bottom_right().y {
+        let max_height_in_lines = ((max_height - POPOVER_Y_PADDING) / line_height).floor() as u32;
-            list_origin.y -= line_height + list_height;
-        }
 
-        cx.defer_draw(context_menu, list_origin, 1);
+        let Some(mut menu) = self.editor.update(cx, |editor, cx| {
-        true
+            editor.render_context_menu(&self.style, max_height_in_lines, cx)
+        }) else {
+            return;
+        };
+
+        let menu_size = menu.layout_as_root(AvailableSpace::min_size(), cx);
+        let menu_position = gpui::Point {
+            x: if target_position.x + menu_size.width > cx.viewport_size().width {
+                // Snap the right edge of the list to the right edge of the window if its horizontal bounds
+                // overflow.
+                (cx.viewport_size().width - menu_size.width).max(Pixels::ZERO)
+            } else {
+                target_position.x
+            },
+            y: if y_is_flipped {
+                bottom_y_when_flipped - menu_size.height
+            } else {
+                target_position.y
+            },
+        };
+
+        cx.defer_draw(menu, menu_position, 1);
     }
 
     #[allow(clippy::too_many_arguments)]
@@ -5893,13 +5934,11 @@ impl Element for EditorElement {
                                 rows_with_hunk_bounds
                             },
                         );
-                    let mut _context_menu_visible = false;
                     let mut code_actions_indicator = None;
                     if let Some(newest_selection_head) = newest_selection_head {
                         if (start_row..end_row).contains(&newest_selection_head.row()) {
-                            _context_menu_visible = self.layout_context_menu(
+                            self.layout_context_menu(
                                 line_height,
-                                &hitbox,
                                 &text_hitbox,
                                 content_origin,
                                 start_row,

crates/ui/src/components/popover.rs

@@ -3,10 +3,13 @@
 use crate::prelude::*;
 use crate::v_flex;
 use gpui::{
-    div, AnyElement, Element, IntoElement, ParentElement, RenderOnce, Styled, WindowContext,
+    div, AnyElement, Element, IntoElement, ParentElement, Pixels, RenderOnce, Styled, WindowContext,
 };
 use smallvec::SmallVec;
 
+/// Y height added beyond the size of the contents.
+pub const POPOVER_Y_PADDING: Pixels = px(8.);
+
 /// A popover is used to display a menu or show some options.
 ///
 /// Clicking the element that launches the popover should not change the current view,
@@ -45,7 +48,12 @@ impl RenderOnce for Popover {
         div()
             .flex()
             .gap_1()
-            .child(v_flex().elevation_2(cx).py_1().children(self.children))
+            .child(
+                v_flex()
+                    .elevation_2(cx)
+                    .py(POPOVER_Y_PADDING / 2.)
+                    .children(self.children),
+            )
             .when_some(self.aside, |this, aside| {
                 this.child(
                     v_flex()