Skip to content

Allow IME input in Search Box#4723

Merged
2 commits merged intomasterfrom
dev/lelian/imesearchbox
Feb 26, 2020
Merged

Allow IME input in Search Box#4723
2 commits merged intomasterfrom
dev/lelian/imesearchbox

Conversation

@leonMSFT
Copy link
Contributor

@leonMSFT leonMSFT commented Feb 26, 2020

Summary of the Pull Request

Currently, when the user attempts to type using IME while the Search Box is focused, the input goes to the terminal instead. This is due to the fact that the TSFInputControl assumes it's in control whenever TermControl gets focus. So, it'll intercept IME input before the Search Box receives it. We simply need to modify TermControl::GotFocus to check if the SearchBox has focus. If it does, TSFInputControl::NotifyFocusEnter shouldn't be called.

As a small side fix, I've also disabled the terminal cursor blinking when the Search Box has focus.

Thinking a little further, if we have more features in the future that behave like search box (i.e. advanced tab switcher, or any other XAML controls that pop up) and require text input, we might need to create a sort of "AnyOtherTextControlInFocus" function to see if TSFInputControl should receive focus or not.

PR Checklist

Validation Steps Performed

Search works as expected with IME. Composition picker appears underneath Search Box when typing IME in the Search Box. Clicking outside of the Search Box still returns control to TSFInputControl/TermControl.

Terminal Cursor blinks when it has focus, and doesn't when the Search Box has focus.

@leonMSFT leonMSFT added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Feb 26, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm cool with this. I'll let you experiment with moving this block of code above the uiaEngine enablement like Dustin proposed before automerging though

@leonMSFT leonMSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 26, 2020
@ghost
Copy link

ghost commented Feb 26, 2020

Hello @leonMSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 61e5917 into master Feb 26, 2020
@ghost ghost deleted the dev/lelian/imesearchbox branch February 26, 2020 19:28
DHowett-MSFT pushed a commit that referenced this pull request Feb 28, 2020
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.
DHowett-MSFT pushed a commit that referenced this pull request Feb 28, 2020
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.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't type with IME in the search widget

4 participants