Merge pull request #17809 from RandomShaper/menu-item-on-release

Improve popup menus usability

8 files changed, 68 insertions, 38 deletions

diff --git a/editor/plugins/canvas_item_editor_plugin.cpp b/editor/plugins/canvas_item_editor_plugin.cpp
index 0184bff050..b8d0958c99 100644
--- a/editor/plugins/canvas_item_editor_plugin.cpp
+++ b/editor/plugins/canvas_item_editor_plugin.cpp
@@ -1672,8 +1672,6 @@ bool CanvasItemEditor::_gui_input_select(const Ref<InputEvent> &p_event) {
 				selection_menu_additive_selection = b->get_shift();
 				selection_menu->set_global_position(b->get_global_position());
 				selection_menu->popup();
-				selection_menu->call_deferred("grab_click_focus");
-				selection_menu->set_invalidate_click_until_motion();
 				return true;
 			}
 		}
diff --git a/editor/plugins/shader_graph_editor_plugin.cpp b/editor/plugins/shader_graph_editor_plugin.cpp
index 59085c203f..e1d28cc215 100644
--- a/editor/plugins/shader_graph_editor_plugin.cpp
+++ b/editor/plugins/shader_graph_editor_plugin.cpp
@@ -2769,8 +2769,6 @@ void ShaderGraphEditor::_popup_requested(const Vector2 &p_position)
 	popup->set_global_position(p_position);
 	popup->set_size( Size2( 200, 0) );
 	popup->popup();
-	popup->call_deferred("grab_click_focus");
-	popup->set_invalidate_click_until_motion();
 }
 
 void ShaderGraphEditor::_notification(int p_what) {
diff --git a/editor/plugins/spatial_editor_plugin.cpp b/editor/plugins/spatial_editor_plugin.cpp
index e484140527..671d5577a0 100644
--- a/editor/plugins/spatial_editor_plugin.cpp
+++ b/editor/plugins/spatial_editor_plugin.cpp
@@ -886,8 +886,6 @@ void SpatialEditorViewport::_list_select(Ref<InputEventMouseButton> b) {
 
 		selection_menu->set_global_position(b->get_global_position());
 		selection_menu->popup();
-		selection_menu->call_deferred("grab_click_focus");
-		selection_menu->set_invalidate_click_until_motion();
 	}
 }
 
diff --git a/scene/gui/menu_button.cpp b/scene/gui/menu_button.cpp
index 2e74faa61d..87cf4dc334 100644
--- a/scene/gui/menu_button.cpp
+++ b/scene/gui/menu_button.cpp
@@ -59,7 +59,6 @@ void MenuButton::pressed() {
 	popup->set_size(Size2(size.width, 0));
 	popup->set_parent_rect(Rect2(Point2(gp - popup->get_global_position()), get_size()));
 	popup->popup();
-	popup->set_invalidate_click_until_motion();
 }
 
 void MenuButton::_gui_input(Ref<InputEvent> p_event) {
@@ -109,7 +108,6 @@ MenuButton::MenuButton() {
 	add_child(popup);
 	popup->set_as_toplevel(true);
 	popup->set_pass_on_modal_close_click(false);
-	connect("button_up", popup, "call_deferred", make_binds("grab_click_focus"));
 	set_process_unhandled_key_input(true);
 	set_action_mode(ACTION_MODE_BUTTON_PRESS);
 }
diff --git a/scene/gui/popup_menu.cpp b/scene/gui/popup_menu.cpp
index 747230e69f..27853a8238 100644
--- a/scene/gui/popup_menu.cpp
+++ b/scene/gui/popup_menu.cpp
@@ -284,7 +284,8 @@ void PopupMenu::_gui_input(const Ref<InputEvent> &p_event) {
 		if (b->is_pressed())
 			return;
 
-		switch (b->get_button_index()) {
+		int button_idx = b->get_button_index();
+		switch (button_idx) {
 
 			case BUTTON_WHEEL_DOWN: {
 
@@ -298,30 +299,37 @@ void PopupMenu::_gui_input(const Ref<InputEvent> &p_event) {
 					_scroll(b->get_factor(), b->get_position());
 				}
 			} break;
-			case BUTTON_LEFT: {
-
-				int over = _get_mouse_over(b->get_position());
-
-				if (invalidated_click) {
-					invalidated_click = false;
-					break;
-				}
-				if (over < 0) {
-					hide();
-					break; //non-activable
-				}
-
-				if (items[over].separator || items[over].disabled)
-					break;
-
-				if (items[over].submenu != "") {
-
-					_activate_submenu(over);
-					return;
+			default: {
+				// Allow activating item by releasing the LMB or any that was down when the popup appeared
+				if (button_idx == BUTTON_LEFT || (initial_button_mask & (1 << (button_idx - 1)))) {
+
+					bool was_during_grabbed_click = during_grabbed_click;
+					during_grabbed_click = false;
+
+					int over = _get_mouse_over(b->get_position());
+
+					if (invalidated_click) {
+						invalidated_click = false;
+						break;
+					}
+					if (over < 0) {
+						if (!was_during_grabbed_click) {
+							hide();
+						}
+						break; //non-activable
+					}
+
+					if (items[over].separator || items[over].disabled)
+						break;
+
+					if (items[over].submenu != "") {
+
+						_activate_submenu(over);
+						return;
+					}
+					activate_item(over);
 				}
-				activate_item(over);
-
-			} break;
+			}
 		}
 
 		//update();
@@ -503,6 +511,11 @@ void PopupMenu::_notification(int p_what) {
 				update();
 			}
 		} break;
+		case NOTIFICATION_POST_POPUP: {
+
+			initial_button_mask = Input::get_singleton()->get_mouse_button_mask();
+			during_grabbed_click = (bool)initial_button_mask;
+		} break;
 		case NOTIFICATION_POPUP_HIDE: {
 
 			if (mouse_over >= 0) {
@@ -1216,15 +1229,20 @@ void PopupMenu::_bind_methods() {
 	ADD_SIGNAL(MethodInfo("index_pressed", PropertyInfo(Variant::INT, "index")));
 }
 
-void PopupMenu::set_invalidate_click_until_motion() {
+void PopupMenu::popup(const Rect2 &p_bounds) {
+
+	grab_click_focus();
 	moved = Vector2();
 	invalidated_click = true;
+	Popup::popup(p_bounds);
 }
 
 PopupMenu::PopupMenu() {
 
 	mouse_over = -1;
 	submenu_over = -1;
+	initial_button_mask = 0;
+	during_grabbed_click = false;
 
 	set_focus_mode(FOCUS_ALL);
 	set_as_toplevel(true);
diff --git a/scene/gui/popup_menu.h b/scene/gui/popup_menu.h
index 60f36e95ec..4a62f81a65 100644
--- a/scene/gui/popup_menu.h
+++ b/scene/gui/popup_menu.h
@@ -78,6 +78,8 @@ class PopupMenu : public Popup {
 	Timer *submenu_timer;
 	List<Rect2> autohide_areas;
 	Vector<Item> items;
+	int initial_button_mask;
+	bool during_grabbed_click;
 	int mouse_over;
 	int submenu_over;
 	Rect2 parent_rect;
@@ -178,7 +180,6 @@ public:
 	void add_autohide_area(const Rect2 &p_area);
 	void clear_autohide_areas();
 
-	void set_invalidate_click_until_motion();
 	void set_hide_on_item_selection(bool p_enabled);
 	bool is_hide_on_item_selection() const;
 
@@ -188,6 +189,8 @@ public:
 	void set_hide_on_multistate_item_selection(bool p_enabled);
 	bool is_hide_on_multistate_item_selection() const;
 
+	virtual void popup(const Rect2 &p_bounds = Rect2());
+
 	PopupMenu();
 	~PopupMenu();
 };
diff --git a/scene/main/viewport.cpp b/scene/main/viewport.cpp
index 08fbf44469..45a969eeda 100644
--- a/scene/main/viewport.cpp
+++ b/scene/main/viewport.cpp
@@ -181,6 +181,7 @@ public:
 Viewport::GUI::GUI() {
 
 	mouse_focus = NULL;
+	mouse_click_grabber = NULL;
 	mouse_focus_button = -1;
 	key_focus = NULL;
 	mouse_over = NULL;
@@ -2278,7 +2279,7 @@ List<Control *>::Element *Viewport::_gui_show_modal(Control *p_control) {
 	else
 		p_control->_modal_set_prev_focus_owner(0);
 
-	if (gui.mouse_focus && !p_control->is_a_parent_of(gui.mouse_focus)) {
+	if (gui.mouse_focus && !p_control->is_a_parent_of(gui.mouse_focus) && !gui.mouse_click_grabber) {
 		Ref<InputEventMouseButton> mb;
 		mb.instance();
 		mb->set_position(gui.mouse_focus->get_local_mouse_position());
@@ -2300,9 +2301,22 @@ Control *Viewport::_gui_get_focus_owner() {
 
 void Viewport::_gui_grab_click_focus(Control *p_control) {
 
+	gui.mouse_click_grabber = p_control;
+	call_deferred("_post_gui_grab_click_focus");
+}
+
+void Viewport::_post_gui_grab_click_focus() {
+
+	Control *focus_grabber = gui.mouse_click_grabber;
+	if (!focus_grabber) {
+		// Redundant grab requests were made
+		return;
+	}
+	gui.mouse_click_grabber = NULL;
+
 	if (gui.mouse_focus) {
 
-		if (gui.mouse_focus == p_control)
+		if (gui.mouse_focus == focus_grabber)
 			return;
 		Ref<InputEventMouseButton> mb;
 		mb.instance();
@@ -2313,9 +2327,9 @@ void Viewport::_gui_grab_click_focus(Control *p_control) {
 		mb->set_position(click);
 		mb->set_button_index(gui.mouse_focus_button);
 		mb->set_pressed(false);
-		gui.mouse_focus->call_deferred(SceneStringNames::get_singleton()->_gui_input, mb);
+		gui.mouse_focus->call_multilevel(SceneStringNames::get_singleton()->_gui_input, mb);
 
-		gui.mouse_focus = p_control;
+		gui.mouse_focus = focus_grabber;
 		gui.focus_inv_xform = gui.mouse_focus->get_global_transform_with_canvas().affine_inverse();
 		click = gui.mouse_focus->get_global_transform_with_canvas().affine_inverse().xform(gui.last_mouse_pos);
 		mb->set_position(click);
@@ -2648,6 +2662,7 @@ void Viewport::_bind_methods() {
 
 	ClassDB::bind_method(D_METHOD("_gui_show_tooltip"), &Viewport::_gui_show_tooltip);
 	ClassDB::bind_method(D_METHOD("_gui_remove_focus"), &Viewport::_gui_remove_focus);
+	ClassDB::bind_method(D_METHOD("_post_gui_grab_click_focus"), &Viewport::_post_gui_grab_click_focus);
 
 	ClassDB::bind_method(D_METHOD("set_shadow_atlas_size", "size"), &Viewport::set_shadow_atlas_size);
 	ClassDB::bind_method(D_METHOD("get_shadow_atlas_size"), &Viewport::get_shadow_atlas_size);
diff --git a/scene/main/viewport.h b/scene/main/viewport.h
index 07bbd3f1fa..94e49033e0 100644
--- a/scene/main/viewport.h
+++ b/scene/main/viewport.h
@@ -248,6 +248,7 @@ private:
 
 		bool key_event_accepted;
 		Control *mouse_focus;
+		Control *mouse_click_grabber;
 		int mouse_focus_button;
 		Control *key_focus;
 		Control *mouse_over;
@@ -323,6 +324,7 @@ private:
 	bool _gui_control_has_focus(const Control *p_control);
 	void _gui_control_grab_focus(Control *p_control);
 	void _gui_grab_click_focus(Control *p_control);
+	void _post_gui_grab_click_focus();
 	void _gui_accept_event();
 
 	Control *_gui_get_focus_owner();