Avoid inconsistently showing the VK due to redundant focus changes to TSF

Problem: Input panel policy only gets evaluated when textinputstore acquires
focus. We call AssociateFocus and SetFocus for every focus change that led to
multiple redundant focus messages to TSF. Input panel policy is set to auto
only when the user taps on the edit control. It calls AssociateFocus and
SetFocus instead of just OnStatusChange on the focused textinputstore.
This leads to an inconsistent showing of the virtual keyboard because the
policy switch sometimes happens after the focus gets evaluated by
TSF.

Solution:When browser launches, address bar is focused and the text input
state is set to URL. The input pane policy is manual on focus. The
text input state change notification calls AssociateFocus which also
calls SetFocus internally on the document manager in TSF. Calling SetFocus
again queues up redundant focus messages in TSF that interferes in other
focus messages so we removed this call in this patch. When user taps on the
address bar, input pane policy is changed to automatic on
ShowVirtualKeyboardIfEnabled call. This notifies TSF with just a status
change of the dynamic flag instead of calling AssociateFocus and SetFocus
(which would have triggered lot of redundant focus messages to TSF.
When the user opens google.com, the script sets focus on the input box
which triggers a DetachTextInputClient from the address bar and also a
text input state change notification. This sets the text input type to be
SEARCH. In between this transition, null TSFtextStore gets focus that also
calls AssociateFocus and TSF transitions its focus from URL to null and then
from null to SEARCH. The input pane policy is set to manual until the user
taps on the search box as we don't want to raise the virtual keyboard on focus
and without user interaction on the page. Once the user taps on the search box,
we set the input pane policy to automatic and call status change to notify
TSF about the change. When TSF calls GetStatus to query for the dynamic flag,
it will have the manual input pane policy disabled and the virtual keyboard
will come up.

Current flow of focus notification to TSF: When text input type is none
i.e. no editable element is in focus, we call AssociateFocus on text input
type and set the input panel policy to manual only when the text input client
is detached or changed. This happens when user switches focus between address
bar and a webpage. We call SetFocus only when the text input type is anything
other than none. We don't call AssociateFocus and SetFocus at the same time
to reduce redundant focus changes that leads to VK issues.

Test: ui\base\ime\win\tsf_input_policy_unittest.cc

Bug:1014365

Change-Id: I2a2dd28a8ae514cfa4912f5051cde351928b2ab2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1895750
Commit-Queue: Anupam Snigdha <snianu@microsoft.com>
Reviewed-by: Yohei Yukawa <yukawa@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714086}

ui/base/ime/win/input_method_win_tsf.cc

@@ -153,6 +153,11 @@ void InputMethodWinTSF::OnWillChangeFocusedClient(
   if (IsWindowFocused(focused_before)) {
     ConfirmCompositionText(/* reset_engine */ true,
                            /* keep_selection */ false);
+    // set input policy back to manual from automatic
+    // We will set the policy to automatic when user taps on the edit control so
+    // software input panel can come up
+    ui::TSFBridge::GetInstance()->SetInputPanelPolicy(
+          /*inputPanelPolicyManual*/ true);
     ui::TSFBridge::GetInstance()->RemoveFocusedClient(focused_before);
   }
 }
@@ -164,7 +169,6 @@ void InputMethodWinTSF::OnDidChangeFocusedClient(
       IsTextInputClientFocused(focused)) {
     ui::TSFBridge::GetInstance()->SetFocusedClient(toplevel_window_handle_,
                                                    focused);
-
     // Force to update the input type since client's TextInputStateChanged()
     // function might not be called if text input types before the client loses
     // focus and after it acquires focus again are the same.

ui/base/ime/win/tsf_bridge.cc

@@ -227,12 +227,18 @@ void TSFBridgeImpl::OnTextInputTypeChanged(const TextInputClient* client) {
     return;
   }
 
-  UpdateAssociateFocus();
-
   TSFDocument* document = GetAssociatedDocument();
   if (!document)
     return;
-  thread_manager_->SetFocus(document->document_manager.Get());
+  // We call AssociateFocus for text input type none that also
+  // triggers SetFocus internally. We don't want to send multiple
+  // focus notifications for the same text input type so we don't
+  // call AssociateFocus and SetFocus together. Just calling SetFocus
+  // should be sufficient for setting focus on a textstore.
+  if (client_->GetTextInputType() != TEXT_INPUT_TYPE_NONE)
+    thread_manager_->SetFocus(document->document_manager.Get());
+  else
+    UpdateAssociateFocus();
   OnTextLayoutChanged();
 }
 
@@ -251,10 +257,7 @@ void TSFBridgeImpl::SetInputPanelPolicy(bool input_panel_policy_manual) {
     return;
   if (!document->text_store)
     return;
-
   document->text_store->SetInputPanelPolicy(input_panel_policy_manual);
-  UpdateAssociateFocus();
-  thread_manager_->SetFocus(document->document_manager.Get());
 }
 
 bool TSFBridgeImpl::CancelComposition() {
@@ -530,6 +533,7 @@ void TSFBridgeImpl::UpdateAssociateFocus() {
   // the attached document manager will not be destroyed while it is attached.
   // This should be true as long as TSFBridge::Shutdown() is called late phase
   // of UI thread shutdown.
+  // AssociateFocus calls SetFocus on the document manager internally
   Microsoft::WRL::ComPtr<ITfDocumentMgr> previous_focus;
   thread_manager_->AssociateFocus(attached_window_handle_,
                                   document->document_manager.Get(),

ui/base/ime/win/tsf_input_policy_unittest.cc

@@ -209,6 +209,53 @@ class TSFInputPanelTest : public testing::Test {
   std::unique_ptr<FakeInputMethod> fake_input_method_;
 };
 
+class TSFMultipleInputPanelTest : public testing::Test {
+ protected:
+  void SetUp() override {
+    text_store1_ = new TSFTextStore();
+    text_store2_ = new TSFTextStore();
+    sink1_ = new MockStoreACPSink();
+    sink2_ = new MockStoreACPSink();
+    EXPECT_EQ(S_OK, text_store1_->AdviseSink(IID_ITextStoreACPSink,
+                                             sink1_.get(), TS_AS_ALL_SINKS));
+    EXPECT_EQ(S_OK, text_store2_->AdviseSink(IID_ITextStoreACPSink,
+                                             sink2_.get(), TS_AS_ALL_SINKS));
+    text_store1_->SetFocusedTextInputClient(kWindowHandle,
+                                            &text_input_client1_);
+    text_store1_->SetInputMethodDelegate(&input_method_delegate_);
+    text_store2_->SetFocusedTextInputClient(kWindowHandle,
+                                            &text_input_client2_);
+    text_store2_->SetInputMethodDelegate(&input_method_delegate_);
+    fake_input_method_ = std::make_unique<FakeInputMethod>();
+    fake_input_method_->SetTSFTextStoreForBridge(text_store1_.get());
+  }
+
+  void SwitchToDifferentTSFTextStore() {
+    fake_input_method_->SetTSFTextStoreForBridge(text_store2_.get());
+  }
+
+  void TearDown() override {
+    EXPECT_EQ(S_OK, text_store1_->UnadviseSink(sink1_.get()));
+    EXPECT_EQ(S_OK, text_store2_->UnadviseSink(sink2_.get()));
+    sink1_ = nullptr;
+    sink2_ = nullptr;
+    text_store1_ = nullptr;
+    text_store2_ = nullptr;
+  }
+
+  // Accessors to the internal state of TSFTextStore.
+
+  base::win::ScopedCOMInitializer com_initializer_;
+  MockTextInputClient text_input_client1_;
+  MockTextInputClient text_input_client2_;
+  MockInputMethodDelegate input_method_delegate_;
+  scoped_refptr<TSFTextStore> text_store1_;
+  scoped_refptr<TSFTextStore> text_store2_;
+  scoped_refptr<MockStoreACPSink> sink1_;
+  scoped_refptr<MockStoreACPSink> sink2_;
+  std::unique_ptr<FakeInputMethod> fake_input_method_;
+};
+
 namespace {
 
 TEST_F(TSFInputPanelTest, GetStatusTest) {
@@ -252,5 +299,38 @@ TEST_F(TSFInputPanelTest, AutomaticInputPaneToManualPolicyTest) {
             status.dwStaticFlags);
 }
 
+TEST_F(TSFMultipleInputPanelTest, InputPaneSwitchForMultipleTSFTextStoreTest) {
+  TS_STATUS status = {};
+  // Invoke the virtual keyboard through InputMethod
+  // and test if the automatic policy flag has been set or not.
+  EXPECT_EQ(S_OK, text_store1_->GetStatus(&status));
+  EXPECT_EQ((ULONG)TS_SD_INPUTPANEMANUALDISPLAYENABLE, status.dwDynamicFlags);
+  EXPECT_EQ((ULONG)(TS_SS_TRANSITORY | TS_SS_NOHIDDENTEXT),
+            status.dwStaticFlags);
+  fake_input_method_->ShowVirtualKeyboardIfEnabled();
+  EXPECT_EQ(S_OK, text_store1_->GetStatus(&status));
+  EXPECT_NE((ULONG)TS_SD_INPUTPANEMANUALDISPLAYENABLE, status.dwDynamicFlags);
+  EXPECT_EQ((ULONG)(TS_SS_TRANSITORY | TS_SS_NOHIDDENTEXT),
+            status.dwStaticFlags);
+  fake_input_method_->DetachTextInputClient(nullptr);
+  SwitchToDifferentTSFTextStore();
+  // Different TSFTextStore is in focus so manual policy should be set in the
+  // previous one
+  EXPECT_EQ(S_OK, text_store1_->GetStatus(&status));
+  EXPECT_EQ((ULONG)TS_SD_INPUTPANEMANUALDISPLAYENABLE, status.dwDynamicFlags);
+  EXPECT_EQ((ULONG)(TS_SS_TRANSITORY | TS_SS_NOHIDDENTEXT),
+            status.dwStaticFlags);
+  EXPECT_EQ(S_OK, text_store2_->GetStatus(&status));
+  EXPECT_EQ((ULONG)TS_SD_INPUTPANEMANUALDISPLAYENABLE, status.dwDynamicFlags);
+  EXPECT_EQ((ULONG)(TS_SS_TRANSITORY | TS_SS_NOHIDDENTEXT),
+            status.dwStaticFlags);
+  fake_input_method_->ShowVirtualKeyboardIfEnabled();
+  EXPECT_EQ(S_OK, text_store2_->GetStatus(&status));
+  EXPECT_NE((ULONG)TS_SD_INPUTPANEMANUALDISPLAYENABLE, status.dwDynamicFlags);
+  EXPECT_EQ((ULONG)(TS_SS_TRANSITORY | TS_SS_NOHIDDENTEXT),
+            status.dwStaticFlags);
+}
+
 }  // namespace
+
 }  // namespace ui

ui/base/ime/win/tsf_text_store.cc

@@ -1213,6 +1213,10 @@ bool TSFTextStore::ConfirmComposition() {
 
 void TSFTextStore::SetInputPanelPolicy(bool input_panel_policy_manual) {
   input_panel_policy_manual_ = input_panel_policy_manual;
+  // This notification tells TSF that the input pane flag has changed.
+  // TSF queries for the status of this flag using GetStatus and gets
+  // the updated value.
+  text_store_acp_sink_->OnStatusChange(TS_SD_INPUTPANEMANUALDISPLAYENABLE);
 }
 
 void TSFTextStore::SendOnLayoutChange() {