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

Other applications open behind the Terminal window - (ConPTY window handle z-order shenanigans with GetConsoleWindow) #2988

Closed
amithegde opened this issue Sep 30, 2019 · 30 comments
Assignees
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. zInbox-Bug Ignore me!

Comments

@amithegde
Copy link

Description of the new feature/enhancement

When I open File explorer from terminal using command such as start . control should shift to explorer window but it remains in terminal.

Proposed technical implementation details (optional)

when file explorer starts control should shift to explorer and any further keyboard interaction should be targeted to explorer window.
You can verify how ConEmu or cmd works to validate this.

@amithegde amithegde added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Sep 30, 2019
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Sep 30, 2019
@amithegde
Copy link
Author

I noticed it does shift control some times. So I guess it's a bug? Some times control doesn't shift and some times it shifts?

@zadjii-msft
Copy link
Member

This might not be possible. This repros for any conpty session.

I (believe) that traditionally, start uses GetConsoleWindow to get the HWND of the console, to place explorer (or any application really) in the z-order above the conhost.exe window.

For something running in conpty however, the console doesn't actually have a window. The Terminal does, but the console doesn't. So there's no way for start to place something above the console window, since there is no console window.

@zadjii-msft zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Conpty For console issues specifically related to conpty labels Sep 30, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 30, 2019
@fpqc
Copy link

fpqc commented Sep 30, 2019

I can't imagine it would be hard for a motivated person to write a small Windows program that launches a program and then sets it at the top of the Z-order without comparing against the current window.

@amithegde
Copy link
Author

This might not be possible. This repros for any conpty session.

I (believe) that traditionally, start uses GetConsoleWindow to get the HWND of the console, to place explorer (or any application really) in the z-order above the conhost.exe window.

For something running in conpty however, the console doesn't actually have a window. The Terminal does, but the console doesn't. So there's no way for start to place something above the console window, since there is no console window.

I will be demotivated to use Terminal in that case since it doesn't behave consistently with existing tools like cmd😒

@DHowett-MSFT DHowett-MSFT added this to the Console Backlog milestone Sep 30, 2019
@DHowett-MSFT DHowett-MSFT added Issue-Bug It either shouldn't be doing this or needs an investigation. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Sep 30, 2019
@DHowett-MSFT DHowett-MSFT changed the title Shift focus to explorer when file explorer is opened from terminal using start command cmd start doesn't work predictably when used through ConPTY (window handle shenanigans) Sep 30, 2019
@DHowett-MSFT DHowett-MSFT added the Priority-3 A description (P3) label Sep 30, 2019
@dracan
Copy link

dracan commented Feb 22, 2020

Not sure if this information is useful or not, but I had created a similar issue #3636 which got closed as a duplicate of this current issue. In my case, it was git difftool that was causing winmerge to not appear in the foreground. I've just discovered that whilst it happens when I do a normal git difftool, which triggers a Launch 'winmerge' [Y/n]? prompt - if instead I do git difftool -y, which suppresses that prompt - it works as expected.

@dracan
Copy link

dracan commented Feb 22, 2020

Although, if more than one file has differences - the first launch of winmerge is in the foreground (when specifying the -y mentioned in my previous post), however any subsequent ones have the issue where the winmerge window is right at the back of all other windows.

I've also just noticed that the same issue happens from the vscode terminal. It doesn't happen when using the native Powershell terminal window though.

@DHowett-MSFT DHowett-MSFT changed the title cmd start doesn't work predictably when used through ConPTY (window handle shenanigans) cmd start doesn't work predictably when used through ConPTY (window handle shenanigans; GetConsoleWindow) Mar 30, 2020
@zadjii-msft
Copy link
Member

zadjii-msft commented Feb 18, 2022

I have nowhere else for good notes, so sorry for the kinda random ping here folks:

b257581 status

As of b257581, this is starting to work a little better
image

Turns out there's piles of edge cases. Seemingly different things zorder in different ways, which is complicating this. Duping everything to this thread may not have been the most wise ;___;


Changed the reparenting to the root terminal HWND instead of the xaml island, and we get:

image

so GetAncestor(GetConsoleWindow(), GA_ROOT) can work the same for WT and conhost

See also:

Scenarios:

@zadjii-msft
Copy link
Member

zadjii-msft commented Mar 28, 2022

Misc notes:

    gci.ProcessHandleList.ModifyConsoleProcessFocus(WI_IsFlagSet(gci.Flags, CONSOLE_HAS_FOCUS));

    ...

    // ModifyConsoleProcessFocus calls _ModifyProcessForegroundRights

    ...

void ConsoleProcessList::_ModifyProcessForegroundRights(const HANDLE hProcess, const bool fForeground) const
{
    LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(hProcess, fForeground));
}

FocusIn/FocusOut

FocusIn/FocusOut can be combined with any of the mouse events since it
uses a different protocol. When set, it causes xterm to send CSI I
when the terminal gains focus, and CSI O when it loses focus.

The easiest prototype may just be always passing true here in conpty mode.


graph TD
    id1[ \u2705Allow windows created by console <br> apps to appear above the Terminal #12799] --> id3[ \u2705Check if FG window is owner #12899]
    id2[ \u2705 Enable the Terminal to tell ConPTY <br> who the owner is #12526] --> id3
    id3 --> id4[ \u2705FocusEvents in conhost, terminal, #12900]  --> id6
    id5[ \u2705 Propagate show/hide window calls against <br> the ConPTY pseudo window to the Terminal #12515] --> id6{{Terminal v1.14}}
    click id1 "http://www.github.com/microsoft/terminal/issues/12799"
    click id2 "http://www.github.com/microsoft/terminal/issues/12526"
    click id3 "http://www.github.com/microsoft/terminal/issues/12899"
    click id4 "http://www.github.com/microsoft/terminal/issues/12900"
    click id5 "http://www.github.com/microsoft/terminal/issues/12515"
Loading

April 7 status update: flowchart above has dependency tree. Gonna stack the PRs in the bubbles on top of #12799, once #12526 merges. We won't have time to review that this week though.

April 27th update: all the above PRs have signoffs &/|| are merged. Should all be available in 1.14.

@zadjii-msft zadjii-msft assigned zadjii-msft and unassigned miniksa Mar 31, 2022
DHowett pushed a commit that referenced this issue Apr 12, 2022
## Window shenanigans, part the first:

This PR enables terminals to tell ConPTY what the owning window for the
pseudo window should be. This allows thigs like MessageBoxes created by
console applications to work. It also enables console apps to use
`GetAncestor(GetConsoleWindow(), GA_ROOT)`  to get directly at the HWND
of the Terminal (but _don't please_).

This is tested with our internal partners and seems to work for their
scenario. 

See #2988, #12799, #12515, #12570.

## PR Checklist
This is 1/3 of #2988.
zadjii-msft added a commit that referenced this issue Apr 19, 2022
…12799)

## Window shenanigans, part the third:

Hooks the Terminal's focus state up to the underlying ConPTY. This is LOAD BEARING for allowing windows created by console applications to bring themselves to the foreground.

We're using the [FocusIn/FocusOut](https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h3-FocusIn_FocusOut) sequences to communicate to ConPTY when a control gains/loses focus. Theoretically, other terminals could do this as well.

## References

#11682 tracks _real_ support for this sequence in Console & conpty. When we do that, we should consider even if a client application disables this mode, the Terminal & conpty should always request this from the hosting terminal (and just ignore internally to ConPTY).

See also #12515, #12526, which are the other two parts of this effort. This was tested with all three merged together, and they worked beautifully for all our scenarios. They are kept separate for ease of review.

## PR Checklist
* [x] This is prototype 3 for #2988
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

This allows windows spawned by console processes to bring themselves to the foreground _when the console is focused_. (Historically, this is also called in the WndProc, when focus changes).

Notably, before this, ConPTY was _never_ focused, so windows could never bring themselves to the foreground when run from a ConPTY console. We're not blanket granting the SetForeground right to all console apps when run in ConPTY. It's the responsibility of the hosting terminal emulator to always tell ConPTY when a particular instance is focused.

## Validation Steps Performed

(gif below)
zadjii-msft added a commit that referenced this issue Apr 19, 2022
#### ⚠️ _Targets #12799_ ⚠️

This is an atomic bit of code that partners with #12799. It's separated as an individual PR to keep diffs more simple. 

This ensures that when a terminal tells ConPTY that it's focused, that ConPTY doesn't do the `ConsoleControl(CONSOLE_FOREGROUND` thing unless the terminal application is actually in the foreground. This prevents a trivial exploit whereby a `malicious.exe` could create a PTY, tell ConPTY it has focus (when it doesn't), then use this mechanism to launch an instance of itself into the foreground. 

When the terminal tells us it's in the foreground, we're gonna look at the owner of the ConPTY window handle. If that owner has focus, then cool, this is allowed. Otherwise, we won't grant them the FG right. For this to work, the terminal just have already called `ReparentPseudoConsole`.

* built on top of #12799 and #12526
* [x] Part of #2988
* [x] Tested manually.
ghost pushed a commit that referenced this issue Apr 20, 2022
Further builds on #12799. #12799 assumes that the connection is prepared to receive FocusIn/FocusOut events as input. For ConPTY we can be relatively sure of that, but that's not _technically_ correct. In the hypothetical world where the connection is not a ConPTY connection, then the other side might not be expecting those sequences. 

This remedies the issue by
* ConPTY will always request focus event mode (from the terminal) when it starts up
* when a client tries to disable focus events in conpty, conpty is gonna note that internally, but never transmit that to the hosting terminal, to leave the terminal in focus event mode.
* `TerminalDispatch` and `ControlCore` are hooked up now to only send focus events when the Terminal is in focus event mode (which will be always for conpty)
* At this point, it was like, 4LOC in `terminalInput.cpp` to add support for focus events to conhost as well.

## checklist
* [x] closes #11682
  * This combined with #12515 will finally close out #2988 as well, but we can do that manually.
* [x] I work here
* [ ] There aren't tests for this. There probably should be.
@zadjii-msft zadjii-msft added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Apr 28, 2022
@zadjii-msft
Copy link
Member

Alrighty. With #12515, #12526, #12799, #12899, #12900 all merged, I think this one's done. Everything I found linked to this thread seems to work in main. At this point, any new issues we discover in 1.14+ will be something else entirely, so we should give them new threads.

Thanks for the patience here folks!

zadjii-msft added a commit that referenced this issue Apr 28, 2022
I was starting to port over the focus & show/hide & reparent things from #2988, but this isn't it. I know sufficiently little about how this works, that I won't be able to do this. Additionally, we don't own the actual WPF conpty connection (VS does), so I can't just implement it here.
@zaaj
Copy link

zaaj commented May 28, 2024

Terminal version 1.19 on Windows 11 here almost half way through 2024 and this is still a problem - Microsoft Graph login-prompt windows usually show up behind the Terminal window that launches them, I also have intermittent issues with Get-Credential in windows powershell 5.1 (pwsh 7 not having all the functionality I use on a day-to-day basis, I'm still feeling stuck on 5 - plus, writing my scripts for 5 makes them more sharable with my less-powershell-savvy colleagues, and is easier to run them on servers where we don't want to install any software not needed, including Pwsh 7).

From a technical programming standpoint, I can understand a ConPTY process not having a console window, but it is obviously being show IN a window, which DOES have a window handle. With 5 PRs all trying to attempt to resolve this, it would appear to be a hard-to-fully-resolve issue. I appreciate that work had been done towards resolving this issue, but it does not appear to be resolved yet. Please keep working on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. zInbox-Bug Ignore me!
Projects
None yet
Development

No branches or pull requests