Repository: microsoft/terminal (language: cpp)

Issue:
`[O` erroneously inputted after exiting from Neovim 0.7.0 in latest Terminal Preview
### Windows Terminal version

1.14.1451.0

### Windows build number

10.0.19044.1706

### Other Software

Neovim 0.7.0 (https://github.com/neovim/neovim/releases/tag/v0.7.0)

### Steps to reproduce

1. Install Neovim
2. Open it by running `nvim` in your shell (works both in `cmd` and `pwsh`)
3. exit it (`:q`)

### Expected Behavior

Neovim exits, nothing else happens.

### Actual Behavior

Neovim exits, and the shell receives `[O` on its console input.

![image](https://user-images.githubusercontent.com/6414091/172247280-950814dc-16d4-4ff2-b46b-2dce47365c62.png)

My guess would be that the characters are part of a response to some VT escape code from Neovim, but for some reason they get written separately and the shell gets them instead of Neovim.


Discussion / hints:
Is it `[0` or `[O` (the number, or the letter)/? I could very much see it being the letter O, from #12900 
Oh, you're right, `[O`, sorry. :)
Hmm. Brainstorming, not debugging. 

* Conpty always requests Focus events, Terminal always sends them. 
* The client app still needs to ask for focus events to actually get the `\x1b[I` `\x1b[O` on input
* neovim must be asking for them, but not turning off that request on exit?
* Alternatively maybe focus events mode needs to be reset on a soft reset, hard reset, alt buffer leave - something that neovim is doing on exit? 

that's where I should start looking. What does neovim do on exit.
Just to add one datapoint, latest Alacritty handles it correctly, not sure if it supports Focus events.
FYI, I had a brief look at this, and Neovim does turn off focus events on exit, but prior to that happening, it appears that someone is calling `WriteConsoleInput` with a `FOCUS_EVENT` record, and that's what triggers that final report. It's not the result of a `WM_SETFOCUS` or `WM_KILLFOCUS` event.
It looks like there was an issue open on Neovim for something similar: neovim/neovim#17985
That says a fix has been merged in but trying the latest Neovim nightly still has the issue for me in Terminal, so it may not be related.
_will continue adding notes here..._

> FYI, I had a brief look at this, and Neovim does turn off focus events on exit, but prior to that happening, it appears that someone is calling `WriteConsoleInput` with a `FOCUS_EVENT` record, and that's what triggers that final report. It's not the result of a `WM_SETFOCUS` or `WM_KILLFOCUS` event.

Thanks for that! That really made this investigation WAY easier. 

```
>	OpenConsole.exe!_WriteConsoleInputWImplHelper(InputBuffer & context, std::deque<std::unique_ptr<IInputEvent,std::default_delete<IInputEvent>>,std::allocator<std::unique_ptr<IInputEvent,std::default_delete<IInputEvent>>>> & events, unsigned __int64 & written, const bool append) Line 429	C++
 	OpenConsole.exe!ApiRoutines::WriteConsoleInputWImpl(InputBuffer & context, const gsl::span<_INPUT_RECORD const ,-1> buffer, unsigned __int64 & written, const bool append) Line 514	C++
 	OpenConsole.exe!ApiDispatchers::ServerWriteConsoleInput(_CONSOLE_API_MSG * const m, int * const __formal) Line 894	C++
 	OpenConsole.exe!ApiSorter::ConsoleDispatchRequest(_CONSOLE_API_MSG * Message) Line 178	C++
 	OpenConsole.exe!IoDispatchers::ConsoleDispatchRequest(_CONSOLE_API_MSG * pMessage) Line 577	C++
 	OpenConsole.exe!IoSorter::ServiceIoOperation(_CONSOLE_API_MSG * const pMsg, _CONSOLE_API_MSG * * ReplyMsg) Line 33	C++
 	OpenConsole.exe!ConsoleIoThread(void * lpParameter) Line 978	C++

```

`m.Descriptor.Process` is a `ConsoleProcessHandle*` in this console.

`Microsoft::Console::Interactivity::ServiceLocator::s_globals.ciConsoleInformation.ProcessHandleList` has the processes. 

The 0th process in the list is the one that sent it

![image](https://user-images.githubusercontent.com/18356694/172668174-afc2ade4-c50c-4fc3-a11f-868922c7925c.png)

Looks like the message _is_ coming from neovim. 

  | Name | Value | Type
-- | -- | -- | --
  | context.InputMode | 0x00000208 | unsigned long
◢ | context._termInput._inputMode | {_data=0x0000000000000201 } | til::enumset<enum Microsoft::Console::VirtualTerminal::TerminalInput::Mode,unsigned __int64>



`0x0201` is `Mode::FocusEvent | Mode::Ansi`, sure. 

`0x208` is `ENABLE_VIRTUAL_TERMINAL_INPUT | ENABLE_WINDOW_INPUT`

So, the console is in VT input mode, and is expecting the client can accept focus events. So this must be happening before neovim disables focus events mode... 

Yep. Sticking a breakpoint in `AdaptDispatch::_SetInputMode`, when `mode == TerminalInput::Mode::FocusEvent` will get hit after the `WriteConsoleInput` one is resumed. So it definitely seems like the FOCUS_EVENT is getting written _before_ they disable the focus event mode. Maybe they just need to drain all the pending console input?
_neovim is sending a win32 console focus event right as it exits?_ I can't fathom why they'd do that: a synthetic focus event doesn't even say anything about the state of the world -- the absolute best it can accomplish is making a console app think it's in focus when it actually doesn't have **win32 focus**...
Yea why are they doing that? 

Value | Meaning
-- | --
FOCUS_EVENT 0x0010 | The Event member contains a [FOCUS_EVENT_RECORD](https://docs.microsoft.com/en-us/windows/console/focus-event-record-str) structure. These events are used internally and should be ignored.

I guess that doesn't explicitly state "don't use this", but _why_
@justinmk any chances you know how/why neovim would be doing this? I reckon this is something that should get moved over to neovim, but I just want to be sure. 
Probably not intentional.

> Maybe they just need to drain all the pending console input?

Maybe.

If someone decides to create an issue at Nvim, please reference this issue.
Filed https://github.com/neovim/neovim/issues/18899 for investigation. 

Theoretically, we might be able to route input in such a way that focus events from the API don't hit this path. Ultimately, I am curious why this is being sent in the first place. That might affect the route we want to go in mitigating this. 
Hmm. Considering this is something who's root cause is up in libuv (https://github.com/neovim/neovim/issues/18899#issuecomment-1150456895), maybe we _do_ need to think about this. 
@zadjii-msft I'm quite partial to your idea of not sending the `CSI I` and `CSI O` reports when the focus events come from an API call. 

If you look at the `FocusEvent` constructors, there's one that takes a `FOCUS_EVENT_RECORD` and one that takes a `bool`. As far as I can see, if it's constructed from an API call it'll use the former, and if it's the result of an actual focus change event it'll use the latter.

So I was thinking we could add a flag in `FocusEvent` that records whether it came from an input record, and then in `TerminalInput`, where we process those events, we just don't generate a focus report if that flag is set.
Note that this fix works for both conhost and Terminal. In the case of conhost, the genuine `FocusEvent` records are generated in the [`HandleFocusEvent`](https://github.com/microsoft/terminal/blob/57c3953aca49f68acc480ca27533fdc7aad78b43/src/host/input.cpp#L197) function, and for Terminal, they're generated in [`InteractDispatch`](https://github.com/microsoft/terminal/blob/ed27737233714dea77877624d1beeb49e2ccd36e/src/terminal/adapter/InteractDispatch.cpp#L240).
Sync'd with Dustin, I think we're both cool with the `bool didIComeFromTheAPI` solution. It's bodgy, but it'll work. We don't want to just entirely filter out Focus messages from the API, cause clearly there is a reason that libuv is sending them. And the boolean is probably the simplest way to get this fixed. 

Fix patch:
diff --git a/.github/actions/spelling/allow/allow.txt b/.github/actions/spelling/allow/allow.txt
index 60b57274c84..43f6b97325d 100644
--- a/.github/actions/spelling/allow/allow.txt
+++ b/.github/actions/spelling/allow/allow.txt
@@ -44,6 +44,7 @@ inlined
 It'd
 kje
 libfuzzer
+libuv
 liga
 lje
 Llast
diff --git a/src/terminal/adapter/ut_adapter/inputTest.cpp b/src/terminal/adapter/ut_adapter/inputTest.cpp
index 070e2f5d053..ace7de68019 100644
--- a/src/terminal/adapter/ut_adapter/inputTest.cpp
+++ b/src/terminal/adapter/ut_adapter/inputTest.cpp
@@ -38,6 +38,7 @@ class Microsoft::Console::VirtualTerminal::InputTest
     static void s_TerminalInputTestNullCallback(_In_ std::deque<std::unique_ptr<IInputEvent>>& inEvents);
 
     TEST_METHOD(TerminalInputTests);
+    TEST_METHOD(TestFocusEvents);
     TEST_METHOD(TerminalInputModifierKeyTests);
     TEST_METHOD(TerminalInputNullKeyTests);
     TEST_METHOD(DifferentModifiersTest);
@@ -300,10 +301,62 @@ void InputTest::TerminalInputTests()
     inputEvent = IInputEvent::Create(irUnhandled);
     VERIFY_ARE_EQUAL(false, pInput->HandleKey(inputEvent.get()), L"Verify MENU_EVENT was NOT handled.");
 
-    Log::Comment(L"Testing FOCUS_EVENT");
-    irUnhandled.EventType = FOCUS_EVENT;
-    inputEvent = IInputEvent::Create(irUnhandled);
-    VERIFY_ARE_EQUAL(false, pInput->HandleKey(inputEvent.get()), L"Verify FOCUS_EVENT was NOT handled.");
+    // Testing FOCUS_EVENTs is handled by TestFocusEvents
+}
+
+void InputTest::TestFocusEvents()
+{
+    // GH#12900, #13238
+    // Focus events that come in from the API should never be translated to VT sequences.
+    // We're relying on the fact that the INPUT_RECORD version of the ctor is only called by the API
+    const auto pInput = new TerminalInput(s_TerminalInputTestCallback);
+
+    INPUT_RECORD irTest = { 0 };
+    irTest.EventType = FOCUS_EVENT;
+
+    {
+        irTest.Event.FocusEvent.bSetFocus = false;
+        auto inputEvent = IInputEvent::Create(irTest);
+        VERIFY_ARE_EQUAL(false, pInput->HandleKey(inputEvent.get()), L"Verify FOCUS_EVENT from API was NOT handled.");
+    }
+    {
+        irTest.Event.FocusEvent.bSetFocus = true;
+        auto inputEvent = IInputEvent::Create(irTest);
+        VERIFY_ARE_EQUAL(false, pInput->HandleKey(inputEvent.get()), L"Verify FOCUS_EVENT from API was NOT handled.");
+    }
+    {
+        auto inputEvent = std::make_unique<FocusEvent>(false);
+        VERIFY_ARE_EQUAL(false, pInput->HandleKey(inputEvent.get()), L"Verify FocusEvent from any other source was NOT handled.");
+    }
+    {
+        auto inputEvent = std::make_unique<FocusEvent>(true);
+        VERIFY_ARE_EQUAL(false, pInput->HandleKey(inputEvent.get()), L"Verify FocusEvent from any other source was NOT handled.");
+    }
+
+    Log::Comment(L"Enable focus event handling");
+
+    pInput->SetInputMode(TerminalInput::Mode::FocusEvent, true);
+
+    {
+        irTest.Event.FocusEvent.bSetFocus = false;
+        auto inputEvent = IInputEvent::Create(irTest);
+        VERIFY_ARE_EQUAL(false, pInput->HandleKey(inputEvent.get()), L"Verify FOCUS_EVENT from API was NOT handled.");
+    }
+    {
+        irTest.Event.FocusEvent.bSetFocus = true;
+        auto inputEvent = IInputEvent::Create(irTest);
+        VERIFY_ARE_EQUAL(false, pInput->HandleKey(inputEvent.get()), L"Verify FOCUS_EVENT from API was NOT handled.");
+    }
+    {
+        s_expectedInput = L"\x1b[O";
+        auto inputEvent = std::make_unique<FocusEvent>(false);
+        VERIFY_ARE_EQUAL(true, pInput->HandleKey(inputEvent.get()), L"Verify FocusEvent from any other source was handled.");
+    }
+    {
+        s_expectedInput = L"\x1b[I";
+        auto inputEvent = std::make_unique<FocusEvent>(true);
+        VERIFY_ARE_EQUAL(true, pInput->HandleKey(inputEvent.get()), L"Verify FocusEvent from any other source was handled.");
+    }
 }
 
 void InputTest::TerminalInputModifierKeyTests()
diff --git a/src/terminal/input/terminalInput.cpp b/src/terminal/input/terminalInput.cpp
index d06a5e0e0b0..79b5ef74f5f 100644
--- a/src/terminal/input/terminalInput.cpp
+++ b/src/terminal/input/terminalInput.cpp
@@ -527,6 +527,14 @@ bool TerminalInput::HandleKey(const IInputEvent* const pInEvent)
     if (pInEvent->EventType() == InputEventType::FocusEvent)
     {
         const auto& focusEvent = *static_cast<const FocusEvent* const>(pInEvent);
+
+        // BODGY
+        // GH#13238 - Filter out focus events that came from the API.
+        if (focusEvent.CameFromApi())
+        {
+            return false;
+        }
+
         return HandleFocus(focusEvent.GetFocus());
     }
 
diff --git a/src/types/FocusEvent.cpp b/src/types/FocusEvent.cpp
index d850fff50fd..1b194842a5f 100644
--- a/src/types/FocusEvent.cpp
+++ b/src/types/FocusEvent.cpp
@@ -1,5 +1,18 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
+//
+// BODGY: GH#13238
+//
+// It appears that some applications (libuv) like to send a FOCUS_EVENT_RECORD
+// as a way to jiggle the input handle. Focus events really shouldn't ever be
+// sent via the API, they don't really do anything internally. However, focus
+// events in the input buffer do get translated by the TerminalInput to VT
+// sequences if we're in the right input mode.
+//
+// To not prevent libuv from jiggling the handle with a focus event, and also
+// make sure that we don't erroneously translate that to a sequence of
+// characters, we're going to filter out focus events that came from the API
+// when translating to VT.
 
 #include "precomp.h"
 #include "inc/IInputEvent.hpp"
diff --git a/src/types/inc/IInputEvent.hpp b/src/types/inc/IInputEvent.hpp
index d98f176795a..74258454836 100644
--- a/src/types/inc/IInputEvent.hpp
+++ b/src/types/inc/IInputEvent.hpp
@@ -514,12 +514,14 @@ class FocusEvent : public IInputEvent
 {
 public:
     constexpr FocusEvent(const FOCUS_EVENT_RECORD& record) :
-        _focus{ !!record.bSetFocus }
+        _focus{ !!record.bSetFocus },
+        _cameFromApi{ true }
     {
     }
 
     constexpr FocusEvent(const bool focus) :
-        _focus{ focus }
+        _focus{ focus },
+        _cameFromApi{ false }
     {
     }
 
@@ -539,8 +541,15 @@ class FocusEvent : public IInputEvent
 
     void SetFocus(const bool focus) noexcept;
 
+    // BODGY - see FocusEvent.cpp for details.
+    constexpr bool CameFromApi() const noexcept
+    {
+        return _cameFromApi;
+    }
+
 private:
     bool _focus;
+    bool _cameFromApi;
 
 #ifdef UNIT_TESTING
     friend std::wostream& operator<<(std::wostream& stream, const FocusEvent* const pFocusEvent);


Test patch: