TermControl: rework input and focus handling by DHowett-MSFT · Pull Request #4748 · microsoft/terminal

DHowett-MSFT · 2020-02-28T23:47:02Z

This commit changes how we handle focus and tabstop-ness in TermControl
and its descendent controls.

The root of the issue that #4031 set out to investigate is that
TermControl hosts a bunch of other controls, but it's using event
tunneling to get first dibs on every bit of keyboard input sent its way
-- even for things that live inside it. This required us to hack around
and detemrine whether the input was intended for someone else before
eventually ignoring it or using it.

I believe the correct thing for us to do is to direct user keyboard
input to a control that lives logically "inside" the terminal surface
and have that control hold focus and process keyboard events.

Herein, we introduce an empty and invisible UserControl to hold focus
inside the terminal surface. Everything I've tested works great here.

It's important to note that the UserControl is invisible. Unfortunately,
this means that it can't receive mouse input. Its parent, the
SwapChainPanel acting as the terminal surface, can.

I've gone through and unmarked things that should not show up in the
tabstop list and changed the focus parameters for things that should.

Doing this lets us kill TermControl's dependency on all private details
in SearchBoxControl, including the bit where we could ask "do you think
you should be focused?", by letting Xaml handle focus. We now listen to
more focus events and funnel focus where we think it belongs. For
SearchBoxControl, that means we focus the text field and continue to
select all the text in it when the SearchBoxControl gets focus.

Fixes #4031.
This is an alternative solution to #4723, so I reverted that fix.

PR Checklist

Closes TermControl key down handling investigation and refactoring #4031
CLA signed
I've discussed this with core contributors already.

This commit changes how we handle focus and tabstop-ness in TermControl and its descendent controls. The root of the issue that #4031 set out to investigate is that TermControl hosts a bunch of other controls, but it's using event tunneling to get first dibs on every bit of keyboard input sent its way -- even for things that live inside it. This required us to hack around and detemrine whether the input was intended for someone else before eventually ignoring it or using it. I believe the _correct_ thing for us to do is to direct user keyboard input to a control that lives logically "inside" the terminal surface and have that control hold focus and process keyboard events. Herein, we introduce an empty and invisible UserControl to hold focus inside the terminal surface. Everything I've tested works great here. It's important to note that the UserControl is invisible. Unfortunately, this means that it can't receive **mouse** input. Its parent, the SwapChainPanel acting as the terminal surface, can. I've gone through and unmarked things that should not show up in the tabstop list and changed the focus parameters for things that should. Doing this lets us kill TermControl's dependency on all private details in SearchBoxControl, including the bit where we could ask "do you think you should be focused?", by letting Xaml handle focus. We now listen to more focus events and funnel focus where we think it belongs. For SearchBoxControl, that means we focus the text field and continue to select all the text in it when the SearchBoxControl gets focus. Fixes #4031. This is an alternative solution to #4723, so I reverted that fix.

carlos-zamora

Tab Stop Issues:

You changed a few tab stop things. They make sense. You can tab to a SwapChainPanel. But I still can't figure out how to tab to a search box. Honestly, it's ok to just remove the search box as a tab stop because the user can still access it via the search box keybinding.

Accessibility:

This introduces a severe regression. For some reason, the TermControlAutomationPeer is no longer being interacted with when the TermControl gets focus. This means that we are completely unable to access our Automation Providers and extract info from the text buffer.
What's weird is that the TCAP still appears in the tree. But there's no way to navigate to it...

src/cascadia/TerminalControl/SearchBoxControl.xaml

carlos-zamora · 2020-03-02T22:11:56Z

src/cascadia/TerminalControl/TermControl.cpp

+    // Method Description:
+    // - Funnels focus from TermControl to its input control
+    // Arguments:
+    // - e: the event args for the focus event
+    void TermControl::OnGotFocus(const RoutedEventArgs& e)
+    {
+        if (e.OriginalSource() == *this)
+        {
+            // If we're about to get focus, funnel it into our input site.
+            TerminalInputTarget().Focus(FocusState::Programmatic);
+        }
+    }


So, this means that whenever the TermControl is attempted to get focus, we redirect focus to the TerminalInputTarget right? So you can't ever accidentally give focus to the search box?

Also, just to be clear, this is called just before the _GotFocusHandler?

Should we consolidate styles and rename this function to OnGotFocusHandler?

OnGotFocus is a special method name in that the UserControl composition class will call it if it's implemented. It must be named this: it is not a handler, it is not bound to an event.

If the TermControl itself attempts to receive focus (not the controls inside it, not the search box, not anything else), we redirect focus to the input target.

DHowett-MSFT · 2020-03-02T22:20:25Z

You changed a few tab stop things. They make sense. You can tab to a SwapChainPanel. But I still can't figure out how to tab to a search box. Honestly, it's ok to just remove the search box as a tab stop because the user can still access it via the search box keybinding.

Information in response to first comment.

leonMSFT

Just a small question around SearchBoxControl::OnGotFocus. Although it seems like Carlos noticed some concerns around accessibility, so I'll hold my approval for now.

leonMSFT · 2020-03-03T18:50:02Z

src/cascadia/TerminalControl/SearchBoxControl.cpp

+    void SearchBoxControl::OnGotFocus(/*const IInspectable& sender,*/ Windows::UI::Xaml::RoutedEventArgs const& args)
    {
-        if (TextBox())
+        if (args.OriginalSource() == *this)


Just to see if I'm understanding this correctly, this condition returns true only if the SearchBoxControl directly gained focus (probably from TermControl)? I'm guessing if the user clicked on the SearchBoxControl::TextBox() or its many Toggle-able buttons, the OriginalSource would be the TextBox or the buttons, so it wouldn't satisfy this condition?

Indeed! I wanted to maintain the behavior that we had, where the searchbox didn't re-select all its text when the textbox was re-focused, but did when we entered the control for the first time. 😄

zadjii-msft

This seems much more manageable than before. I trust Carlos to make sure his concerns about the accessibility actually get sorted out before he signs off

miniksa · 2020-03-06T16:36:01Z

This one's a party of Leon, Mike, and Carlos, so I'm skipping this one intentionally.

…input_sink

zadjii-msft · 2020-06-26T13:57:27Z

Hey I'm gonna mark this as "needs author feedback" - I'm not sure how to resolve the accessibility concerns Carlos had, or if they're still reproing, but maybe we should loop back on this, esp with #6680

ghost · 2020-07-03T15:00:11Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

DHowett-MSFT requested review from carlos-zamora, leonMSFT and zadjii-msft February 28, 2020 23:47

DHowett-MSFT force-pushed the dev/duhowett/be_my_input_sink branch from 650b321 to bfdce8f Compare February 28, 2020 23:52

carlos-zamora requested changes Mar 2, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Mar 2, 2020

leonMSFT reviewed Mar 3, 2020

View reviewed changes

zadjii-msft approved these changes Mar 5, 2020

View reviewed changes

zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Needs-Second It's a PR that needs another sign-off labels Mar 6, 2020

ghost requested a review from miniksa March 6, 2020 13:37

zadjii-msft mentioned this pull request Mar 24, 2020

Switch to the I-beam cursor when hovering over the terminal #5028

Closed

DHowett added 2 commits March 25, 2020 15:49

Merge remote-tracking branch 'origin/master' into dev/duhowett/be_my_…

ec22e43

…input_sink

Merge remote-tracking branch 'origin/master' into dev/duhowett/be_my_…

8881ce1

…input_sink

zadjii-msft added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Second It's a PR that needs another sign-off labels Jun 26, 2020

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jul 3, 2020

ghost closed this Jul 10, 2020

DHowett deleted the dev/duhowett/be_my_input_sink branch October 26, 2021 16:59

This pull request was closed.

Conversation

DHowett-MSFT commented Feb 28, 2020

PR Checklist

Uh oh!

carlos-zamora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlos-zamora Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT commented Mar 2, 2020

Uh oh!

leonMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

leonMSFT Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

miniksa commented Mar 6, 2020

Uh oh!

zadjii-msft commented Jun 26, 2020

Uh oh!

ghost commented Jul 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

carlos-zamora left a comment •

edited

Loading