Editor: The popups selecting menu options on mouse move

EmrysMyrddin commented 7 months ago

Tested versions

Bug introduced by this commit: 06c2cda848b4bca8ee2f54a09417a97a7c1384d3

System information

macOS 14.1, Apple M3 Pro

Issue description

~When editing an exported Dictionary property, trying to change the type of a property opens a popup to chose the new type.~

In fact, this seems to be the case of every popup menus. The right click menus for example have the same problem, if you maintain the right click and drag over a menu item, this menu item will be clicked once you start moving the mouse again after releasing the mouse button.

If the mouse doesn't immediatly move after clicking of the edit button, the popup will close itself when the mouse begins to move, using whatever type is under the mouse as the selected type.

Steps to reproduce

Create a script with an exported dictionary
Add this script to a node
Open inspector for this Node
Edit dictionary
Click on the ✎ button
Don't move the mouse for ~1s
Move the mouse

https://github.com/godotengine/godot/assets/3855602/f460ea12-4b91-4a24-89ef-cf220229216e

Minimal reproduction project (MRP)

N/A

EmrysMyrddin commented 7 months ago

That's weird ^^' I was expecting it to not fix your problem

DanielSnd commented 7 months ago

That's weird ^^' I was expecting it to not fix your problem

clicking_issue

I think it didn't entirely, but did in parts? I just tried again after seeing your comment and this is how it's looking for me at the moment. It doesn't "click" on the place the mouse is, before it would be opening the add child window and such, now it seems to be dismissing the popup but not clicking things on it. But also not every time like it was before, can see in the start of the gif that many times the menu shows up just fine, while before it was reproducible 100% of the time.

I do feel this behaviour is different from when I tried and commented yesterday. I'm assuming since this does depend on how long the popup takes to load, when I tried and assumed the behaviour was 100% fixed my popups were loading faster and hiding the issue.

WhalesState commented 7 months ago

The last one is expected behavior. I don't know if it's OS difference, but that's how most apps work (e.g. VS Code, GIMP, Audacity etc.).

In VS Code and Audacity the items are activated when mouse button is up [Windows 10].

kitbdev commented 7 months ago

Yeah, it's an OS difference, see https://github.com/godotengine/godot-proposals/discussions/7463 and https://github.com/godotengine/godot/issues/80346

WhalesState commented 7 months ago

Those issues was caused by https://github.com/godotengine/godot/pull/86952, it was an attempt to fix opening the popup and triggering an action in a single mouse click which wasn't working for 4.x because it's no longer embedding the subwindows by default for the editor. merging this https://github.com/godotengine/godot/pull/87462 which reverts the changes and this https://github.com/godotengine/godot/pull/88392 will fix those issues if the second one will make the non-embedded PopupMenu recieves the initial mouse release event after opening the Menu, but one of them will need to be rebased to fix the conflict in the popup_menu.cpp input function.

I have reverted the changes because of those issues:

The ScrollBar can't be clicked. Clicking it will activate an item under it.
Menu items gets selected when the mouse event is pressed instead of released. After fixing those two issues i have found another issue which will lead to the same fix needed before merging the pull request.
When opening the popup and triggering an action in a single mouse click over a non-clickable item like [submenu or separator or disabled item] the drag_to_press won't become false because the window don't recieve the initial mouse release event when disabling embedded subwindows and will select the first selectable item when the mouse enters it.

akien-mga commented 7 months ago

So given the discussion so far and the multiple related PRs, I think it might be best to start with reverting #86952 to go back to the previous status quo.

Then we can evaluate @bruvzg's and @WhalesState's PRs, and if needed other ones to re-enable what #86952 was trying to achieve without regression. WDYT?

For any such change, we'll need to ensure we test on all platforms with both single-window and multi-window modes.

bruvzg commented 7 months ago

Before https://github.com/godotengine/godot/pull/86952 it was fully usable, and issues were mostly visual, so reverting it should be good enough.

https://github.com/godotengine/godot/pull/88392 fixes more stuff, but break drag-drop (probably can be fixed in the drag-and-drop code).

But I feel like the whole mouse input should be re-evaluated and changed, and maybe send all event to all windows (the global mouse events filters can be integrated as well, to remove native popup code from DisplaServer), and filter them based on window_id in place as necessary, this way popups and DnD can use all events.

donn-xx commented 7 months ago

Just to subscribe and confirm that this is happening on my system. It's almost impossible to use Godot right now!

v4.3.dev.custom_build [fb10e67fe] Godot v4.3.dev (fb10e67fe) - Ubuntu 22.04.3 LTS 22.04 - X11 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2060 (nvidia; 535.154.05) - Intel(R) Core(TM) i5-9400F CPU @ 2.90GHz (6 Threads)

godotengine / godot