From 7575f4d2ab6f4e3bf40f0f13dbf61348758e0e9b Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 31 Mar 2022 13:17:32 -0500 Subject: [PATCH] Auto-focus window renamer textbox on open (#12798) Does what it says on the tin. This is maximal BODGE. `TeachingTip` doesn't provide an `Opened` event. (https://github.com/microsoft/microsoft-ui-xaml/issues/1607). But we want to focus the renamer text box when it's opened. We can't do that immediately, the TextBox technically isn't in the visual tree yet. We have to wait for it to get added some time after we call IsOpen. How do we do that reliably? Usually, for this kind of thing, we'd just use a one-off LayoutUpdated event, as a notification that the TextBox was added to the tree. HOWEVER: * The _first_ time this is fired, when the box is _first_ opened, yeeting focus doesn't work on the first LayoutUpdated. It does work on the second LayoutUpdated. Okay, so we'll wait for two LayoutUpdated events, and focus on the second. * On subsequent opens: We only ever get a single LayoutUpdated. Period. But, you can successfully focus it on that LayoutUpdated. So, we'll keep track of how many LayoutUpdated's we've _ever_ gotten. If we've had at least 2, then we can focus the text box. We're also not using a ContentDialog for this, because in Xaml Islands a text box in a ContentDialog won't receive _any_ keypresses. Fun! ## References * microsoft/microsoft-ui-xaml#1607 * microsoft/microsoft-ui-xaml#6910 * microsoft/microsoft-ui-xaml#3257 * microsoft/terminal#9662 ## PR Checklist * [x] Will close out #12021, but that's an a11y bug that needs secondary validation * [x] Closes #11322 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed Tested manually (cherry picked from commit b57fe85997c587e45066ecbed4a3d3fed6b749f0) Service-Card-Id: 79978834 Service-Version: 1.13 --- .../TerminalApp/AppActionHandlers.cpp | 48 ++++++++++++++++--- src/cascadia/TerminalApp/TerminalPage.cpp | 26 +++++++++- src/cascadia/TerminalApp/TerminalPage.h | 5 ++ src/cascadia/TerminalApp/TerminalPage.xaml | 1 + 4 files changed, 72 insertions(+), 8 deletions(-) diff --git a/src/cascadia/TerminalApp/AppActionHandlers.cpp b/src/cascadia/TerminalApp/AppActionHandlers.cpp index dd01c2a2e22..a82575656a9 100644 --- a/src/cascadia/TerminalApp/AppActionHandlers.cpp +++ b/src/cascadia/TerminalApp/AppActionHandlers.cpp @@ -844,18 +844,54 @@ namespace winrt::TerminalApp::implementation } _UpdateTeachingTipTheme(WindowRenamer().try_as()); - WindowRenamer().IsOpen(true); - // PAIN: We can't immediately focus the textbox in the TeachingTip. It's - // not technically focusable until it is opened. However, it doesn't - // provide an event to tell us when it is opened. That's tracked in - // microsoft/microsoft-ui-xaml#1607. So for now, the user _needs_ to - // click on the text box manually. + // BODGY: GH#12021 + // + // TeachingTip doesn't provide an Opened event. + // (microsoft/microsoft-ui-xaml#1607). But we want to focus the renamer + // text box when it's opened. We can't do that immediately, the TextBox + // technically isn't in the visual tree yet. We have to wait for it to + // get added some time after we call IsOpen. How do we do that reliably? + // Usually, for this kind of thing, we'd just use a one-off + // LayoutUpdated event, as a notification that the TextBox was added to + // the tree. HOWEVER: + // * The _first_ time this is fired, when the box is _first_ opened, + // tossing focus doesn't work on the first LayoutUpdated. It does + // work on the second LayoutUpdated. Okay, so we'll wait for two + // LayoutUpdated events, and focus on the second. + // * On subsequent opens: We only ever get a single LayoutUpdated. + // Period. But, you can successfully focus it on that LayoutUpdated. + // + // So, we'll keep track of how many LayoutUpdated's we've _ever_ gotten. + // If we've had at least 2, then we can focus the text box. // // We're also not using a ContentDialog for this, because in Xaml // Islands a text box in a ContentDialog won't receive _any_ keypresses. // Fun! // WindowRenamerTextBox().Focus(FocusState::Programmatic); + _renamerLayoutUpdatedRevoker.revoke(); + _renamerLayoutUpdatedRevoker = WindowRenamerTextBox().LayoutUpdated(winrt::auto_revoke, [weakThis = get_weak()](auto&&, auto&&) { + if (auto self{ weakThis.get() }) + { + auto& count{ self->_renamerLayoutCount }; + + // Don't just always increment this, we don't want to deal with overflow situations + if (count < 2) + { + count++; + } + + if (count >= 2) + { + self->_renamerLayoutUpdatedRevoker.revoke(); + self->WindowRenamerTextBox().Focus(FocusState::Programmatic); + } + } + }); + // Make sure to mark that enter was not pressed in the renamer quite + // yet. More details in TerminalPage::_WindowRenamerKeyDown. + _renamerPressedEnter = false; + WindowRenamer().IsOpen(true); args.Handled(true); } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 92567cbcadb..98426a124ce 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -3610,6 +3610,26 @@ namespace winrt::TerminalApp::implementation // of thing with co_return winrt::make(false). } + // Method Description: + // - Used to track if the user pressed enter with the renamer open. If we + // immediately focus it after hitting Enter on the command palette, then + // the Enter keydown will dismiss the command palette and open the + // renamer, and then the enter keyup will go to the renamer. So we need to + // make sure both a down and up go to the renamer. + // Arguments: + // - e: the KeyRoutedEventArgs describing the key that was released + // Return Value: + // - + void TerminalPage::_WindowRenamerKeyDown(const IInspectable& /*sender*/, + winrt::Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e) + { + const auto key = e.OriginalKey(); + if (key == Windows::System::VirtualKey::Enter) + { + _renamerPressedEnter = true; + } + } + // Method Description: // - Manually handle Enter and Escape for committing and dismissing a window // rename. This is highly similar to the TabHeaderControl's KeyUp handler. @@ -3620,16 +3640,18 @@ namespace winrt::TerminalApp::implementation void TerminalPage::_WindowRenamerKeyUp(const IInspectable& sender, winrt::Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e) { - if (e.OriginalKey() == Windows::System::VirtualKey::Enter) + const auto key = e.OriginalKey(); + if (key == Windows::System::VirtualKey::Enter && _renamerPressedEnter) { // User is done making changes, close the rename box _WindowRenamerActionClick(sender, nullptr); } - else if (e.OriginalKey() == Windows::System::VirtualKey::Escape) + else if (key == Windows::System::VirtualKey::Escape) { // User wants to discard the changes they made WindowRenamerTextBox().Text(WindowName()); WindowRenamer().IsOpen(false); + _renamerPressedEnter = false; } } diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 2e48d9db883..1a72fe40c12 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -215,6 +215,10 @@ namespace winrt::TerminalApp::implementation std::shared_ptr _windowIdToast{ nullptr }; std::shared_ptr _windowRenameFailedToast{ nullptr }; + winrt::Windows::UI::Xaml::Controls::TextBox::LayoutUpdated_revoker _renamerLayoutUpdatedRevoker; + int _renamerLayoutCount{ 0 }; + bool _renamerPressedEnter{ false }; + winrt::Windows::Foundation::IAsyncOperation _ShowDialogHelper(const std::wstring_view& name); void _ShowAboutDialog(); @@ -414,6 +418,7 @@ namespace winrt::TerminalApp::implementation void _WindowRenamerActionClick(const IInspectable& sender, const IInspectable& eventArgs); void _RequestWindowRename(const winrt::hstring& newName); + void _WindowRenamerKeyDown(const IInspectable& sender, winrt::Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e); void _WindowRenamerKeyUp(const IInspectable& sender, winrt::Windows::UI::Xaml::Input::KeyRoutedEventArgs const& e); void _UpdateTeachingTipTheme(winrt::Windows::UI::Xaml::FrameworkElement element); diff --git a/src/cascadia/TerminalApp/TerminalPage.xaml b/src/cascadia/TerminalApp/TerminalPage.xaml index 5ca8ed25858..2f6e8f66960 100644 --- a/src/cascadia/TerminalApp/TerminalPage.xaml +++ b/src/cascadia/TerminalApp/TerminalPage.xaml @@ -193,6 +193,7 @@ IsLightDismissEnabled="True">