[reland] Focus tab contents in BeginNavigation case of NTP-initiated navigations.

[ This is a reland of https://crrev.com/c/1970834 + changes that remove
  a redundant test in //chrome/browser/portal/portal_browsertest.cc (see
  CR comments).  Original CL description follows below. ]

Tab contents may get focused during some renderer-initiated navigations
(e.g. navigations from an NTP-replacement extension - see the regression
test added in r723022).  This CL ensures that the focus decision is
applied not only to navigations that go through OpenURL, but also to
navigations that go through (more freqeuent, usual) BeginNavigation.
This change helps ensure that we retain the right focus behavior after
more navigations switch to the BeginNavigation code path (e.g. after
relanding ShouldFork removal in https://crrev.com/c/1949448).

The CL covers both the OpenURL and BeginNavigation code paths by
handling the focus decisions in a newly added TabContentFocusingHelper
class (replicating/moving the focus decisions from OpenURL-only
Browser::UpdateUIForNavigationInTab).  Adding a new tab-helper class is
useful, because the existing, OpenURL/BeginNavigation-shared navigation
notifications handlers (e.g. Browser::ScheduleUIUpdate) cannot
distinguish between replaceState and new navigations (see also new steps
in the OmniboxFocusInteractiveTest.NtpReplacementExtension test).

While this CL changes how focus behavior is implemented, it should have
no effect on end-to-end/high-level expectations of focus behavior (as
observed by end users or browser tests).

browser_tests do not guarantee window activation and/or focus, but
before this CL navigation in browser_tests would focus WebContents.
This is not happening after this CL, and requires moving a handful of
focus-dependent tests from browser_tests to interactive_ui_tests.

Bug: 1029161
Tbr: Karan Bhatia <karandeepb@chromium.org>
Tbr: Rouslan Solomakhin <rouslan@chromium.org>
Tbr: Brian Sheedy <bsheedy@chromium.org>
Tbr: Bill Budge <bbudge@chromium.org>
Tbr: Peter Kasting <pkasting@chromium.org>
Tbr: Charlie Reis <creis@chromium.org>
Change-Id: I7324e7ecb08a51601b12fba3f57c68af59f33128
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2024155Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Adithya Srinivasan <adithyas@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737502}

chrome/browser/extensions/ntp_overridden_bubble_delegate.cc

@@ -23,10 +23,6 @@
 
 namespace {
 
-// Whether the user has been notified about extension overriding the new tab
-// page.
-const char kNtpBubbleAcknowledged[] = "ack_ntp_bubble";
-
 // Whether existing NTP extensions have been automatically acknowledged.
 const char kDidAcknowledgeExistingNtpExtensions[] =
     "ack_existing_ntp_extensions";
@@ -48,6 +44,9 @@ base::LazyInstance<std::set<std::pair<Profile*, std::string>>>::Leaky
 
 namespace extensions {
 
+const char NtpOverriddenBubbleDelegate::kNtpBubbleAcknowledged[] =
+    "ack_ntp_bubble";
+
 NtpOverriddenBubbleDelegate::NtpOverriddenBubbleDelegate(Profile* profile)
     : extensions::ExtensionMessageBubbleController::Delegate(profile),
       profile_(profile) {

chrome/browser/extensions/ntp_overridden_bubble_delegate.h

@@ -19,6 +19,10 @@ namespace extensions {
 class NtpOverriddenBubbleDelegate
     : public ExtensionMessageBubbleController::Delegate {
  public:
+  // Name of the preference that says whether the user has been notified about
+  // extension overriding the new tab page.
+  static const char kNtpBubbleAcknowledged[];
+
   explicit NtpOverriddenBubbleDelegate(Profile* profile);
   ~NtpOverriddenBubbleDelegate() override;
 

chrome/browser/portal/portal_browsertest.cc

@@ -123,38 +123,6 @@ IN_PROC_BROWSER_TEST_F(PortalBrowserTest, HttpBasicAuthenticationInPortal) {
   EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
 }
 
-IN_PROC_BROWSER_TEST_F(PortalBrowserTest, FocusTransfersAcrossActivation) {
-  ASSERT_TRUE(embedded_test_server()->Start());
-  GURL url(embedded_test_server()->GetURL("/portal/activate.html"));
-  ui_test_utils::NavigateToURL(browser(), url);
-
-  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
-  EXPECT_EQ(true, content::EvalJs(contents, "loadPromise"));
-  EXPECT_TRUE(content::ExecJs(contents,
-                              "var blurPromise = new Promise(r => {"
-                              "  window.onblur = () => r(true)"
-                              "})"));
-  EXPECT_TRUE(content::ExecJs(contents,
-                              "var button = document.createElement('button');"
-                              "document.body.appendChild(button);"
-                              "button.focus();"
-                              "var buttonBlurPromise = new Promise(r => {"
-                              "  button.onblur = () => r(true)"
-                              "});"));
-  WebContents* portal_contents = contents->GetInnerWebContents()[0];
-  EXPECT_TRUE(content::ExecJs(portal_contents,
-                              "var focusPromise = new Promise(r => {"
-                              "  window.onfocus = () => r(true)"
-                              "})"));
-
-  // Activate the portal, and then check if the predecessor contents lost focus,
-  // and the portal contents got focus.
-  EXPECT_EQ(true, content::EvalJs(contents, "activate()"));
-  EXPECT_EQ(true, content::EvalJs(contents, "blurPromise"));
-  EXPECT_EQ(true, content::EvalJs(contents, "buttonBlurPromise"));
-  EXPECT_EQ(true, content::EvalJs(portal_contents, "focusPromise"));
-}
-
 namespace {
 
 std::vector<base::string16> GetRendererTaskTitles(

chrome/browser/spellchecker/spellcheck_mac_view_browsertest.mm → chrome/browser/spellchecker/spellcheck_mac_view_interactive_uitest.mm

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #import <Cocoa/Cocoa.h>
+
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ -17,13 +18,14 @@
 
 namespace {
 
-class SpellCheckMacViewBrowserTest : public InProcessBrowserTest {
+class SpellCheckMacViewInteractiveUiTest : public InProcessBrowserTest {
  public:
-  SpellCheckMacViewBrowserTest() {}
+  SpellCheckMacViewInteractiveUiTest() {}
 };
 
 #if BUILDFLAG(ENABLE_SPELLCHECK)
-IN_PROC_BROWSER_TEST_F(SpellCheckMacViewBrowserTest, SpellCheckPanelVisible) {
+IN_PROC_BROWSER_TEST_F(SpellCheckMacViewInteractiveUiTest,
+                       SpellCheckPanelVisible) {
   spellcheck::SpellCheckPanelBrowserTestHelper test_helper;
 
   ASSERT_TRUE(embedded_test_server()->Start());

chrome/browser/ui/BUILD.gn

@@ -948,6 +948,8 @@ jumbo_static_library("ui") {
       "find_bar/find_bar_platform_helper.cc",
       "find_bar/find_bar_platform_helper.h",
       "find_bar/find_bar_platform_helper_mac.mm",
+      "focus_tab_after_navigation_helper.cc",
+      "focus_tab_after_navigation_helper.h",
       "global_error/global_error.cc",
       "global_error/global_error.h",
       "global_error/global_error_bubble_view_base.h",

chrome/browser/ui/browser.cc

@@ -1007,7 +1007,6 @@ bool Browser::CanSaveContents(content::WebContents* web_contents) const {
 
 void Browser::UpdateUIForNavigationInTab(WebContents* contents,
                                          ui::PageTransition transition,
-                                         NavigateParams::WindowAction action,
                                          bool user_initiated) {
   tab_strip_model_->TabNavigating(contents, transition);
 
@@ -1030,26 +1029,6 @@ void Browser::UpdateUIForNavigationInTab(WebContents* contents,
   // the throbber will show the default favicon for a split second when
   // navigating away from the new tab page.
   ScheduleUIUpdate(contents, content::INVALIDATE_TYPE_URL);
-
-  // Figure out if the navigating contents can take focus (potentially taking it
-  // away from other, currently-focused UI element like the omnibox).
-  // Specifically, user-initiated navigations can give focus to the tab;
-  // renderer-initiated navigations usually don't, unless the NTP triggers them
-  // (in which case they're treated similarly to a user-initiated navigation).
-  //
-  // TODO(lukasza): https://crbug.com/1034999: Try to avoid special-casing
-  // kChromeUINewTabURL below and covering it via IsNTPOrRelatedURL instead.
-  const GURL& current_url = contents->GetLastCommittedURL();
-  bool contents_can_take_focus =
-      user_initiated || current_url == GURL(chrome::kChromeUINewTabURL) ||
-      search::IsNTPOrRelatedURL(
-          current_url,
-          Profile::FromBrowserContext(contents->GetBrowserContext()));
-
-  if (contents_can_take_focus && contents_is_selected &&
-      (window()->IsActive() || action == NavigateParams::SHOW_WINDOW)) {
-    contents->SetInitialFocus();
-  }
 }
 
 void Browser::RegisterKeepAlive() {
@@ -1681,7 +1660,7 @@ bool Browser::ShouldFocusLocationBarByDefault(WebContents* source) {
 
   // This should be based on the pending entry if there is one, so that
   // back/forward navigations to the NTP are handled.  The visible entry can't
-  // be used here, since back/forward navigations are not treated as  visible
+  // be used here, since back/forward navigations are not treated as visible
   // entries to avoid URL spoofs.
   content::NavigationEntry* entry =
       source->GetController().GetPendingEntry()

chrome/browser/ui/browser.h

@@ -512,7 +512,6 @@ class Browser : public TabStripModelObserver,
   // this Browser. Updates the UI for the start of this navigation.
   void UpdateUIForNavigationInTab(content::WebContents* contents,
                                   ui::PageTransition transition,
-                                  NavigateParams::WindowAction action,
                                   bool user_initiated);
 
   // Used to register a KeepAlive to affect the Chrome lifetime. The KeepAlive

chrome/browser/ui/browser_navigator.cc

@@ -662,8 +662,7 @@ void Navigate(NavigateParams* params) {
        params->disposition == WindowOpenDisposition::CURRENT_TAB)) {
     // The navigation occurred in the source tab.
     params->browser->UpdateUIForNavigationInTab(
-        contents_to_navigate_or_insert, params->transition,
-        params->window_action, user_initiated);
+        contents_to_navigate_or_insert, params->transition, user_initiated);
   } else if (singleton_index == -1) {
     // If some non-default value is set for the index, we should tell the
     // TabStripModel to respect it.

chrome/browser/ui/focus_tab_after_navigation_helper.cc

+// Copyright 2020 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 "chrome/browser/ui/focus_tab_after_navigation_helper.h"
+
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/search.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/webui_url_constants.h"
+#include "content/public/browser/browser_url_handler.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/web_contents.h"
+
+FocusTabAfterNavigationHelper::FocusTabAfterNavigationHelper(
+    content::WebContents* contents)
+    : content::WebContentsObserver(contents) {}
+
+FocusTabAfterNavigationHelper::~FocusTabAfterNavigationHelper() = default;
+
+void FocusTabAfterNavigationHelper::ReadyToCommitNavigation(
+    content::NavigationHandle* navigation) {
+  // Focus the tab contents if needed.  This is done at the ReadyToCommit time
+  // to:
+  // 1) ignore same-document navigations (ReadyToCommitNavigation method is not
+  //    invoked for same-document navigations)
+  // 2) postpone moving the focus until we are ready to commit the page
+  // 3) move the focus before the page starts rendering
+  // (only 1 is a hard-requirement;  2 and 3 seem desirable but there are no
+  // known scenarios where violating these requirements would lead to bugs).
+  if (ShouldFocusTabContents(navigation))
+    web_contents()->SetInitialFocus();
+}
+
+bool FocusTabAfterNavigationHelper::ShouldFocusTabContents(
+    content::NavigationHandle* navigation) {
+  // Don't focus content in an inactive window or tab.
+  Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
+  if (!browser)
+    return false;
+  if (!browser->window()->IsActive())
+    return false;
+  if (browser->tab_strip_model()->GetActiveWebContents() != web_contents())
+    return false;
+
+  // Don't focus content after subframe navigations.
+  if (!navigation->IsInMainFrame())
+    return false;
+
+  // Browser-initiated navigations (e.g. typing in an omnibox) should focus
+  // the tab contents.
+  if (!navigation->IsRendererInitiated())
+    return true;
+
+  // Renderer-initiated navigations shouldn't focus the tab contents, unless the
+  // navigation is leaving the NTP.  See also https://crbug.com/1027719.
+  bool started_at_ntp = IsNtpURL(web_contents()->GetLastCommittedURL());
+  if (!started_at_ntp)
+    return false;
+
+  // Rewrite chrome://newtab to compare with the navigation URL.
+  GURL rewritten_ntp_url = web_contents()->GetLastCommittedURL();
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  content::BrowserURLHandler::GetInstance()->RewriteURLIfNecessary(
+      &rewritten_ntp_url, profile);
+
+  // Focus if the destination is not the NTP.
+  return !IsNtpURL(navigation->GetURL()) &&
+         (navigation->GetURL() != rewritten_ntp_url);
+}
+
+bool FocusTabAfterNavigationHelper::IsNtpURL(const GURL& url) {
+  // TODO(lukasza): https://crbug.com/1034999: Try to avoid special-casing
+  // kChromeUINewTabURL below and covering it via IsNTPOrRelatedURL instead.
+  if (url == GURL(chrome::kChromeUINewTabURL))
+    return true;
+
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  return search::IsNTPOrRelatedURL(url, profile);
+}
+
+WEB_CONTENTS_USER_DATA_KEY_IMPL(FocusTabAfterNavigationHelper)

chrome/browser/ui/focus_tab_after_navigation_helper.h

+// Copyright 2020 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.
+
+#ifndef CHROME_BROWSER_UI_FOCUS_TAB_AFTER_NAVIGATION_HELPER_H_
+#define CHROME_BROWSER_UI_FOCUS_TAB_AFTER_NAVIGATION_HELPER_H_
+
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
+
+// FocusTabAfterNavigationHelper focuses the tab contents (potentially taking
+// focus away from other browser elements like the omnibox) after
+// 1) browser-initiated navigations (e.g. after omnibox- or bookmark-initiated
+//    navigations)
+// 2) navigations that leave NTP (e.g. after an NTP-replacement extension or
+//    third-party NTP executes window.location = ...).
+class FocusTabAfterNavigationHelper
+    : public content::WebContentsObserver,
+      public content::WebContentsUserData<FocusTabAfterNavigationHelper> {
+ public:
+  ~FocusTabAfterNavigationHelper() override;
+
+  // Not copyable nor movable.
+  FocusTabAfterNavigationHelper(const FocusTabAfterNavigationHelper&) = delete;
+  FocusTabAfterNavigationHelper& operator=(
+      const FocusTabAfterNavigationHelper&) = delete;
+
+  // content::WebContentsObserver
+  void ReadyToCommitNavigation(content::NavigationHandle* navigation) override;
+
+ private:
+  friend class content::WebContentsUserData<FocusTabAfterNavigationHelper>;
+
+  explicit FocusTabAfterNavigationHelper(content::WebContents* contents);
+
+  bool ShouldFocusTabContents(content::NavigationHandle* navigation);
+  bool IsNtpURL(const GURL& url);
+
+  WEB_CONTENTS_USER_DATA_KEY_DECL();
+};
+
+#endif  // CHROME_BROWSER_UI_FOCUS_TAB_AFTER_NAVIGATION_HELPER_H_

chrome/browser/ui/tab_helpers.cc

@@ -72,6 +72,7 @@
 #include "chrome/browser/ui/blocked_content/popup_blocker_tab_helper.h"
 #include "chrome/browser/ui/blocked_content/popup_opener_tab_helper.h"
 #include "chrome/browser/ui/find_bar/find_bar_state.h"
+#include "chrome/browser/ui/focus_tab_after_navigation_helper.h"
 #include "chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h"
 #include "chrome/browser/ui/navigation_correction_tab_observer.h"
 #include "chrome/browser/ui/passwords/manage_passwords_ui_controller.h"
@@ -323,6 +324,7 @@ void TabHelpers::AttachTabHelpers(WebContents* web_contents) {
   extensions::ChromeExtensionWebContentsObserver::CreateForWebContents(
       web_contents);
   extensions::WebNavigationTabObserver::CreateForWebContents(web_contents);
+  FocusTabAfterNavigationHelper::CreateForWebContents(web_contents);
   FormInteractionTabHelper::CreateForWebContents(web_contents);
   FramebustBlockTabHelper::CreateForWebContents(web_contents);
   IntentPickerTabHelper::CreateForWebContents(web_contents);

chrome/browser/vr/webxr_vr_spatial_tracking_test.cc

@@ -12,6 +12,7 @@ namespace vr {
 // are no runtimes available.
 IN_PROC_BROWSER_TEST_F(WebXrVrRuntimelessBrowserTest,
                        TestInlineIdentityAlwaysAvailable) {
+  browser()->tab_strip_model()->GetActiveWebContents()->Focus();
   LoadUrlAndAwaitInitialization(
       GetFileUrlForHtmlTestFile("test_inline_viewer_available"));
   WaitOnJavaScriptStep();

chrome/test/BUILD.gn

@@ -1193,7 +1193,6 @@ if (!is_android) {
       "../browser/speech/extension_api/tts_extension_apitest.cc",
       "../browser/speech/speech_recognition_browsertest.cc",
       "../browser/speech/speech_recognizer_browsertest.cc",
-      "../browser/spellchecker/spellcheck_mac_view_browsertest.mm",
       "../browser/spellchecker/spellcheck_service_browsertest.cc",
       "../browser/ssl/certificate_reporting_test_utils.cc",
       "../browser/ssl/certificate_reporting_test_utils.h",
@@ -5625,6 +5624,7 @@ if (!is_android) {
       "../browser/resource_coordinator/tab_metrics_logger_interactive_uitest.cc",
       "../browser/site_isolation/site_per_process_interactive_browsertest.cc",
       "../browser/site_isolation/site_per_process_text_input_browsertest.cc",
+      "../browser/spellchecker/spellcheck_mac_view_interactive_uitest.mm",
       "../browser/ui/autofill/autofill_popup_controller_interactive_uitest.cc",
       "../browser/ui/blocked_content/popup_blocker_browsertest.cc",
       "../browser/ui/browser_command_controller_interactive_browsertest.cc",

chrome/test/ppapi/ppapi_browsertest.cc

@@ -260,16 +260,6 @@ TEST_PPAPI_NACL(TraceEvent)
 
 TEST_PPAPI_NACL(InputEvent)
 
-// Flaky on Linux and Windows. http://crbug.com/135403
-#if defined(OS_LINUX) || defined(OS_WIN)
-#define MAYBE_ImeInputEvent DISABLED_ImeInputEvent
-#else
-#define MAYBE_ImeInputEvent ImeInputEvent
-#endif
-
-TEST_PPAPI_OUT_OF_PROCESS(MAYBE_ImeInputEvent)
-TEST_PPAPI_NACL(MAYBE_ImeInputEvent)
-
 // Graphics2D_Dev isn't supported in NaCl, only test the other interfaces
 // TODO(jhorwich) Enable when Graphics2D_Dev interfaces are proxied in NaCl.
 TEST_PPAPI_NACL(Graphics2D_InvalidResource)

chrome/test/ppapi/ppapi_interactive_browsertest.cc

@@ -9,20 +9,28 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
-
-//
-// Interface tests.
-//
+#include "ppapi/shared_impl/test_utils.h"
 
 // Disable tests under ASAN.  http://crbug.com/104832.
 // This is a bit heavy handed, but the majority of these tests fail under ASAN.
 // See bug for history.
-#if !defined(ADDRESS_SANITIZER)
-
+#if defined(ADDRESS_SANITIZER)
+#define MAYBE_MouseLock_SucceedWhenAllowed DISABLED_MouseLock_SucceedWhenAllowed
+#else
+#define MAYBE_MouseLock_SucceedWhenAllowed MouseLock_SucceedWhenAllowed
+#endif  // ADDRESS_SANITIZER
 // Disabled due to timeouts: http://crbug.com/136548
-IN_PROC_BROWSER_TEST_F(
-    OutOfProcessPPAPITest, DISABLED_MouseLock_SucceedWhenAllowed) {
+IN_PROC_BROWSER_TEST_F(OutOfProcessPPAPITest,
+                       MAYBE_MouseLock_SucceedWhenAllowed) {
   RunTestViaHTTP("MouseLock_SucceedWhenAllowed");
 }
 
-#endif // ADDRESS_SANITIZER
+// Flaky on Linux and Windows. http://crbug.com/135403
+#if defined(OS_LINUX) || defined(OS_WIN)
+#define MAYBE_ImeInputEvent DISABLED_ImeInputEvent
+#else
+#define MAYBE_ImeInputEvent ImeInputEvent
+#endif
+IN_PROC_BROWSER_TEST_F(OutOfProcessPPAPITest, MAYBE_ImeInputEvent) {
+  RunTest(ppapi::StripTestPrefixes("ImeInputEvent"));
+}