Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moving through nested menus can cause them to erroniously close #7325

Closed
ZingBallyhoo opened this issue Feb 16, 2024 · 4 comments
Closed

Moving through nested menus can cause them to erroniously close #7325

ZingBallyhoo opened this issue Feb 16, 2024 · 4 comments
Labels
bug menus menu bars, menu items popups

Comments

@ZingBallyhoo
Copy link

Version/Branch of Dear ImGui:

Version 1.90.4 WIP (19031), Branch: master

Back-ends:

imgui_impl_win32.cpp + imgui_impl_dx11.cpp

Compiler, OS:

Windows 10 + MSVC

Full config/build information:

Dear ImGui 1.90.4 WIP (19031)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=199711
define: _WIN32
define: _WIN64
define: _MSC_VER=1938
define: _MSVC_LANG=201402
--------------------------------
io.BackendPlatformName: imgui_impl_win32
io.BackendRendererName: imgui_impl_dx11
io.ConfigFlags: 0x00000003
 NavEnableKeyboard
 NavEnableGamepad
io.ConfigInputTextCursorBlink
io.ConfigWindowsResizeFromEdges
io.ConfigMemoryCompactTimer = 60.0
io.BackendFlags: 0x0000000E
 HasMouseCursors
 HasSetMousePos
 RendererHasVtxOffset
--------------------------------
io.Fonts: 1 fonts, Flags: 0x00000000, TexSize: 512,64
io.DisplaySize: 1264.00,761.00
io.DisplayFramebufferScale: 1.00,1.00
--------------------------------
style.WindowPadding: 8.00,8.00
style.WindowBorderSize: 1.00
style.FramePadding: 4.00,3.00
style.FrameRounding: 0.00
style.FrameBorderSize: 0.00
style.ItemSpacing: 8.00,4.00
style.ItemInnerSpacing: 4.00,4.00

Details:

My Issue/Question:

I don't think I can describe this issue better than the title, please see the videos.
The same thing happens with any further level of nesting (always closes up to the same place, in this case "Nested2")

Introduced in commit 76e09c4 (part of 1.90.2)

Screenshots/Video:

Current Behavior Expected Behavior
2024-02-16.22-33-07.mp4
2024-02-16.22-32-33.mp4

Minimal, Complete and Verifiable Example code:

if (ImGui::Begin("MenuRepro"))
{
    if (ImGui::BeginMenu("Nested1"))
    {
        if (ImGui::BeginMenu("Nested2"))
        {
            for (int i = 0; i < 10; i++)
            {
                if (ImGui::BeginMenu(std::to_string(i).c_str())) ImGui::EndMenu();
            }
            ImGui::EndMenu();
        }
        ImGui::EndMenu();
    }
}
ImGui::End();
@ocornut ocornut added menus menu bars, menu items bug labels Feb 19, 2024
ocornut added a commit that referenced this issue Feb 20, 2024
…ld erroneously close the window. (#7325, #7287, #7063)

Amend 76e09c4. Initial call to ClosePopupToLevel d31fe97 (#2880).
See "widgets_menu_reopen_2" in TestSuite.
ocornut added a commit to ocornut/imgui_test_engine that referenced this issue Feb 20, 2024
@ocornut
Copy link
Owner

ocornut commented Feb 20, 2024

Thank you for the report. Should be fixed 014e0ac.
Previous commit 76e09c4 is only accidentally responsible that it made another issue surface, but the logic of it seems correct.

When moving from "A" to "B":

  • "A" closes itself in BeginMenu(), which focus parent.
  • "B" opens (visible and focused next frame)

When moving from "B" to "A":

  • "A" opens which triggers a popup closure, otherwise that specific call to ClosePopupToLevel() had restore_focus_to_window_under_popup = false which here is incorrect (false is only used by FocusXXX functions), so focus was kept on "A"
  • "B" opens (aimed visible and focused next frame)
  • NewFrame() detect A is focused but not visible anymore, tries to focus top-most window using FocusTopMostWindowUnderOne(NULL, NULL, NULL, ImGuiFocusRequestFlags_RestoreFocusedChild); but because this looks root window only it would revert focus to the root menus, which in turns close all its sub-menus.

So I fixed the call to ClosePopupToLevel() to use restore_focus_to_window_under_popup = true which seems correct and passes all my tests (and checked where it was introduced, in d31fe97).
But it's not impossible that the logic of FocusTopMostWindowUnderOne() would be improved in the future, as ideally it should be usable in chain of child menus (I don't have a clear situation in mind where it would be problematic but I can envision it is possible).

I was puzzled by why it didn't happen in the demo, but this requires:

  • two adjacent sibling menus
  • moving upward
  • not being in the root menu
    The only situation where this exists in the demo is with the "Options"/"Colors" menu when browsed from the recursive path, but the demo also show appending to a menu with the "Options" and this accidentally "fixes" the issue.

Added test ocornut/imgui_test_engine@e4254ea

@ocornut ocornut closed this as completed Feb 20, 2024
@ZingBallyhoo
Copy link
Author

I am actually still able to reproduce this on 1.90.4.. but only when the mouse is moving fast (skipping over items?)

2024-02-26.22-10-45.mp4

@ocornut ocornut reopened this Feb 26, 2024
@ocornut
Copy link
Owner

ocornut commented Feb 27, 2024

I can confirm there is an issue when trying to open a sibling menu two frames in a row. I'm investigating.

@ocornut ocornut added the popups label Feb 27, 2024
ocornut added a commit that referenced this issue Feb 27, 2024
…dow and minor tweaks. Should be functionally a no-op.

This is expected to clear the noise so next commit can be cleared to read. (#7325)
Last renamed in b3ea01d
ocornut added a commit that referenced this issue Feb 27, 2024
… successive frames would erroneously close the window. (#7325, #7287, #7063)

Amend 014e0ac
ocornut added a commit to ocornut/imgui_test_engine that referenced this issue Feb 27, 2024
@ocornut
Copy link
Owner

ocornut commented Feb 27, 2024

I spent quite a few hours investigating what the proper solution was, and in the end I think this simple fix is the most correct thing to do: c3f8f4d

I have amended the test for it : ocornut/imgui_test_engine@a35d13a

Thanks again!

Some info, when we close a popup via ClosePopupToLevel() we rely on info stored in g.OpenPopupStack[] for that popup, but that info is incomplete if the popup window has never been Begin()-ed into.

  • Frame N: Mouse is over Item 3
  • Frame N+1: Move to item 2: when item 2 detects hover is requests popup open but can't display immediately (technically in this specific ordering it could, but the logic is to avoid recycling same popup host window in a same frame).
  • Frame N+2: Move to item 1: when item 1 detects hover it requests popup open, which closes previous one, that closure normally first restore previous focus because opening the new window.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug menus menu bars, menu items popups
Projects
None yet
Development

No branches or pull requests

2 participants