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

System menu for Terminal shows up on wrong monitor #15421

Open
zadjii-msft opened this issue May 24, 2023 · 7 comments · May be fixed by #15657
Open

System menu for Terminal shows up on wrong monitor #15421

zadjii-msft opened this issue May 24, 2023 · 7 comments · May be fixed by #15657
Labels
Area-Windowing Window frame, quake mode, tearout Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Milestone

Comments

@zadjii-msft
Copy link
Member

From MSFT:41390752, courtesy of @oldnewthing

Open Terminal maximized on right-hand monitor of dual-monitor system. Press Alt+Space to open the system menu.

Expect: Menu appears at the top left corner of the window.
Actual: Menu appears at the top right corner of the wrong monitor.

Screen shot shows menu opening on the wrong monitor.

Maybe this has to do with monitor layout? I suppose each of my monitors are set up a little higher than the one on the left:
image

Sure enough, this layout repros:
image

My monitors are nowhere near that exotically laid out.

image

When maximized on monitor 3 (the primary monitor), the system menu appears in the upper right of monitor 2.

@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Area-Windowing Window frame, quake mode, tearout labels May 24, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label May 24, 2023
@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 24, 2023
@zadjii-msft zadjii-msft added this to the Backlog milestone May 24, 2023
@michalnpl
Copy link
Contributor

I can confirm the monitor display layout is important. The issue does not repro on this layout:

image

but it reproes on both:

image

and

image

@michalnpl
Copy link
Contributor

Getting closer using Inspect. For this configuration:

image

getting :PowersShell" window (HWND?)

image

Answering only to the desktop being out of bounds... almost like there is padding or a margin.

@oldnewthing
Copy link
Member

oldnewthing commented May 29, 2023

Okay, I see what's going on. You are manually showing the system menu at the upper left corner of the GetWindowRect and didn't take into account the fact that maximized windows have borders which extend beyond the monitor. It never occurred to me that you would try to show the system menu yourself instead of just letting DefWindowProc do it. (Why not just let DefWindowProc do it?)

This also explains why the system menu shows too high. Regular apps show the system menu below the caption, but Terminal shows it over the caption. Put the system menu at the upper left client pixel (not the upper left window pixel) and I think you'll be all good again.

@michalnpl
Copy link
Contributor

I've developed a fix for this issue. I tweaked the position calculations for the system menu, and now it behaves properly. I also added a bunch of error checking and error logging. Thanks for the pointers @oldnewthing!

I've tested manually all the cases that were working just fine before (and they're still good, no worries there) and also with the situations that were causing trouble. Happy to say it's all working correctly now.

I've run the TAEF tests too, and everything came out clean. I also checked the fullscreen mode (F11), and it's all good.

I lack insight into why DefWindowProc doesn't open system menu, but that is recognized in code:

case WM_NCRBUTTONUP:
        // The `DefWindowProc` function doesn't open the system menu for some
        // reason so we have to do it ourselves.
        if (wParam == HTCAPTION)
        {
            OpenSystemMenu(GET_X_LPARAM(lParam), GET_Y_LPARAM(lParam));
        }

Maybe @zadjii-msft can help here; it may be the same reason why some events have to be routed manually.

I was also wondering if this behavior of "borders which extend beyond the monitor" is caused by backward compatibility from DUI or DWM...

Anyway, I'd like to go ahead and create a PR with the fix if that's OK?

@michalnpl
Copy link
Contributor

Ping.

@DHowett
Copy link
Member

DHowett commented Jun 16, 2023

Oh, sorry @michalnpl! Mike went on paternity leave recently. I'm certain this is a fine fix--it is at least better than what we have--and would be glad to see it in PR. Please do.

Why not just let DefWindowProc do it?

As we always say, "I think there was a reason!" This was probably an early casualty of the weird stuff we had to do to interact with the DirectInput capture site that was making our lives difficult when we moved the tabs into the non-client area1.

OpenSystemMenu is the handler for the user-rebindable "open system menu" action, which they can unbind from Alt+Space. I'm not certain whether we can have DefWindowProc handle it for us (though I would very much like to!)

Footnotes

  1. which happened long before WinAppSDK/WinUI3 offered support for it, so it's very much homebrew and very much rough-hewn

@michalnpl michalnpl linked a pull request Jul 5, 2023 that will close this issue
1 task
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Jul 5, 2023
@michalnpl
Copy link
Contributor

Thanks @DHowett , PR created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Windowing Window frame, quake mode, tearout Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants