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

Popups opened from another popup are immediately closed since 1.89.5 #6462

Closed
cfillion opened this issue May 26, 2023 · 5 comments
Closed

Popups opened from another popup are immediately closed since 1.89.5 #6462

cfillion opened this issue May 26, 2023 · 5 comments
Labels
bug multi-viewports popups test wanted Should add corresponding test to ImGuiTestSuite

Comments

@cfillion
Copy link
Contributor

cfillion commented May 26, 2023

Version/Branch of Dear ImGui:

Version: 1.89.5 and 54c1ac3, OK in 1.89.4
Branch: docking

Back-end/Renderer/OS

Observed on:

  • macOS in example_apple_metal
  • macOS in example_sdl2_opengl3
  • macOS, Linux and Windows in custom backends

Not happening on:

  • macOS in example_glfw_opengl3
  • Windows in example_win32_directx10 (EDIT: unless disabling handling of ImGuiViewportFlags_NoFocusOnClick)

My Issue/Question:

When opening a second popup while another popup that has its own viewport is open, the new popup is closed before the user can interact with it.

There was a similar regression with modal popups however that has already been fixed in #6357. A similar issue remains for non-modal popups in some backends. I haven't found what other backends do differently focus-related to work properly.

It seems that (as expected) the OS gives focus back to the underlying window ('test') when the platform window of the first popup is destroyed and that triggers this line before the platform window of the second popup is created and shown:

imgui/imgui.cpp

Line 14022 in 2e810d5

FocusWindow(focused_viewport->Window, focus_request_flags);

[148347] [focus] SetNavWindow("test")
[148352] [popup] OpenPopup("first" -> 0xCA2DB483)
[148352] [popup] OpenPopupEx(0xCA2DB483)
[148353] [viewport] Add Viewport F94658BA '##Popup_ca2db483'
[148353] [focus] SetNavWindow("##Popup_ca2db483")
[148354] [viewport] Create Platform Window F94658BA '##Popup_ca2db483'
[148404] [popup] CloseCurrentPopup 0 -> 0
[148404] [popup] ClosePopupToLevel(0), restore_focus_to_window_under_popup=1
[148404] [focus] SetNavWindow("test")
[148404] [popup] OpenPopup("second" -> 0x37F95BFE)
[148404] [popup] OpenPopupEx(0x37F95BFE)
[148405] [viewport] Add Viewport 2322D768 '##Popup_37f95bfe'
[148405] [focus] SetNavWindow("##Popup_37f95bfe")
* Platform_DestroyWindow for "##Popup_ca2db483" (first) is called here *
[148406] [focus] SetNavWindow("test")
[148406] [popup] ClosePopupsOverWindow("test")
[148406] [popup] ClosePopupToLevel(0), restore_focus_to_window_under_popup=0
[148407] [viewport] Delete Viewport F94658BA '##Popup_ca2db483'
[148408] [viewport] Delete Viewport 2322D768 '##Popup_37f95bfe'

Screenshots/Video

Screencap

Standalone, minimal, complete and verifiable example:

if(ImGui::Begin("test")) {
  if(ImGui::BeginPopup("second")) {
    ImGui::Text("success!");
    ImGui::EndPopup();
  }

  bool open_edit_popup { false };
  if(ImGui::BeginPopup("first")) {
    if(ImGui::Button("open second"))
      open_edit_popup = true;
    ImGui::EndPopup();
  }
  if(open_edit_popup)
    ImGui::OpenPopup("second");

  if(ImGui::Button("open first"))
    ImGui::OpenPopup("first");
}
ImGui::End();
@ocornut ocornut added the popups label May 28, 2023
@cfillion
Copy link
Contributor Author

cfillion commented May 29, 2023

I found the difference between my Windows backend and imgui_impl_win32.cpp: mine wasn't handling ImGuiViewportFlags_NoFocusOnClick. (Somehow with imgui_impl_glfw.cpp on macOS GetWindowFocus for 'test' still returns false after the platform window for the first popup is destroyed so the bug doesn't trigger there.)

Issue can be duplicated with imgui_impl_win32.cpp after commenting out its handling of ImGuiViewportFlags_NoFocusOnClick.

Preventing focus isn't reliable everywhere (could be focused externally, on X11 it's only a hint to the window manager, plus I'm dealing with a parent application whose UI toolkit forces focus on click on Linux).

cfillion added a commit to cfillion/reaimgui that referenced this issue May 29, 2023
Partial workaround for ocornut/imgui#6462

GDK SWELL's GetFocus() implementation is hard-coded to change on click so this does not the fix the issue above on Linux.
https://github.com/justinfrankel/WDL/blob/6a7ba42ab175a859c565c4f68b890048d53b94c7/WDL/swell/swell-generic-gdk.cpp#LL1440C9-L1440C9
@ocornut
Copy link
Owner

ocornut commented May 30, 2023

Thank you for the investigation and repro and report. Will be looking at this now.

@ocornut ocornut added the test wanted Should add corresponding test to ImGuiTestSuite label May 30, 2023
@ocornut
Copy link
Owner

ocornut commented May 30, 2023

Here is a slightly different repro which showcase how it isn't specifically a ImGuiViewportFlags_NoFocusOnClick issue. Here the first popup is replaced by a regular movable window.

The issue happens when the first window (or popup in your version) is destroyed, giving platform focus back to main viewport before the new one has a chance to appear. In my version the bug happens even if the backend handles ImGuiViewportFlags_NoFocusOnClick.

Preventing focus isn't reliable everywhere (could be focused externally

If popup B is opened over window A, focusing window A would normally close the popup.
This is essentially what we are doing.

While I think this one specific issue probably has a specific resolution, which I am looking for, we could potentially decide to make imgui-focus-on-platform-focus an operation with less side-effects by deciding to not close popups, which would be a non-confident way of reducing the scope of work done since #6299. Not entirely opposed to it as it seems like this is can of worm.

I'm certainly glad we now have this Debug Log to help. I started adding some basic platform-focus-emulation in tests for viewports (see "viewport_platform_focus" in test suite https://github.com/ocornut/imgui_test_engine/blob/main/imgui_test_suite/imgui_tests_viewports.cpp#L170) recently so the least I can do here would be to try emulating this issue in a platform agnostic way.

if (ImGui::Begin("test"))
{
    static bool open_window_1 = false;

    if (ImGui::BeginPopup("second"))
    {
        ImGui::Text("success!");
        ImGui::EndPopup();
    }

    bool open_edit_popup = false;
    if (open_window_1 && ImGui::Begin("First"))
    {
        if (ImGui::Button("open second"))
        {
            open_edit_popup = true;
            open_window_1 = false;
        }
        ImGui::End();
    }
    if (open_edit_popup)
        ImGui::OpenPopup("second");

    if (ImGui::Button("open first"))
        open_window_1 = true;
}
ImGui::End();

@ocornut
Copy link
Owner

ocornut commented May 30, 2023

Hello,

I have pushed 3418d50 which I think should fix this problem, as well as a test for it.
As the whole setup is rather complicated could you give it a short with your setup/backend ?

Thanks!

ocornut added a commit to ocornut/imgui_test_engine that referenced this issue May 30, 2023
…ewport is destroyed + TestSuite: added "viewport_platform_focus_3".

Without support in imgui_app the test would succeed prior to fix due to reason explained in ocornut/imgui#6462
TestSuite: other fixes for viewport support of test suite.
ocornut added a commit that referenced this issue May 30, 2023
…#6462, #6299)

Avoid applying imgui-side focus when focus change is due to a viewport destruction.
@cfillion
Copy link
Contributor Author

cfillion commented May 30, 2023

Thank you! Fix works perfectly. I've tested with example_apple_metal & example_sdl2_opengl3 on macOS and my backend on all platforms (macOS/Linux/Windows/wine).

If popup B is opened over window A, focusing window A would normally close the popup. This is essentially what we are doing.

[...] we could potentially decide to make imgui-focus-on-platform-focus an operation with less side-effects by deciding to not close popups,

I like that popup-closing behavior, it behaves like native widget toolkits. My backend builds upon that by clearing ImGui focus when a non-imgui (or different context's) window gains it. Popups are closed, active item cleared (like #6467) and titlebar visibly unfocused.

void onFocusLossAllImGuiPlatformWindowsInContext()
{
  if(ImGui::GetTopMostPopupModal())
    ImGui::ClearActiveID();
  else
    ImGui::FocusWindow(nullptr);
}

Usecase 1: imgui popup auto-closing when focusing anything else in or outside the application:
Take

Usecase 2: imgui window losing focus and clearing active item to mimick a native window:
Take

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug multi-viewports popups test wanted Should add corresponding test to ImGuiTestSuite
Projects
None yet
Development

No branches or pull requests

2 participants