From ee6060b3a4106cac0d9b220023fa002a68a8fc41 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 24 Nov 2025 09:35:52 -0800 Subject: [PATCH 1/3] Fix search not scrolling to result past view (#19571) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary of the Pull Request Fixes a bug where search would not scroll to results just below the viewport. This was caused by code intended to scroll the search result in such a way that it isn't covered by the search box. The scroll offset is calculated in `TermControl::_calculateSearchScrollOffset()` then handed down in the `SearchRequest` when conducting a search. This would get to `Terminal::ScrollToSearchHighlight()` where the offset is applied to the search result's position so that we would scroll to the adjusted position. The adjustment was overly aggressive in that it would apply it to both "start" and "end". In reality, we don't need to apply it to "end" because it wouldn't be covered by the search box (we only scroll to end if it's past the end of the current view anyways). The fix applies the adjustment only to "start" and only does so if it's actually in the first few rows that would be covered by the search box. That unveiled another bug where `Terminal::_ScrollToPoints()` would also be too aggressive about scrolling the "end" into view. In some testing, it would generally end up scrolling to the end of the buffer. To fix this cascading bug, I just had `_ScrollToPoints()` just call `Terminal::_ScrollToPoint()` (singular, not plural) which is consistently used throughout the Terminal code for selection (so it's battle tested). `_ScrollToPoints()` was kept since it's still used for accessibility when selecting a new region to keep the new selection in view. It's also just a nice wrapper that ensures a range is visible (or at least as much as it could be). ## References and Relevant Issues Scroll offset was added in #17516 ## Validation Steps Performed ✅ search results that would be covered by the search box are still adjusted ✅ search results that are past the end of the view become visible ✅ UIA still selects properly and brings the selection into view ## PR Checklist Duncan reported this bug internally, but there doesn't seem to be one on the repo. --- src/cascadia/TerminalCore/Terminal.cpp | 12 +++++++++--- .../TerminalCore/terminalrenderdata.cpp | 19 ++----------------- 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index b6965e3037..aec06719d7 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -1297,9 +1297,15 @@ void Terminal::ScrollToSearchHighlight(til::CoordType searchScrollOffset) if (_searchHighlightFocused < _searchHighlights.size()) { const auto focused = til::at(_searchHighlights, _searchHighlightFocused); - const auto adjustedStart = til::point{ focused.start.x, std::max(0, focused.start.y - searchScrollOffset) }; - const auto adjustedEnd = til::point{ focused.end.x, std::max(0, focused.end.y - searchScrollOffset) }; - _ScrollToPoints(adjustedStart, adjustedEnd); + + // Only adjust the y coordinates if "start" is in a row that would be covered by the search box + auto adjustedStart = focused.start; + const auto firstVisibleRow = _VisibleStartIndex(); + if (focused.start.y > firstVisibleRow && focused.start.y < firstVisibleRow + searchScrollOffset) + { + adjustedStart.y = std::max(0, focused.start.y - searchScrollOffset); + } + _ScrollToPoints(adjustedStart, focused.end); } } diff --git a/src/cascadia/TerminalCore/terminalrenderdata.cpp b/src/cascadia/TerminalCore/terminalrenderdata.cpp index f2a9087a90..82f539f18b 100644 --- a/src/cascadia/TerminalCore/terminalrenderdata.cpp +++ b/src/cascadia/TerminalCore/terminalrenderdata.cpp @@ -152,29 +152,14 @@ const til::point_span* Terminal::GetSearchHighlightFocused() const noexcept // - The updated scroll offset til::CoordType Terminal::_ScrollToPoints(const til::point coordStart, const til::point coordEnd) { - auto notifyScrollChange = false; if (coordStart.y < _VisibleStartIndex()) { - // recalculate the scrollOffset - _scrollOffset = ViewStartIndex() - coordStart.y; - notifyScrollChange = true; + _ScrollToPoint(coordStart); } else if (coordEnd.y > _VisibleEndIndex()) { - // recalculate the scrollOffset, note that if the found text is - // beneath the current visible viewport, it may be within the - // current mutableViewport and the scrollOffset will be smaller - // than 0 - _scrollOffset = std::max(0, ViewStartIndex() - coordStart.y); - notifyScrollChange = true; + _ScrollToPoint(coordEnd); } - - if (notifyScrollChange) - { - _activeBuffer().TriggerScroll(); - _NotifyScrollEvent(); - } - return _VisibleStartIndex(); } From 1ca0c76bc74012fdd1ff6211b5c8f389a4efd9b4 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 24 Nov 2025 19:01:21 +0100 Subject: [PATCH 2/3] Fix a WPF<>TSF crash by avoiding TF_TMAE_CONSOLE (#19584) As explained in detail in the diff. Closes #19562 --- .github/actions/spelling/expect/expect.txt | 7 +++- src/cascadia/TerminalControl/HwndTerminal.cpp | 5 +++ src/cascadia/TerminalControl/HwndTerminal.hpp | 1 + .../dll/Microsoft.Terminal.Control.def | 1 + .../WpfTerminalControl/NativeMethods.cs | 3 ++ .../WpfTerminalControl/TerminalContainer.cs | 6 ++- src/tsf/Handle.cpp | 5 +++ src/tsf/Handle.h | 1 + src/tsf/Implementation.cpp | 42 +++++++++++++++++-- src/tsf/Implementation.h | 2 +- 10 files changed, 67 insertions(+), 6 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index ede4229f16..932611291a 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -1091,6 +1091,8 @@ NEXTLINE nfe NLSMODE NOACTIVATE +NOACTIVATEKEYBOARDLAYOUT +NOACTIVATETIP NOAPPLYNOW NOCLIP NOCOMM @@ -1504,7 +1506,6 @@ scrolllock scrolloffset SCROLLSCALE SCROLLSCREENBUFFER -scursor sddl SDKDDK segfault @@ -1703,9 +1704,11 @@ TEXTMETRIC TEXTMETRICW textmode texttests +TFCAT THUMBPOSITION THUMBTRACK tilunittests +TIPCAP titlebars TITLEISLINKNAME TLDP @@ -1757,6 +1760,8 @@ UIACCESS uiacore uiautomationcore uielem +UIELEMENTENABLED +UIELEMENTENABLEDONLY UINTs uld uldash diff --git a/src/cascadia/TerminalControl/HwndTerminal.cpp b/src/cascadia/TerminalControl/HwndTerminal.cpp index 58c9f34e4e..f4cfb967ae 100644 --- a/src/cascadia/TerminalControl/HwndTerminal.cpp +++ b/src/cascadia/TerminalControl/HwndTerminal.cpp @@ -462,6 +462,11 @@ void HwndTerminal::SendOutput(std::wstring_view data) _terminal->Write(data); } +void _stdcall AvoidBuggyTSFConsoleFlags() +{ + Microsoft::Console::TSF::Handle::AvoidBuggyTSFConsoleFlags(); +} + HRESULT _stdcall CreateTerminal(HWND parentHwnd, _Out_ void** hwnd, _Out_ void** terminal) { auto publicTerminal = std::make_unique(parentHwnd); diff --git a/src/cascadia/TerminalControl/HwndTerminal.hpp b/src/cascadia/TerminalControl/HwndTerminal.hpp index 4d41741952..68ffc73ec7 100644 --- a/src/cascadia/TerminalControl/HwndTerminal.hpp +++ b/src/cascadia/TerminalControl/HwndTerminal.hpp @@ -41,6 +41,7 @@ typedef struct _TerminalTheme } TerminalTheme, *LPTerminalTheme; extern "C" { +__declspec(dllexport) void _stdcall AvoidBuggyTSFConsoleFlags(); __declspec(dllexport) HRESULT _stdcall CreateTerminal(HWND parentHwnd, _Out_ void** hwnd, _Out_ void** terminal); __declspec(dllexport) void _stdcall TerminalSendOutput(void* terminal, LPCWSTR data); __declspec(dllexport) void _stdcall TerminalRegisterScrollCallback(void* terminal, void __stdcall callback(int, int, int)); diff --git a/src/cascadia/TerminalControl/dll/Microsoft.Terminal.Control.def b/src/cascadia/TerminalControl/dll/Microsoft.Terminal.Control.def index c02c0721ab..60ce76f1a9 100644 --- a/src/cascadia/TerminalControl/dll/Microsoft.Terminal.Control.def +++ b/src/cascadia/TerminalControl/dll/Microsoft.Terminal.Control.def @@ -4,6 +4,7 @@ EXPORTS DllGetActivationFactory = WINRT_GetActivationFactory PRIVATE ; Flat C ABI + AvoidBuggyTSFConsoleFlags CreateTerminal DestroyTerminal TerminalCalculateResize diff --git a/src/cascadia/WpfTerminalControl/NativeMethods.cs b/src/cascadia/WpfTerminalControl/NativeMethods.cs index fdd42a1450..d5839accc3 100644 --- a/src/cascadia/WpfTerminalControl/NativeMethods.cs +++ b/src/cascadia/WpfTerminalControl/NativeMethods.cs @@ -171,6 +171,9 @@ namespace Microsoft.Terminal.Wpf SWP_SHOWWINDOW = 0x0040, } + [DllImport("Microsoft.Terminal.Control.dll", CharSet = CharSet.Unicode, CallingConvention = CallingConvention.StdCall, ExactSpelling = true)] + public static extern void AvoidBuggyTSFConsoleFlags(); + [DllImport("Microsoft.Terminal.Control.dll", CharSet = CharSet.Unicode, CallingConvention = CallingConvention.StdCall, PreserveSig = false)] public static extern void CreateTerminal(IntPtr parent, out IntPtr hwnd, out IntPtr terminal); diff --git a/src/cascadia/WpfTerminalControl/TerminalContainer.cs b/src/cascadia/WpfTerminalControl/TerminalContainer.cs index 547338f909..ffe11ea231 100644 --- a/src/cascadia/WpfTerminalControl/TerminalContainer.cs +++ b/src/cascadia/WpfTerminalControl/TerminalContainer.cs @@ -11,7 +11,6 @@ namespace Microsoft.Terminal.Wpf using System.Windows.Automation.Peers; using System.Windows.Interop; using System.Windows.Media; - using System.Windows.Threading; /// /// The container class that hosts the native hwnd terminal. @@ -32,6 +31,11 @@ namespace Microsoft.Terminal.Wpf /// public TerminalContainer() { + // WPF & TSF can't deal with us setting TF_TMAE_CONSOLE on the UI thread. + // It simply crashes on Windows 10 if you use the Emoji picker. + // (On later versions of Windows it just doesn't work.) + NativeMethods.AvoidBuggyTSFConsoleFlags(); + this.MessageHook += this.TerminalContainer_MessageHook; this.GotFocus += this.TerminalContainer_GotFocus; this.Focusable = true; diff --git a/src/tsf/Handle.cpp b/src/tsf/Handle.cpp index e60e251307..b7cae1d1be 100644 --- a/src/tsf/Handle.cpp +++ b/src/tsf/Handle.cpp @@ -16,6 +16,11 @@ Handle Handle::Create() return handle; } +void Handle::AvoidBuggyTSFConsoleFlags() +{ + Implementation::AvoidBuggyTSFConsoleFlags(); +} + void Handle::SetDefaultScopeAlphanumericHalfWidth(bool enable) { Implementation::SetDefaultScopeAlphanumericHalfWidth(enable); diff --git a/src/tsf/Handle.h b/src/tsf/Handle.h index 7e2e912574..c3be18ba93 100644 --- a/src/tsf/Handle.h +++ b/src/tsf/Handle.h @@ -33,6 +33,7 @@ namespace Microsoft::Console::TSF struct Handle { static Handle Create(); + static void AvoidBuggyTSFConsoleFlags(); static void SetDefaultScopeAlphanumericHalfWidth(bool enable); Handle() = default; diff --git a/src/tsf/Implementation.cpp b/src/tsf/Implementation.cpp index 7d0e2dfd34..99ea34c01c 100644 --- a/src/tsf/Implementation.cpp +++ b/src/tsf/Implementation.cpp @@ -27,9 +27,45 @@ static void TfPropertyvalClose(TF_PROPERTYVAL* val) } using unique_tf_propertyval = wil::unique_struct; +// The flags passed to ActivateEx don't replace the flags during previous calls. +// Instead, they're additive. So, if we pass flags that some concurrently running +// TSF clients don't expect we may blow them up accidentally. +// +// Such is the case with WPF (and TSF, which is actually at fault there). +// If you pass TF_TMAE_CONSOLE it'll instantly crash on Windows 10 on first text input. +// On Windows 11 it'll at least not crash but still make emoji input completely non-functional. +// +// -------- +// +// In any case, we pass the same flags as conhost v1: +// - TF_TMAE_UIELEMENTENABLEDONLY: TSF activates only text services that are +// categorized in GUID_TFCAT_TIPCAP_UIELEMENTENABLED. +// - TF_TMAE_NOACTIVATEKEYBOARDLAYOUT: TSF does not sync the current keyboard layout +// while this method is called. The keyboard layout will be adjusted when the +// calling thread gets focus. This flag must be used with TF_TMAE_NOACTIVATETIP. +// - TF_TMAE_CONSOLE: A text service is activated for console usage. +// Some IMEs are known to use this as a hint. Particularly a Korean IME can benefit +// from this, because Korean relies on "recomposing" previously finished compositions. +// That can't work in a terminal, since we submit composed text to the shell immediately. +// +// I'm not sure what TF_TMAE_UIELEMENTENABLEDONLY does. I tried to figure it out but failed. +// +// For TF_TMAE_NOACTIVATEKEYBOARDLAYOUT, I'm 99% sure it doesn't do anything, including in +// conhost v1. This is because IMM will be initialized on WM_ACTIVATE, which calls ActivateEx(0). +// Any subsequent ActivateEx() calls will update the flags, _except_ for this one and +// TF_TMAE_NOACTIVATETIP which are explicitly filtered out. +// +// TF_TMAE_NOACTIVATETIP however is important. Without it, TIPs are immediately initialized. +static std::atomic s_activationFlags{ TF_TMAE_NOACTIVATETIP | TF_TMAE_UIELEMENTENABLEDONLY | TF_TMAE_NOACTIVATEKEYBOARDLAYOUT | TF_TMAE_CONSOLE }; +void Implementation::AvoidBuggyTSFConsoleFlags() noexcept +{ + s_activationFlags.fetch_and(~static_cast(TF_TMAE_CONSOLE), std::memory_order_relaxed); +} + +static std::atomic s_wantsAnsiInputScope{ false }; void Implementation::SetDefaultScopeAlphanumericHalfWidth(bool enable) noexcept { - _wantsAnsiInputScope.store(enable, std::memory_order_relaxed); + s_wantsAnsiInputScope.store(enable, std::memory_order_relaxed); } void Implementation::Initialize() @@ -40,7 +76,7 @@ void Implementation::Initialize() // There's no point in calling TF_GetThreadMgr. ITfThreadMgr is a per-thread singleton. _threadMgrEx = wil::CoCreateInstance(CLSID_TF_ThreadMgr, CLSCTX_INPROC_SERVER); - THROW_IF_FAILED(_threadMgrEx->ActivateEx(&_clientId, TF_TMAE_CONSOLE)); + THROW_IF_FAILED(_threadMgrEx->ActivateEx(&_clientId, s_activationFlags.load(std::memory_order_relaxed))); THROW_IF_FAILED(_threadMgrEx->CreateDocumentMgr(_documentMgr.addressof())); TfEditCookie ecTextStore; @@ -319,7 +355,7 @@ STDMETHODIMP Implementation::GetWnd(HWND* phwnd) noexcept STDMETHODIMP Implementation::GetAttribute(REFGUID rguidAttribute, VARIANT* pvarValue) noexcept { - if (_wantsAnsiInputScope.load(std::memory_order_relaxed) && IsEqualGUID(rguidAttribute, GUID_PROP_INPUTSCOPE)) + if (s_wantsAnsiInputScope.load(std::memory_order_relaxed) && IsEqualGUID(rguidAttribute, GUID_PROP_INPUTSCOPE)) { _ansiInputScope.AddRef(); pvarValue->vt = VT_UNKNOWN; diff --git a/src/tsf/Implementation.h b/src/tsf/Implementation.h index fecec35cea..82e84ab888 100644 --- a/src/tsf/Implementation.h +++ b/src/tsf/Implementation.h @@ -16,6 +16,7 @@ namespace Microsoft::Console::TSF struct Implementation : ITfContextOwner, ITfContextOwnerCompositionSink, ITfTextEditSink { + static void AvoidBuggyTSFConsoleFlags() noexcept; static void SetDefaultScopeAlphanumericHalfWidth(bool enable) noexcept; virtual ~Implementation() = default; @@ -129,6 +130,5 @@ namespace Microsoft::Console::TSF int _compositions = 0; AnsiInputScope _ansiInputScope{ this }; - inline static std::atomic _wantsAnsiInputScope{ false }; }; } From 38d2fdad5f7549aa10d5a0974bc2b805cf011dec Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 24 Nov 2025 16:21:49 -0800 Subject: [PATCH 3/3] Replace NullableColorPicker ContentDialog with Flyout (#19572) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary of the Pull Request Updates the NullableColorPicker to use a flyout instead of a content dialog. Frankly, it should've been this way from the start. #19561 is an issue regarding the rectangle on the right side of the picker. The complaint being that it should be something more useful than a preview, an idea being that it could be a lightness gradient. Unfortunately, the WinUI color picker doesn't let you do that. It's just a plain preview. That said, there's a lot of customizations that can be added still to increase value here. To name a few: - IsColorSliderVisible --> a color slider to adjust the lightness of the color (as desired in #19561) - IsHexInputVisible --> an input field to see and adjust the hex value directly - IsColorChannelTextInputVisible --> several input fields to adjust individual RGB channels or switch over to HSV However, the content dialog doesn't allow for text input due to a WinUI bug and it's too small to display all of those controls. Instead, I just discarded the content dialog altogether and opted into a flyout. This makes it a more consistent experience with the other color pickers (i.e. tab color, edit color scheme page). This also adds space for all of the functionality mentioned above (those properties are enabled by default). ## Validation Steps Performed ✅ selecting a color still works Closes #19561 --- .../NullableColorPicker.cpp | 9 ++---- .../NullableColorPicker.h | 6 ++-- .../NullableColorPicker.xaml | 29 +++++-------------- 3 files changed, 12 insertions(+), 32 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/NullableColorPicker.cpp b/src/cascadia/TerminalSettingsEditor/NullableColorPicker.cpp index 4cb9f655b7..7996535c97 100644 --- a/src/cascadia/TerminalSettingsEditor/NullableColorPicker.cpp +++ b/src/cascadia/TerminalSettingsEditor/NullableColorPicker.cpp @@ -159,12 +159,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation CurrentColor(nullptr); } - safe_void_coroutine NullableColorPicker::MoreColors_Clicked(const IInspectable& /*sender*/, const RoutedEventArgs& /*args*/) - { - co_await ColorPickerDialog().ShowAsync(); - } - - void NullableColorPicker::ColorPickerDialog_Opened(const IInspectable& /*sender*/, const ContentDialogOpenedEventArgs& /*args*/) + void NullableColorPicker::Flyout_Opening(const IInspectable& /*sender*/, const IInspectable& /*args*/) { // Initialize color picker with current color if (CurrentColor()) @@ -185,7 +180,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation } } - void NullableColorPicker::ColorPickerDialog_PrimaryButtonClick(const IInspectable& /*sender*/, const ContentDialogButtonClickEventArgs& /*args*/) + void NullableColorPicker::Flyout_Closing(const winrt::Windows::Foundation::IInspectable& /*sender*/, const winrt::Windows::UI::Xaml::Controls::Primitives::FlyoutBaseClosingEventArgs& /*args*/) { const auto& selectedColor = ColorPickerControl().Color(); const Microsoft::Terminal::Core::Color terminalColor{ selectedColor.R, selectedColor.G, selectedColor.B, selectedColor.A }; diff --git a/src/cascadia/TerminalSettingsEditor/NullableColorPicker.h b/src/cascadia/TerminalSettingsEditor/NullableColorPicker.h index 876ce23d7f..04ce05aa32 100644 --- a/src/cascadia/TerminalSettingsEditor/NullableColorPicker.h +++ b/src/cascadia/TerminalSettingsEditor/NullableColorPicker.h @@ -22,10 +22,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void ColorChip_DataContextChanged(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::UI::Xaml::DataContextChangedEventArgs& args); void NullColorButton_Clicked(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::UI::Xaml::RoutedEventArgs& args); - safe_void_coroutine MoreColors_Clicked(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::UI::Xaml::RoutedEventArgs& args); - - void ColorPickerDialog_Opened(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::UI::Xaml::Controls::ContentDialogOpenedEventArgs& args); - void ColorPickerDialog_PrimaryButtonClick(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::UI::Xaml::Controls::ContentDialogButtonClickEventArgs& args); + void Flyout_Opening(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& args); + void Flyout_Closing(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::UI::Xaml::Controls::Primitives::FlyoutBaseClosingEventArgs& args); DEPENDENCY_PROPERTY(Editor::ColorSchemeViewModel, ColorSchemeVM); DEPENDENCY_PROPERTY(Windows::Foundation::IReference, CurrentColor); diff --git a/src/cascadia/TerminalSettingsEditor/NullableColorPicker.xaml b/src/cascadia/TerminalSettingsEditor/NullableColorPicker.xaml index 6755543311..941f29b5e3 100644 --- a/src/cascadia/TerminalSettingsEditor/NullableColorPicker.xaml +++ b/src/cascadia/TerminalSettingsEditor/NullableColorPicker.xaml @@ -97,25 +97,6 @@ - - - - @@ -146,8 +127,14 @@