Reland "Re-focus View on tab strip close"

This is a reland of 26ae2ec0

The original CL was reverted due to the test uncovering existing bug
1029086. This reland includes the fix for the uninitialized read.

Original change's description:
> Re-focus View on tab strip close
>
> When the WebUI tab strip's WebContents gains focus, the focused
> TextInputClient changes while the focused View does not. This causes
> text input to stop working in the omnibox despite it having Views-focus.
>
> This blurs and re-focuses the currently focused View. For the Omnibox,
> this makes it re-install itself as the focused TextInputClient.
>
> This is a workaround and should be removed once bug 994350 is fixed.
>
> Bug: 1027375, 994350
> Change-Id: I2be055110535dc6164a7d31a64a3cf503fae7a7b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1935729
> Reviewed-by: Dana Fried <dfried@chromium.org>
> Commit-Queue: Dana Fried <dfried@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#719473}

Bug: 1027375, 994350, 1029086
Change-Id: I7f12945c9df64190df2d3d30726acd38d3457834
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1940869Reviewed-by: Dana Fried <dfried@chromium.org>
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719774}

chrome/browser/ui/views/frame/webui_tab_strip_container_view.cc

@@ -198,6 +198,13 @@ void WebUITabStripContainerView::UpdateButtons() {
   }
 }
 
+void WebUITabStripContainerView::SetVisibleForTesting(bool visible) {
+  SetContainerTargetVisibility(visible);
+  animation_.SetCurrentValue(visible ? 1.0 : 0.0);
+  animation_.End();
+  PreferredSizeChanged();
+}
+
 const ui::AcceleratorProvider*
 WebUITabStripContainerView::GetAcceleratorProvider() const {
   return BrowserView::GetBrowserViewForBrowser(browser_);
@@ -218,6 +225,24 @@ void WebUITabStripContainerView::SetContainerTargetVisibility(
     animation_.SetSlideDuration(base::TimeDelta::FromMilliseconds(200));
     animation_.Hide();
     web_view_->SetFocusBehavior(FocusBehavior::NEVER);
+
+    // Tapping in the WebUI tab strip gives keyboard focus to the
+    // WebContents's native window. While this doesn't take away View
+    // focus, it will change the focused TextInputClient; see
+    // |ui::InputMethod::SetFocusedTextInputClient()|. The Omnibox is a
+    // TextInputClient, and it installs itself as the focused
+    // TextInputClient when it receives Views-focus. So, tapping in the
+    // tab strip while the Omnibox has focus will mean text cannot be
+    // entered until it is blurred and re-focused. This caused
+    // crbug.com/1027375.
+    //
+    // TODO(crbug.com/994350): stop WebUI tab strip from taking focus on
+    // tap and remove this workaround.
+    views::FocusManager* const focus_manager = GetFocusManager();
+    if (focus_manager) {
+      focus_manager->StoreFocusedView(true /* clear_native_focus */);
+      focus_manager->RestoreFocusedView();
+    }
   }
   auto_closer_->set_enabled(target_visible);
 }
@@ -278,6 +303,7 @@ void WebUITabStripContainerView::ShowContextMenuAtPoint(
 }
 
 TabStripUILayout WebUITabStripContainerView::GetLayout() {
+  DCHECK(tab_contents_container_);
   return TabStripUILayout::CalculateForWebViewportSize(
       tab_contents_container_->size());
 }
@@ -338,10 +364,16 @@ void WebUITabStripContainerView::OnViewBoundsChanged(View* observed_view) {
 }
 
 void WebUITabStripContainerView::OnViewIsDeleting(View* observed_view) {
+  view_observer_.Remove(observed_view);
+
   if (observed_view == new_tab_button_)
     new_tab_button_ = nullptr;
   else if (observed_view == tab_counter_)
     tab_counter_ = nullptr;
+  else if (observed_view == tab_contents_container_)
+    tab_contents_container_ = nullptr;
+  else
+    NOTREACHED();
 }
 
 bool WebUITabStripContainerView::SetPaneFocusAndFocusDefault() {

chrome/browser/ui/views/frame/webui_tab_strip_container_view.h

@@ -52,6 +52,13 @@ class WebUITabStripContainerView : public TabStripUI::Embedder,
 
   void UpdateButtons();
 
+  // Clicking the tab counter button opens and closes the container with
+  // an animation, so it is unsuitable for an interactive test. This
+  // should be called instead. View::SetVisible() isn't sufficient since
+  // the container's preferred size will change.
+  void SetVisibleForTesting(bool visible);
+  views::WebView* web_view_for_testing() const { return web_view_; }
+
  private:
   class AutoCloser;
 
@@ -94,7 +101,7 @@ class WebUITabStripContainerView : public TabStripUI::Embedder,
 
   Browser* const browser_;
   views::WebView* const web_view_;
-  views::View* const tab_contents_container_;
+  views::View* tab_contents_container_;
   ToolbarButton* new_tab_button_ = nullptr;
   views::View* tab_counter_ = nullptr;
 

chrome/browser/ui/views/frame/webui_tab_strip_interactive_uitest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/ui/ui_features.h"
+#include "chrome/browser/ui/view_ids.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/frame/webui_tab_strip_container_view.h"
+#include "chrome/browser/ui/views/location_bar/location_bar_view.h"
+#include "chrome/browser/ui/views/omnibox/omnibox_view_views.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/interactive_test_utils.h"
+#include "ui/base/ui_base_switches.h"
+#include "ui/views/controls/webview/webview.h"
+
+class WebUITabStripInteractiveTest : public InProcessBrowserTest {
+ public:
+  WebUITabStripInteractiveTest() {
+    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+        switches::kTopChromeTouchUi, switches::kTopChromeTouchUiEnabled);
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        switches::kWebUITabStrip);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_override_;
+};
+
+// Regression test for crbug.com/1027375.
+IN_PROC_BROWSER_TEST_F(WebUITabStripInteractiveTest,
+                       CanTypeInOmniboxAfterTabStripClose) {
+  BrowserView* const browser_view =
+      BrowserView::GetBrowserViewForBrowser(browser());
+  WebUITabStripContainerView* const container = browser_view->webui_tab_strip();
+
+  // Open the tab strip.
+  container->SetVisibleForTesting(true);
+  browser_view->Layout();
+
+  // Make sure the tab strip's contents are fully loaded.
+  views::WebView* const container_web_view = container->web_view_for_testing();
+  ASSERT_TRUE(WaitForLoadStop(container_web_view->GetWebContents()));
+
+  ui_test_utils::FocusView(browser(), VIEW_ID_OMNIBOX);
+
+  OmniboxViewViews* const omnibox =
+      browser_view->toolbar()->location_bar()->omnibox_view();
+  omnibox->SetUserText(base::ASCIIToUTF16(""));
+
+  // Click in tab strip, then close it.
+  base::RunLoop click_loop;
+  ui_test_utils::MoveMouseToCenterAndPress(
+      container_web_view, ui_controls::LEFT,
+      ui_controls::DOWN | ui_controls::UP, click_loop.QuitClosure());
+  click_loop.Run();
+  container->SetVisibleForTesting(false);
+
+  // The omnibox should still be focused and should accept keyboard input.
+  EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
+  EXPECT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_A, false,
+                                              false, false, false));
+  EXPECT_EQ(base::ASCIIToUTF16("a"), omnibox->GetText());
+}

chrome/browser/ui/views/toolbar/webui_tab_counter_button.cc

@@ -6,6 +6,7 @@
 
 #include "base/i18n/message_formatter.h"
 #include "base/i18n/number_formatting.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -75,9 +76,13 @@ std::unique_ptr<views::View> CreateWebUITabCounterButton(
     TabStripModel* tab_strip_model) {
   auto tab_counter = std::make_unique<WebUITabCounterButton>(listener);
 
+  tab_counter->SetID(VIEW_ID_WEBUI_TAB_STRIP_TAB_COUNTER);
+  // TODO(crbug.com/1028827): replace this with production string and provide
+  // tab count.
+  tab_counter->SetAccessibleName(base::ASCIIToUTF16("Open tab strip"));
+
   // TODO(999557): Create a custom text style to get the correct size/weight.
   // TODO(999557): Figure out how to get the right font.
-  tab_counter->SetID(VIEW_ID_WEBUI_TAB_STRIP_TAB_COUNTER);
   tab_counter->SetProperty(views::kFlexBehaviorKey,
                            views::FlexSpecification::ForSizeRule(
                                views::MinimumFlexSizeRule::kScaleToMinimum,

chrome/test/BUILD.gn

@@ -5834,6 +5834,11 @@ if (!is_android) {
     if (is_linux && !is_chromeos) {
       deps += [ "//ui/base/ime/linux" ]
     }
+
+    if (enable_webui_tab_strip) {
+      sources +=
+          [ "../browser/ui/views/frame/webui_tab_strip_interactive_uitest.cc" ]
+    }
   }
 }