Fix PopupMenu doesn't respect its ScrollContainer's margins

editor/editor_command_palette.cpp

@@ -36,6 +36,7 @@
 #include "editor/gui/editor_toaster.h"
 #include "editor/themes/editor_scale.h"
 #include "scene/gui/control.h"
+#include "scene/gui/margin_container.h"
 #include "scene/gui/tree.h"
 
 EditorCommandPalette *EditorCommandPalette::singleton = nullptr;

editor/editor_layouts_dialog.cpp

@@ -37,6 +37,7 @@
 #include "editor/themes/editor_scale.h"
 #include "scene/gui/item_list.h"
 #include "scene/gui/line_edit.h"
+#include "scene/gui/margin_container.h"
 
 void EditorLayoutsDialog::_line_gui_input(const Ref<InputEvent> &p_event) {
 	Ref<InputEventKey> k = p_event;

editor/export/project_export.cpp

@@ -45,6 +45,7 @@
 #include "scene/gui/check_button.h"
 #include "scene/gui/item_list.h"
 #include "scene/gui/link_button.h"
+#include "scene/gui/margin_container.h"
 #include "scene/gui/menu_button.h"
 #include "scene/gui/option_button.h"
 #include "scene/gui/popup_menu.h"

editor/gui/editor_object_selector.cpp

@@ -35,6 +35,7 @@
 #include "editor/editor_string_names.h"
 #include "editor/multi_node_edit.h"
 #include "editor/themes/editor_scale.h"
+#include "scene/gui/margin_container.h"
 
 Size2 EditorObjectSelector::get_minimum_size() const {
 	Ref<Font> font = get_theme_font(SNAME("font"));

editor/plugins/animation_tree_editor_plugin.cpp

@@ -45,6 +45,7 @@
 #include "scene/animation/animation_blend_tree.h"
 #include "scene/animation/animation_player.h"
 #include "scene/gui/button.h"
+#include "scene/gui/margin_container.h"
 #include "scene/gui/menu_button.h"
 #include "scene/gui/panel.h"
 #include "scene/gui/scroll_container.h"

editor/plugins/font_config_plugin.cpp

@@ -33,6 +33,7 @@
 #include "editor/editor_settings.h"
 #include "editor/import/dynamic_font_import_settings.h"
 #include "editor/themes/editor_scale.h"
+#include "scene/gui/margin_container.h"
 
 /*************************************************************************/
 /*  EditorPropertyFontMetaObject                                         */

editor/plugins/text_shader_editor.h

@@ -32,6 +32,7 @@
 #define TEXT_SHADER_EDITOR_H
 
 #include "editor/code_editor.h"
+#include "scene/gui/margin_container.h"
 #include "scene/gui/menu_button.h"
 #include "scene/gui/panel_container.h"
 #include "scene/gui/rich_text_label.h"

scene/gui/color_picker.cpp

@@ -36,6 +36,7 @@
 #include "core/os/keyboard.h"
 #include "core/os/os.h"
 #include "scene/gui/color_mode.h"
+#include "scene/gui/margin_container.h"
 #include "scene/resources/image_texture.h"
 #include "scene/resources/style_box_flat.h"
 #include "scene/resources/style_box_texture.h"

scene/gui/popup_menu.cpp

@@ -213,8 +213,8 @@ Size2 PopupMenu::_get_item_icon_size(int p_idx) const {
 }
 
 Size2 PopupMenu::_get_contents_minimum_size() const {
-	Size2 minsize = theme_cache.panel_style->get_minimum_size(); // Accounts for margin in the margin container
-	minsize.x += scroll_container->get_v_scroll_bar()->get_size().width * 2; // Adds a buffer so that the scrollbar does not render over the top of content
+	Size2 minsize = theme_cache.panel_style->get_minimum_size();
+	minsize.width += scroll_container->get_v_scroll_bar()->get_size().width;
 
 	float max_w = 0.0;
 	float icon_w = 0.0;
@@ -304,16 +304,11 @@ int PopupMenu::_get_items_total_height() const {
 }
 
 int PopupMenu::_get_mouse_over(const Point2 &p_over) const {
-	if (p_over.x < 0 || p_over.x >= get_size().width) {
+	if (p_over.x < 0 || p_over.x >= get_size().width || p_over.y < theme_cache.panel_style->get_margin(Side::SIDE_TOP)) {
 		return -1;
 	}
 
-	// Accounts for margin in the margin container
-	Point2 ofs = theme_cache.panel_style->get_offset();
-
-	if (ofs.y > p_over.y) {
-		return -1;
-	}
+	Point2 ofs;
 
 	for (int i = 0; i < items.size(); i++) {
 		ofs.y += theme_cache.v_separation;
@@ -570,32 +565,39 @@ void PopupMenu::_input_from_window_internal(const Ref<InputEvent> &p_event) {
 	if (scroll_container->get_v_scroll_bar()->is_visible_in_tree()) {
 		if (is_layout_rtl()) {
 			item_clickable_area.position.x += scroll_container->get_v_scroll_bar()->get_size().width;
-		} else {
-			item_clickable_area.size.width -= scroll_container->get_v_scroll_bar()->get_size().width;
 		}
+		item_clickable_area.size.width -= scroll_container->get_v_scroll_bar()->get_size().width;
 	}
 
 	Ref<InputEventMouseButton> b = p_event;
 
 	if (b.is_valid()) {
-		if (!item_clickable_area.has_point(b->get_position())) {
-			return;
-		}
-
 		MouseButton button_idx = b->get_button_index();
-		if (!b->is_pressed()) {
-			// Activate the item on release of either the left mouse button or
-			// any mouse button held down when the popup was opened.
-			// This allows for opening the popup and triggering an action in a single mouse click.
-			if (button_idx == MouseButton::LEFT || initial_button_mask.has_flag(mouse_button_to_mask(button_idx))) {
+		// Activate the item on release of either the left mouse button or
+		// any mouse button held down when the popup was opened.
+		// This allows for opening the popup and triggering an action in a single mouse click.
+		if (button_idx == MouseButton::LEFT || initial_button_mask.has_flag(mouse_button_to_mask(button_idx))) {
+			if (b->is_pressed()) {
+				is_scrolling = is_layout_rtl() ? b->get_position().x < item_clickable_area.position.x : b->get_position().x > item_clickable_area.size.width;
+
+				if (!item_clickable_area.has_point(b->get_position())) {
+					return;
+				}
+				_mouse_over_update(b->get_position());
+			} else {
+				if (is_scrolling) {
+					is_scrolling = false;
+					return;
+				}
 				bool was_during_grabbed_click = during_grabbed_click;
 				during_grabbed_click = false;
 				initial_button_mask.clear();
 
+				if (!item_clickable_area.has_point(b->get_position())) {
+					return;
+				}
 				// Disable clicks under a time threshold to avoid selection right when opening the popup.
-				uint64_t now = OS::get_singleton()->get_ticks_msec();
-				uint64_t diff = now - popup_time_msec;
-				if (diff < 150) {
+				if (was_during_grabbed_click && OS::get_singleton()->get_ticks_msec() - popup_time_msec < 150) {
 					return;
 				}
 
@@ -638,25 +640,7 @@ void PopupMenu::_input_from_window_internal(const Ref<InputEvent> &p_event) {
 		if (!item_clickable_area.has_point(m->get_position())) {
 			return;
 		}
-
-		int over = _get_mouse_over(m->get_position());
-		int id = (over < 0 || items[over].separator || items[over].disabled) ? -1 : (items[over].id >= 0 ? items[over].id : over);
-
-		if (id < 0) {
-			mouse_over = -1;
-			control->queue_redraw();
-			return;
-		}
-
-		if (items[over].submenu && submenu_over != over) {
-			submenu_over = over;
-			submenu_timer->start();
-		}
-
-		if (over != mouse_over) {
-			mouse_over = over;
-			control->queue_redraw();
-		}
+		_mouse_over_update(m->get_position());
 	}
 
 	Ref<InputEventKey> k = p_event;
@@ -700,14 +684,31 @@ void PopupMenu::_input_from_window_internal(const Ref<InputEvent> &p_event) {
 	}
 }
 
+void PopupMenu::_mouse_over_update(const Point2 &p_over) {
+	int over = _get_mouse_over(p_over);
+	int id = (over < 0 || items[over].separator || items[over].disabled) ? -1 : (items[over].id >= 0 ? items[over].id : over);
+
+	if (id < 0) {
+		mouse_over = -1;
+		control->queue_redraw();
+		return;
+	}
+
+	if (!is_scrolling && items[over].submenu && submenu_over != over) {
+		submenu_over = over;
+		submenu_timer->start();
+	}
+
+	if (over != mouse_over) {
+		mouse_over = over;
+		control->queue_redraw();
+	}
+}
+
 void PopupMenu::_draw_items() {
 	control->set_custom_minimum_size(Size2(0, _get_items_total_height()));
 	RID ci = control->get_canvas_item();
 
-	Size2 margin_size;
-	margin_size.width = margin_container->get_margin_size(SIDE_LEFT) + margin_container->get_margin_size(SIDE_RIGHT);
-	margin_size.height = margin_container->get_margin_size(SIDE_TOP) + margin_container->get_margin_size(SIDE_BOTTOM);
-
 	// Space between the item content and the sides of popup menu.
 	bool rtl = control->is_layout_rtl();
 	// In Item::checkable_type enum order (less the non-checkable member), with disabled repeated at the end.
@@ -720,8 +721,7 @@ void PopupMenu::_draw_items() {
 		submenu = theme_cache.submenu;
 	}
 
-	float scroll_width = scroll_container->get_v_scroll_bar()->is_visible_in_tree() ? scroll_container->get_v_scroll_bar()->get_size().width : 0;
-	float display_width = control->get_size().width - scroll_width;
+	float display_width = control->get_size().width;
 
 	// Find the widest icon and whether any items have a checkbox, and store the offsets for each.
 	float icon_ofs = 0.0;
@@ -765,11 +765,7 @@ void PopupMenu::_draw_items() {
 		float h = _get_item_height(i);
 
 		if (i == mouse_over) {
-			if (rtl) {
-				theme_cache.hover_style->draw(ci, Rect2(item_ofs + Point2(scroll_width, -theme_cache.v_separation / 2), Size2(display_width, h + theme_cache.v_separation)));
-			} else {
-				theme_cache.hover_style->draw(ci, Rect2(item_ofs + Point2(0, -theme_cache.v_separation / 2), Size2(display_width, h + theme_cache.v_separation)));
-			}
+			theme_cache.hover_style->draw(ci, Rect2(item_ofs + Point2(0, -theme_cache.v_separation / 2), Size2(display_width, h + theme_cache.v_separation)));
 		}
 
 		String text = items[i].xl_text;
@@ -851,7 +847,7 @@ void PopupMenu::_draw_items() {
 		// Submenu arrow on right hand side.
 		if (items[i].submenu) {
 			if (rtl) {
-				submenu->draw(ci, Point2(scroll_width + theme_cache.panel_style->get_margin(SIDE_LEFT) + theme_cache.item_end_padding, item_ofs.y + Math::floor(h - submenu->get_height()) / 2), icon_color);
+				submenu->draw(ci, Point2(theme_cache.panel_style->get_margin(SIDE_LEFT) + theme_cache.item_end_padding, item_ofs.y + Math::floor(h - submenu->get_height()) / 2), icon_color);
 			} else {
 				submenu->draw(ci, Point2(display_width - theme_cache.panel_style->get_margin(SIDE_RIGHT) - submenu->get_width() - theme_cache.item_end_padding, item_ofs.y + Math::floor(h - submenu->get_height()) / 2), icon_color);
 			}
@@ -888,7 +884,7 @@ void PopupMenu::_draw_items() {
 		// Accelerator / Shortcut
 		if (items[i].accel != Key::NONE || (items[i].shortcut.is_valid() && items[i].shortcut->has_valid_event())) {
 			if (rtl) {
-				item_ofs.x = scroll_width + theme_cache.panel_style->get_margin(SIDE_LEFT) + theme_cache.item_end_padding;
+				item_ofs.x = theme_cache.panel_style->get_margin(SIDE_LEFT) + theme_cache.item_end_padding;
 			} else {
 				item_ofs.x = display_width - theme_cache.panel_style->get_margin(SIDE_RIGHT) - items[i].accel_text_buf->get_size().x - theme_cache.item_end_padding;
 			}
@@ -907,11 +903,6 @@ void PopupMenu::_draw_items() {
 	}
 }
 
-void PopupMenu::_draw_background() {
-	RID ci2 = margin_container->get_canvas_item();
-	theme_cache.panel_style->draw(ci2, Rect2(Point2(), margin_container->get_size()));
-}
-
 void PopupMenu::_minimum_lifetime_timeout() {
 	close_allowed = true;
 	// If the mouse still isn't in this popup after timer expires, close.
@@ -1023,7 +1014,11 @@ void PopupMenu::_notification(int p_what) {
 			}
 		} break;
 
-		case NOTIFICATION_THEME_CHANGED:
+		case NOTIFICATION_THEME_CHANGED: {
+			scroll_container->add_theme_style_override("panel", theme_cache.panel_style);
+
+			[[fallthrough]];
+		}
 		case Control::NOTIFICATION_LAYOUT_DIRECTION_CHANGED:
 		case NOTIFICATION_TRANSLATION_CHANGED: {
 			DisplayServer *ds = DisplayServer::get_singleton();
@@ -1169,14 +1164,6 @@ void PopupMenu::_notification(int p_what) {
 				if (!is_embedded()) {
 					set_process_internal(true);
 				}
-
-				// Set margin on the margin container
-				margin_container->begin_bulk_theme_override();
-				margin_container->add_theme_constant_override("margin_left", theme_cache.panel_style->get_margin(Side::SIDE_LEFT));
-				margin_container->add_theme_constant_override("margin_top", theme_cache.panel_style->get_margin(Side::SIDE_TOP));
-				margin_container->add_theme_constant_override("margin_right", theme_cache.panel_style->get_margin(Side::SIDE_RIGHT));
-				margin_container->add_theme_constant_override("margin_bottom", theme_cache.panel_style->get_margin(Side::SIDE_BOTTOM));
-				margin_container->end_bulk_theme_override();
 			}
 		} break;
 	}
@@ -2800,16 +2787,11 @@ void PopupMenu::popup(const Rect2i &p_bounds) {
 }
 
 PopupMenu::PopupMenu() {
-	// Margin Container
-	margin_container = memnew(MarginContainer);
-	margin_container->set_anchors_and_offsets_preset(Control::PRESET_FULL_RECT);
-	add_child(margin_container, false, INTERNAL_MODE_FRONT);
-	margin_container->connect("draw", callable_mp(this, &PopupMenu::_draw_background));
-
 	// Scroll Container
 	scroll_container = memnew(ScrollContainer);
+	scroll_container->set_anchors_and_offsets_preset(Control::PRESET_FULL_RECT);
 	scroll_container->set_clip_contents(true);
-	margin_container->add_child(scroll_container);
+	add_child(scroll_container, false, INTERNAL_MODE_FRONT);
 
 	// The control which will display the items
 	control = memnew(Control);

scene/gui/popup_menu.h

@@ -32,7 +32,6 @@
 #define POPUP_MENU_H
 
 #include "core/input/shortcut.h"
-#include "scene/gui/margin_container.h"
 #include "scene/gui/popup.h"
 #include "scene/gui/scroll_container.h"
 #include "scene/property_list_helper.h"
@@ -110,10 +109,12 @@ class PopupMenu : public Popup {
 	Vector<Item> items;
 	BitField<MouseButtonMask> initial_button_mask;
 	bool during_grabbed_click = false;
+	bool is_scrolling = false;
 	int mouse_over = -1;
 	int submenu_over = -1;
 	String _get_accel_text(const Item &p_item) const;
 	int _get_mouse_over(const Point2 &p_over) const;
+	void _mouse_over_update(const Point2 &p_over);
 	virtual Size2 _get_contents_minimum_size() const override;
 
 	int _get_item_height(int p_idx) const;
@@ -142,7 +143,6 @@ class PopupMenu : public Popup {
 	uint64_t search_time_msec = 0;
 	String search_string = "";
 
-	MarginContainer *margin_container = nullptr;
 	ScrollContainer *scroll_container = nullptr;
 	Control *control = nullptr;
 
@@ -195,7 +195,6 @@ class PopupMenu : public Popup {
 	} theme_cache;
 
 	void _draw_items();
-	void _draw_background();
 
 	void _minimum_lifetime_timeout();
 	void _close_pressed();