From 01aacb5ec21b9742c79b92afc6249c462b972602 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 1 May 2024 16:17:01 -0500 Subject: [PATCH 1/5] i'm just gonna start by committing everything --- .../ScratchIslandApp/SampleApp/MyPage.xaml | 70 ++++++++++++++++++- .../ScratchIslandApp/SampleApp/MySettings.h | 2 +- src/cascadia/TerminalApp/Pane.cpp | 30 ++++---- src/cascadia/TerminalApp/Pane.h | 2 + .../GlobalAppearance.xaml | 3 +- .../TerminalSettingsEditor/Launch.cpp | 24 +++++++ src/cascadia/TerminalSettingsEditor/Launch.h | 4 +- 7 files changed, 115 insertions(+), 20 deletions(-) diff --git a/scratch/ScratchIslandApp/SampleApp/MyPage.xaml b/scratch/ScratchIslandApp/SampleApp/MyPage.xaml index 0c132dea21b..f1685179695 100644 --- a/scratch/ScratchIslandApp/SampleApp/MyPage.xaml +++ b/scratch/ScratchIslandApp/SampleApp/MyPage.xaml @@ -51,7 +51,75 @@ Padding="16" HorizontalAlignment="Stretch" VerticalAlignment="Stretch" - Background="#0000ff" /> + Background="#0000ff"> + + + + + + + + + Blue + Green + Red + Yellow + + + + + + Blue + Green + Red + Yellow + + + + + + + Blue + Green + Red + Yellow + + + + + + + + + + Blue + Green + Red + Yellow + + + + + + + + + + diff --git a/scratch/ScratchIslandApp/SampleApp/MySettings.h b/scratch/ScratchIslandApp/SampleApp/MySettings.h index 3e490416d9a..d73290390e2 100644 --- a/scratch/ScratchIslandApp/SampleApp/MySettings.h +++ b/scratch/ScratchIslandApp/SampleApp/MySettings.h @@ -11,7 +11,7 @@ Licensed under the MIT license. #include #include "MySettings.g.h" -using IFontFeatureMap = winrt::Windows::Foundation::Collections::IMap; +using IFontFeatureMap = winrt::Windows::Foundation::Collections::IMap; using IFontAxesMap = winrt::Windows::Foundation::Collections::IMap; namespace winrt::SampleApp::implementation diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index ac599612359..e64846ff30c 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -47,14 +47,8 @@ Pane::Pane(const IPaneContent& content, const bool lastFocused) : // LOAD-BEARING: This will NOT work if the border's BorderBrush is set to // Colors::Transparent! The border won't get Tapped events, and they'll fall // through to something else. - _borderFirst.Tapped([this](auto&, auto& e) { - _FocusFirstChild(); - e.Handled(true); - }); - _borderSecond.Tapped([this](auto&, auto& e) { - _FocusFirstChild(); - e.Handled(true); - }); + _borderFirst.Tapped([this](auto& s, auto& e) { _borderTappedHandler(s, e); }); + _borderSecond.Tapped([this](auto& s, auto& e) { _borderTappedHandler(s, e); }); } Pane::Pane(std::shared_ptr first, @@ -88,14 +82,8 @@ Pane::Pane(std::shared_ptr first, // LOAD-BEARING: This will NOT work if the border's BorderBrush is set to // Colors::Transparent! The border won't get Tapped events, and they'll fall // through to something else. - _borderFirst.Tapped([this](auto&, auto& e) { - _FocusFirstChild(); - e.Handled(true); - }); - _borderSecond.Tapped([this](auto&, auto& e) { - _FocusFirstChild(); - e.Handled(true); - }); + _borderFirst.Tapped([this](auto& s, auto& e) { _borderTappedHandler(s, e); }); + _borderSecond.Tapped([this](auto& s, auto& e) { _borderTappedHandler(s, e); }); } // Extract the terminal settings from the current (leaf) pane's control @@ -1237,6 +1225,10 @@ void Pane::UpdateVisuals() // - void Pane::_Focus() { + if (WasLastFocused()) + { + return; + } GotFocus.raise(shared_from_this(), FocusState::Programmatic); if (const auto& lastContent{ GetLastFocusedContent() }) { @@ -3021,3 +3013,9 @@ winrt::Windows::UI::Xaml::Media::SolidColorBrush Pane::_ComputeBorderColor() return _themeResources.unfocusedBorderBrush; } + +void Pane::_borderTappedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/, const winrt::Windows::UI::Xaml::Input::TappedRoutedEventArgs& e) +{ + _FocusFirstChild(); + e.Handled(true); +} diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index 67daee1694f..f25a7b0834a 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -314,6 +314,8 @@ class Pane : public std::enable_shared_from_this SplitState _convertAutomaticOrDirectionalSplitState(const winrt::Microsoft::Terminal::Settings::Model::SplitDirection& splitType) const; + void _borderTappedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::UI::Xaml::Input::TappedRoutedEventArgs& e); + // Function Description: // - Returns true if the given direction can be used with the given split // type. diff --git a/src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml b/src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml index 510779c27a3..26bb23a4149 100644 --- a/src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml +++ b/src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml @@ -31,7 +31,8 @@ + SelectedItem="{x:Bind ViewModel.CurrentLanguage, Mode=TwoWay}" + Style="{StaticResource ComboBoxSettingStyle}"> diff --git a/src/cascadia/TerminalSettingsEditor/Launch.cpp b/src/cascadia/TerminalSettingsEditor/Launch.cpp index 15ed5bee124..016fdccac6d 100644 --- a/src/cascadia/TerminalSettingsEditor/Launch.cpp +++ b/src/cascadia/TerminalSettingsEditor/Launch.cpp @@ -41,4 +41,28 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { _ViewModel = e.Parameter().as(); } + static inline void DismissAllPopups(const winrt::Windows::UI::Xaml::XamlRoot& xamlRoot) + { + const auto popups{ winrt::Windows::UI::Xaml::Media::VisualTreeHelper::GetOpenPopupsForXamlRoot(xamlRoot) }; + for (const auto& p : popups) + { + p.IsOpen(false); + } + } + // When the ScrollViewer scrolls, dismiss any popups we might have. + void Launch::ViewChanging(const winrt::Windows::Foundation::IInspectable& sender, + const winrt::Windows::UI::Xaml::Controls::ScrollViewerViewChangingEventArgs& /*e*/) + { + // Inside this struct, we can't get at the XamlRoot() that our subclass + // implements. I mean, _we_ can, but when XAML does its code + // generation, _XAML_ won't be able to figure it out. + // + // Fortunately for us, we don't need to! The sender is a UIElement, so + // we can just get _their_ XamlRoot(). + if (const auto& uielem{ sender.try_as() }) + { + DismissAllPopups(uielem.XamlRoot()); + } + } + } diff --git a/src/cascadia/TerminalSettingsEditor/Launch.h b/src/cascadia/TerminalSettingsEditor/Launch.h index 985be422af3..aefda5b192d 100644 --- a/src/cascadia/TerminalSettingsEditor/Launch.h +++ b/src/cascadia/TerminalSettingsEditor/Launch.h @@ -8,7 +8,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { - struct Launch : public HasScrollViewer, LaunchT + struct Launch : public /*HasScrollViewer,*/ LaunchT { public: Launch(); @@ -17,6 +17,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation til::property_changed_event PropertyChanged; WINRT_OBSERVABLE_PROPERTY(Editor::LaunchViewModel, ViewModel, PropertyChanged.raise, nullptr); + void ViewChanging(const winrt::Windows::Foundation::IInspectable& sender, + const winrt::Windows::UI::Xaml::Controls::ScrollViewerViewChangingEventArgs& /*e*/); }; } From c52c6b8b634991d3d0ec888cf214c18bca68f683 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 1 May 2024 16:19:16 -0500 Subject: [PATCH 2/5] revert these guys --- .../ScratchIslandApp/SampleApp/MyPage.xaml | 70 +------------------ .../TerminalSettingsEditor/Launch.cpp | 24 ------- src/cascadia/TerminalSettingsEditor/Launch.h | 4 +- 3 files changed, 2 insertions(+), 96 deletions(-) diff --git a/scratch/ScratchIslandApp/SampleApp/MyPage.xaml b/scratch/ScratchIslandApp/SampleApp/MyPage.xaml index f1685179695..0c132dea21b 100644 --- a/scratch/ScratchIslandApp/SampleApp/MyPage.xaml +++ b/scratch/ScratchIslandApp/SampleApp/MyPage.xaml @@ -51,75 +51,7 @@ Padding="16" HorizontalAlignment="Stretch" VerticalAlignment="Stretch" - Background="#0000ff"> - - - - - - - - - Blue - Green - Red - Yellow - - - - - - Blue - Green - Red - Yellow - - - - - - - Blue - Green - Red - Yellow - - - - - - - - - - Blue - Green - Red - Yellow - - - - - - - - - - + Background="#0000ff" /> diff --git a/src/cascadia/TerminalSettingsEditor/Launch.cpp b/src/cascadia/TerminalSettingsEditor/Launch.cpp index 016fdccac6d..15ed5bee124 100644 --- a/src/cascadia/TerminalSettingsEditor/Launch.cpp +++ b/src/cascadia/TerminalSettingsEditor/Launch.cpp @@ -41,28 +41,4 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { _ViewModel = e.Parameter().as(); } - static inline void DismissAllPopups(const winrt::Windows::UI::Xaml::XamlRoot& xamlRoot) - { - const auto popups{ winrt::Windows::UI::Xaml::Media::VisualTreeHelper::GetOpenPopupsForXamlRoot(xamlRoot) }; - for (const auto& p : popups) - { - p.IsOpen(false); - } - } - // When the ScrollViewer scrolls, dismiss any popups we might have. - void Launch::ViewChanging(const winrt::Windows::Foundation::IInspectable& sender, - const winrt::Windows::UI::Xaml::Controls::ScrollViewerViewChangingEventArgs& /*e*/) - { - // Inside this struct, we can't get at the XamlRoot() that our subclass - // implements. I mean, _we_ can, but when XAML does its code - // generation, _XAML_ won't be able to figure it out. - // - // Fortunately for us, we don't need to! The sender is a UIElement, so - // we can just get _their_ XamlRoot(). - if (const auto& uielem{ sender.try_as() }) - { - DismissAllPopups(uielem.XamlRoot()); - } - } - } diff --git a/src/cascadia/TerminalSettingsEditor/Launch.h b/src/cascadia/TerminalSettingsEditor/Launch.h index aefda5b192d..985be422af3 100644 --- a/src/cascadia/TerminalSettingsEditor/Launch.h +++ b/src/cascadia/TerminalSettingsEditor/Launch.h @@ -8,7 +8,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { - struct Launch : public /*HasScrollViewer,*/ LaunchT + struct Launch : public HasScrollViewer, LaunchT { public: Launch(); @@ -17,8 +17,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation til::property_changed_event PropertyChanged; WINRT_OBSERVABLE_PROPERTY(Editor::LaunchViewModel, ViewModel, PropertyChanged.raise, nullptr); - void ViewChanging(const winrt::Windows::Foundation::IInspectable& sender, - const winrt::Windows::UI::Xaml::Controls::ScrollViewerViewChangingEventArgs& /*e*/); }; } From 5b3cf010c567afecb43ddc883d167cc777f9b7bf Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 1 May 2024 16:22:56 -0500 Subject: [PATCH 3/5] comments --- src/cascadia/TerminalApp/Pane.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index e64846ff30c..320340ed875 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -1225,10 +1225,14 @@ void Pane::UpdateVisuals() // - void Pane::_Focus() { + // Don't focus our content if we're already focused. This prevents a bug + // where tapping on the arrow in a ComboBox will land in our Tapped handler, + // and if we steal focus from the ComboBox, it won't open. See GH#17062 if (WasLastFocused()) { return; } + GotFocus.raise(shared_from_this(), FocusState::Programmatic); if (const auto& lastContent{ GetLastFocusedContent() }) { From 800324e3b4ba752e38e51340b2bc708a72fab19b Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Wed, 1 May 2024 16:17:45 -0700 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Leonard Hecker --- src/cascadia/TerminalApp/Pane.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 320340ed875..275e2913029 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -47,8 +47,8 @@ Pane::Pane(const IPaneContent& content, const bool lastFocused) : // LOAD-BEARING: This will NOT work if the border's BorderBrush is set to // Colors::Transparent! The border won't get Tapped events, and they'll fall // through to something else. - _borderFirst.Tapped([this](auto& s, auto& e) { _borderTappedHandler(s, e); }); - _borderSecond.Tapped([this](auto& s, auto& e) { _borderTappedHandler(s, e); }); + _borderFirst.Tapped({ this, _borderTappedHandler }); + _borderSecond.Tapped({ this, _borderTappedHandler }); } Pane::Pane(std::shared_ptr first, @@ -82,8 +82,8 @@ Pane::Pane(std::shared_ptr first, // LOAD-BEARING: This will NOT work if the border's BorderBrush is set to // Colors::Transparent! The border won't get Tapped events, and they'll fall // through to something else. - _borderFirst.Tapped([this](auto& s, auto& e) { _borderTappedHandler(s, e); }); - _borderSecond.Tapped([this](auto& s, auto& e) { _borderTappedHandler(s, e); }); + _borderFirst.Tapped({ this, _borderTappedHandler }); + _borderSecond.Tapped({ this, _borderTappedHandler }); } // Extract the terminal settings from the current (leaf) pane's control From 4281a3a33bac3903545d92dc837da18fbe19df2b Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Wed, 1 May 2024 16:35:02 -0700 Subject: [PATCH 5/5] ugh --- src/cascadia/TerminalApp/Pane.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 275e2913029..fd195d8d42a 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -47,8 +47,8 @@ Pane::Pane(const IPaneContent& content, const bool lastFocused) : // LOAD-BEARING: This will NOT work if the border's BorderBrush is set to // Colors::Transparent! The border won't get Tapped events, and they'll fall // through to something else. - _borderFirst.Tapped({ this, _borderTappedHandler }); - _borderSecond.Tapped({ this, _borderTappedHandler }); + _borderFirst.Tapped({ this, &Pane::_borderTappedHandler }); + _borderSecond.Tapped({ this, &Pane::_borderTappedHandler }); } Pane::Pane(std::shared_ptr first, @@ -82,8 +82,8 @@ Pane::Pane(std::shared_ptr first, // LOAD-BEARING: This will NOT work if the border's BorderBrush is set to // Colors::Transparent! The border won't get Tapped events, and they'll fall // through to something else. - _borderFirst.Tapped({ this, _borderTappedHandler }); - _borderSecond.Tapped({ this, _borderTappedHandler }); + _borderFirst.Tapped({ this, &Pane::_borderTappedHandler }); + _borderSecond.Tapped({ this, &Pane::_borderTappedHandler }); } // Extract the terminal settings from the current (leaf) pane's control