[omnibox] Prevent on-focus suggestions popping up from Alt+Tab.

Currently, when we restore focus to the omnibox by merely switching the
window (Alt+Tab), we trigger on focus suggestions. This is not good.

This CL does two things:
 1) Fixes FocusManager so that it provides the correct |reason| for
    restoring the focus to Views.
 2) Modifies OmniboxViewViews and OmniboxEditModel to only trigger
    on-focus suggestions for DIRECT focus events, and not RESTORE
    events.

Bug: 1013287, 996516
Change-Id: I885162f29b43b052b3251f02c22c37c047a1bf64
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1865621Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708229}

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

@@ -1308,8 +1308,15 @@ void OmniboxViewViews::OnFocus() {
   // TODO(tommycli): This does not seem like it should be necessary.
   // Investigate why it's needed and see if we can remove it.
   model()->ResetDisplayTexts();
+
+  bool suppress = suppress_on_focus_suggestions_;
+  if (GetFocusManager() &&
+      GetFocusManager()->focus_change_reason() !=
+          views::FocusManager::FocusChangeReason::kDirectFocusChange) {
+    suppress = true;
+  }
   // TODO(oshima): Get control key state.
-  model()->OnSetFocus(false, suppress_on_focus_suggestions_);
+  model()->OnSetFocus(false, suppress);
   // Don't call controller()->OnSetFocus, this view has already acquired focus.
 
   // Restore the selection we saved in OnBlur() if it's still valid.

ui/views/focus/focus_manager.cc

@@ -377,6 +377,14 @@ void FocusManager::SetFocusedViewWithReason(View* view,
     delegate_->OnDidChangeFocus(old_focused_view, focused_view_);
 }
 
+void FocusManager::SetFocusedView(View* view) {
+  FocusChangeReason reason = FocusChangeReason::kDirectFocusChange;
+  if (in_restoring_focused_view_)
+    reason = FocusChangeReason::kFocusRestore;
+
+  SetFocusedViewWithReason(view, reason);
+}
+
 void FocusManager::ClearFocus() {
   // SetFocusedView(nullptr) is going to clear out the stored view to. We need
   // to persist it in this case.
@@ -438,12 +446,8 @@ bool FocusManager::RestoreFocusedView() {
       } else {
         // This usually just sets the focus if this view is focusable, but
         // let the view override RequestFocus if necessary.
+        base::AutoReset<bool> in_restore_bit(&in_restoring_focused_view_, true);
         view->RequestFocus();
-
-        // If it succeeded, the reason would be incorrect; set it to
-        // focus restore.
-        if (focused_view_ == view)
-          focus_change_reason_ = FocusChangeReason::kFocusRestore;
       }
     }
     // The |keyboard_accessible_| mode may have changed while the widget was

ui/views/focus/focus_manager.h

@@ -171,9 +171,7 @@ class VIEWS_EXPORT FocusManager : public ViewObserver {
   // a reason). If the focus change should only happen if the view is
   // currenty focusable, enabled, and visible, call view->RequestFocus().
   void SetFocusedViewWithReason(View* view, FocusChangeReason reason);
-  void SetFocusedView(View* view) {
-    SetFocusedViewWithReason(view, FocusChangeReason::kDirectFocusChange);
-  }
+  void SetFocusedView(View* view);
 
   // Get the reason why the focus most recently changed.
   FocusChangeReason focus_change_reason() const { return focus_change_reason_; }
@@ -378,6 +376,9 @@ class VIEWS_EXPORT FocusManager : public ViewObserver {
   // access is enabled.
   bool keyboard_accessible_ = false;
 
+  // Whether FocusManager is currently trying to restore a focused view.
+  bool in_restoring_focused_view_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(FocusManager);
 };
 

ui/views/focus/focus_manager_unittest.cc

@@ -39,11 +39,9 @@ namespace views {
 enum FocusTestEventType { ON_FOCUS = 0, ON_BLUR };
 
 struct FocusTestEvent {
-  FocusTestEvent(FocusTestEventType type, int view_id)
-      : type(type), view_id(view_id) {}
-
   FocusTestEventType type;
   int view_id;
+  FocusManager::FocusChangeReason focus_change_reason;
 };
 
 class SimpleTestView : public View {
@@ -55,11 +53,19 @@ class SimpleTestView : public View {
   }
 
   void OnFocus() override {
-    event_list_->push_back(FocusTestEvent(ON_FOCUS, GetID()));
+    event_list_->push_back({
+        ON_FOCUS,
+        GetID(),
+        GetFocusManager()->focus_change_reason(),
+    });
   }
 
   void OnBlur() override {
-    event_list_->push_back(FocusTestEvent(ON_BLUR, GetID()));
+    event_list_->push_back({
+        ON_BLUR,
+        GetID(),
+        GetFocusManager()->focus_change_reason(),
+    });
   }
 
  private:
@@ -84,6 +90,8 @@ TEST_F(FocusManagerTest, ViewFocusCallbacks) {
   ASSERT_EQ(1, static_cast<int>(event_list.size()));
   EXPECT_EQ(ON_FOCUS, event_list[0].type);
   EXPECT_EQ(kView1ID, event_list[0].view_id);
+  EXPECT_EQ(FocusChangeReason::kDirectFocusChange,
+            event_list[0].focus_change_reason);
 
   event_list.clear();
   view2->RequestFocus();
@@ -92,12 +100,18 @@ TEST_F(FocusManagerTest, ViewFocusCallbacks) {
   EXPECT_EQ(kView1ID, event_list[0].view_id);
   EXPECT_EQ(ON_FOCUS, event_list[1].type);
   EXPECT_EQ(kView2ID, event_list[1].view_id);
+  EXPECT_EQ(FocusChangeReason::kDirectFocusChange,
+            event_list[0].focus_change_reason);
+  EXPECT_EQ(FocusChangeReason::kDirectFocusChange,
+            event_list[1].focus_change_reason);
 
   event_list.clear();
   GetFocusManager()->ClearFocus();
   ASSERT_EQ(1, static_cast<int>(event_list.size()));
   EXPECT_EQ(ON_BLUR, event_list[0].type);
   EXPECT_EQ(kView2ID, event_list[0].view_id);
+  EXPECT_EQ(FocusChangeReason::kDirectFocusChange,
+            event_list[0].focus_change_reason);
 }
 
 TEST_F(FocusManagerTest, FocusChangeListener) {
@@ -706,7 +720,10 @@ TEST_P(FocusManagerArrowKeyTraversalTest, ArrowKeyTraversal) {
 }
 
 TEST_F(FocusManagerTest, StoreFocusedView) {
-  View* view = new View;
+  std::vector<FocusTestEvent> event_list;
+  const int kView1ID = 1;
+  SimpleTestView* view = new SimpleTestView(&event_list, kView1ID);
+
   // Add view to the view hierarchy and make it focusable.
   GetWidget()->GetRootView()->AddChildView(view);
   view->SetFocusBehavior(View::FocusBehavior::ALWAYS);
@@ -716,13 +733,39 @@ TEST_F(FocusManagerTest, StoreFocusedView) {
   EXPECT_EQ(nullptr, GetFocusManager()->GetFocusedView());
   EXPECT_TRUE(GetFocusManager()->RestoreFocusedView());
   EXPECT_EQ(view, GetFocusManager()->GetStoredFocusView());
+  ASSERT_EQ(3, static_cast<int>(event_list.size()));
+  EXPECT_EQ(ON_FOCUS, event_list[0].type);
+  EXPECT_EQ(kView1ID, event_list[0].view_id);
+  EXPECT_EQ(FocusChangeReason::kDirectFocusChange,
+            event_list[0].focus_change_reason);
+  EXPECT_EQ(ON_BLUR, event_list[1].type);
+  EXPECT_EQ(kView1ID, event_list[1].view_id);
+  EXPECT_EQ(FocusChangeReason::kDirectFocusChange,
+            event_list[1].focus_change_reason);
+  EXPECT_EQ(ON_FOCUS, event_list[2].type);
+  EXPECT_EQ(kView1ID, event_list[2].view_id);
+  EXPECT_EQ(FocusChangeReason::kFocusRestore,
+            event_list[2].focus_change_reason);
 
   // Repeat with |true|.
+  event_list.clear();
   GetFocusManager()->SetFocusedView(view);
   GetFocusManager()->StoreFocusedView(true);
   EXPECT_EQ(nullptr, GetFocusManager()->GetFocusedView());
   EXPECT_TRUE(GetFocusManager()->RestoreFocusedView());
   EXPECT_EQ(view, GetFocusManager()->GetStoredFocusView());
+  ASSERT_EQ(2, static_cast<int>(event_list.size()));
+  EXPECT_EQ(ON_BLUR, event_list[0].type);
+  EXPECT_EQ(kView1ID, event_list[0].view_id);
+  EXPECT_EQ(FocusChangeReason::kDirectFocusChange,
+            event_list[0].focus_change_reason);
+  EXPECT_EQ(ON_FOCUS, event_list[1].type);
+  EXPECT_EQ(kView1ID, event_list[1].view_id);
+  EXPECT_EQ(FocusChangeReason::kFocusRestore,
+            event_list[1].focus_change_reason);
+
+  // Necessary for clean teardown.
+  GetFocusManager()->ClearFocus();
 }
 
 #if defined(OS_MACOSX)

ui/views/test/focus_manager_test.h

@@ -17,6 +17,8 @@ class FocusChangeListener;
 
 class FocusManagerTest : public ViewsTestBase, public WidgetDelegate {
  public:
+  using FocusChangeReason = FocusManager::FocusChangeReason;
+
   FocusManagerTest();
   ~FocusManagerTest() override;