Skip to content

Commit

Permalink
Auto-focus window renamer textbox on open (#12798)
Browse files Browse the repository at this point in the history
Does what it says on the tin. This is maximal BODGE.

`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,
  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
* #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 b57fe85)
Service-Card-Id: 79978834
Service-Version: 1.13
  • Loading branch information
zadjii-msft authored and DHowett committed Mar 31, 2022
1 parent 9845a25 commit 7575f4d
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 8 deletions.
48 changes: 42 additions & 6 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -844,18 +844,54 @@ namespace winrt::TerminalApp::implementation
}

_UpdateTeachingTipTheme(WindowRenamer().try_as<winrt::Windows::UI::Xaml::FrameworkElement>());
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);
}
Expand Down
26 changes: 24 additions & 2 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3610,6 +3610,26 @@ namespace winrt::TerminalApp::implementation
// of thing with co_return winrt::make<RenameWindowResult>(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:
// - <none>
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.
Expand All @@ -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;
}
}

Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,10 @@ namespace winrt::TerminalApp::implementation
std::shared_ptr<Toast> _windowIdToast{ nullptr };
std::shared_ptr<Toast> _windowRenameFailedToast{ nullptr };

winrt::Windows::UI::Xaml::Controls::TextBox::LayoutUpdated_revoker _renamerLayoutUpdatedRevoker;
int _renamerLayoutCount{ 0 };
bool _renamerPressedEnter{ false };

winrt::Windows::Foundation::IAsyncOperation<winrt::Windows::UI::Xaml::Controls::ContentDialogResult> _ShowDialogHelper(const std::wstring_view& name);

void _ShowAboutDialog();
Expand Down Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/TerminalPage.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@
IsLightDismissEnabled="True">
<mux:TeachingTip.Content>
<TextBox x:Name="WindowRenamerTextBox"
KeyDown="_WindowRenamerKeyDown"
KeyUp="_WindowRenamerKeyUp"
Text="{x:Bind WindowName, Mode=OneWay}" />
</mux:TeachingTip.Content>
Expand Down

0 comments on commit 7575f4d

Please sign in to comment.