From 818f03b610dd9d3329fd1bc833c3dadab901dcdf Mon Sep 17 00:00:00 2001 From: Don-Vito Date: Thu, 10 Dec 2020 00:01:08 +0200 Subject: [PATCH] Make command palette ephemeral (#8377) So the implementation is somewhat dirty. The ideas was nice - add lostFocusHandler However it broke few things: * In the TabSwitcher the ListItem must be focusable since otherwise the SingleSelectionMode behavior breaks. To address this I had to put the lostFocusHandler on the items as well * When you click the flyout, the palette loses focus, which makes the terminal page to set the focus on the tab, closing the flyout. To address this I had to ensure the tab won't get focused once the flyout is open. In addition, flyout should fix the focus before opening, otherwise alt+tab will put a focus on a tab row rather than on tab * I also had to close the palette if the tab order changes. To prevent inconsistencies. Closes #8355 --- src/cascadia/TerminalApp/CommandPalette.cpp | 38 +++++++++++++++ src/cascadia/TerminalApp/CommandPalette.h | 3 ++ src/cascadia/TerminalApp/CommandPalette.xaml | 6 ++- src/cascadia/TerminalApp/TerminalPage.cpp | 51 ++++++++++---------- 4 files changed, 70 insertions(+), 28 deletions(-) diff --git a/src/cascadia/TerminalApp/CommandPalette.cpp b/src/cascadia/TerminalApp/CommandPalette.cpp index 90c1f7f95c1d..2cd6a8cf54cb 100644 --- a/src/cascadia/TerminalApp/CommandPalette.cpp +++ b/src/cascadia/TerminalApp/CommandPalette.cpp @@ -438,6 +438,44 @@ namespace winrt::TerminalApp::implementation void CommandPalette::_rootPointerPressed(Windows::Foundation::IInspectable const& /*sender*/, Windows::UI::Xaml::Input::PointerRoutedEventArgs const& /*e*/) { + if (Visibility() != Visibility::Collapsed) + { + _dismissPalette(); + } + } + + // Method Description: + // - The purpose of this event handler is to hide the palette if it loses focus. + // We say we lost focus if our root element and all its descendants lost focus. + // This handler is invoked when our root element or some descendant loses focus. + // At this point we need to learn if the newly focused element belongs to this palette. + // To achieve this: + // - We start with the newly focused element and traverse its visual ancestors up to the Xaml root. + // - If one of the ancestors is this CommandPalette, then by our definition the focus is not lost + // - If we reach the Xaml root without meeting this CommandPalette, + // then the focus is not contained in it anymore and it should be dismissed + // Arguments: + // - + // Return Value: + // - + void CommandPalette::_lostFocusHandler(Windows::Foundation::IInspectable const& /*sender*/, + Windows::UI::Xaml::RoutedEventArgs const& /*args*/) + { + auto focusedElementOrAncestor = Input::FocusManager::GetFocusedElement(this->XamlRoot()).try_as(); + while (focusedElementOrAncestor) + { + if (focusedElementOrAncestor == *this) + { + // This palette is the focused element or an ancestor of the focused element. No need to dismiss. + return; + } + + // Go up to the next ancestor + focusedElementOrAncestor = winrt::Windows::UI::Xaml::Media::VisualTreeHelper::GetParent(focusedElementOrAncestor); + } + + // We got to the root (the element with no parent) and didn't meet this palette on the path. + // It means that it lost the focus and needs to be dismissed. _dismissPalette(); } diff --git a/src/cascadia/TerminalApp/CommandPalette.h b/src/cascadia/TerminalApp/CommandPalette.h index 21c001293074..731ce0b53876 100644 --- a/src/cascadia/TerminalApp/CommandPalette.h +++ b/src/cascadia/TerminalApp/CommandPalette.h @@ -85,6 +85,9 @@ namespace winrt::TerminalApp::implementation void _updateUIForStackChange(); void _rootPointerPressed(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::PointerRoutedEventArgs const& e); + + void _lostFocusHandler(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::RoutedEventArgs const& args); + void _backdropPointerPressed(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::PointerRoutedEventArgs const& e); void _listItemClicked(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Controls::ItemClickEventArgs const& e); diff --git a/src/cascadia/TerminalApp/CommandPalette.xaml b/src/cascadia/TerminalApp/CommandPalette.xaml index 680a6be37f4d..b6b7c9107a07 100644 --- a/src/cascadia/TerminalApp/CommandPalette.xaml +++ b/src/cascadia/TerminalApp/CommandPalette.xaml @@ -17,6 +17,7 @@ the MIT License. See LICENSE in the project root for license information. --> PreviewKeyDown="_previewKeyDownHandler" KeyDown="_keyDownHandler" PreviewKeyUp="_keyUpHandler" + LostFocus="_lostFocusHandler" mc:Ignorable="d" AutomationProperties.Name="{x:Bind ControlName, Mode=OneWay}"> @@ -245,8 +246,9 @@ the MIT License. See LICENSE in the project root for license information. --> + IsTabStop="False" + AutomationProperties.Name="{x:Bind Command.Name, Mode=OneWay}" + AutomationProperties.AcceleratorKey="{x:Bind Command.KeyChordText, Mode=OneWay}"> diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index d43ff02f404c..2f76c68f166b 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -585,6 +585,21 @@ namespace winrt::TerminalApp::implementation newTabFlyout.Items().Append(aboutFlyout); } + // Before opening the fly-out set focus on the current tab + // so no matter how fly-out is closed later on the focus will return to some tab. + // We cannot do it on closing because if the window loses focus (alt+tab) + // the closing event is not fired. + // It is important to set the focus on the tab + // Since the previous focus location might be discarded in the background, + // e.g., the command palette will be dismissed by the menu, + // and then closing the fly-out will move the focus to wrong location. + newTabFlyout.Opening([this](auto&&, auto&&) { + if (auto index{ _GetFocusedTabIndex() }) + { + _tabs.GetAt(*index).Focus(FocusState::Programmatic); + _UpdateMRUTab(index.value()); + } + }); _newTabButton.Flyout(newTabFlyout); } @@ -1072,19 +1087,6 @@ namespace winrt::TerminalApp::implementation _tabView.TabItems().RemoveAt(tabIndex); _UpdateTabIndices(); - // If the tab switcher is currently open, update it to reflect the - // current list of tabs. - const auto tabSwitchMode = _settings.GlobalSettings().TabSwitcherMode(); - const bool commandPaletteIsVisible = CommandPalette().Visibility() == Visibility::Visible; - if (tabSwitchMode == TabSwitcherMode::MostRecentlyUsed && commandPaletteIsVisible) - { - CommandPalette().SetTabActions(_mruTabActions, false); - } - else if (commandPaletteIsVisible) - { - _UpdatePaletteWithInOrderTabs(); - } - // To close the window here, we need to close the hosting window. if (_tabs.Size() == 0) { @@ -2107,6 +2109,7 @@ namespace winrt::TerminalApp::implementation } } + CommandPalette().Visibility(Visibility::Collapsed); _UpdateTabView(); } @@ -2693,11 +2696,16 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_CommandPaletteClosed(const IInspectable& /*sender*/, const RoutedEventArgs& /*eventArgs*/) { - // Return focus to the active control - if (auto index{ _GetFocusedTabIndex() }) + // We don't want to set focus on the tab if fly-out is open as it will be closed + // TODO GH#5400: consider checking we are not in the opening state, by hooking both Opening and Open events + if (!_newTabButton.Flyout().IsOpen()) { - _tabs.GetAt(*index).Focus(FocusState::Programmatic); - _UpdateMRUTab(index.value()); + // Return focus to the active control + if (auto index{ _GetFocusedTabIndex() }) + { + _tabs.GetAt(*index).Focus(FocusState::Programmatic); + _UpdateMRUTab(index.value()); + } } } @@ -2834,15 +2842,6 @@ namespace winrt::TerminalApp::implementation { _mruTabActions.RemoveAt(mruIndex); _mruTabActions.InsertAt(0, command); - - // If the tab switcher is currently open, AND we're using it in - // MRU mode, then update it to reflect the current list of tabs. - const auto tabSwitchMode = _settings.GlobalSettings().TabSwitcherMode(); - const bool commandPaletteIsVisible = CommandPalette().Visibility() == Visibility::Visible; - if (tabSwitchMode == TabSwitcherMode::MostRecentlyUsed && commandPaletteIsVisible) - { - CommandPalette().SetTabActions(_mruTabActions, false); - } } } }