[omnibox] For about:blank homepage, select-all the initial focus

Currently, when the user has chosen about:blank as their homepage, and
Chromium starts up, the location bar is focused, but the URL
"about:blank" is NOT selected.

This leads to the cursor being at index 0, and the user starts typing
BEFORE the existing "about:blank" URL, so typing "foo" leads to:
"fooabout:blank".

This is surprising behavior, and this CL fixes that, as well as adds a
test.

We originally tried to fix this in 2019, but this effort had to be
reverted because we caused a bug on the NTP:
https://chromium-review.googlesource.com/c/chromium/src/+/1559115

This CL preserves the fixed NTP behavior (validated by a test), and
also fixes the about:blank behavior, also validated by a new test.

Bug: 45260
Change-Id: I8ca57ea10215c297aabc22e51138af7ea21ef677
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2464202Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816272}

chrome/browser/ui/browser.cc

@@ -1318,11 +1318,9 @@ void Browser::SetFocusToLocationBar() {
   // Two differences between this and FocusLocationBar():
   // (1) This doesn't get recorded in user metrics, since it's called
   //     internally.
-  // (2) This is called with |select_all| == false, because this is a renderer
-  //     initiated focus (this method is a WebContentsDelegate override).
-  //     We don't select-all for renderer initiated focuses, as the user may
-  //     currently be typing something while the tab finishes loading. We don't
-  //     want to clobber user input by selecting all while the user is typing.
+  // (2) This is called with |is_user_initiated| == false, because this is a
+  //     renderer initiated focus (this method is a WebContentsDelegate
+  //     override).
   window_->SetFocusToLocationBar(false);
 }
 

chrome/browser/ui/location_bar/location_bar.h

@@ -48,8 +48,6 @@ class LocationBar {
   // Renderer-initiated focuses (like browser startup or NTP finished loading),
   // should have |is_user_initiated| set to false, so we can avoid disrupting
   // user actions and avoid requesting on-focus suggestions.
-  //
-  // TODO(tommycli): See if there's a more descriptive name for this method.
   virtual void FocusLocation(bool is_user_initiated) = 0;
 
   // Puts the user into keyword mode with their default search provider.

chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

@@ -737,6 +737,22 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest,
   EXPECT_EQ(old_selected_line, popup_model->selected_line());
 }
 
+// Verifies that https://crbug.com/45260 doesn't regress.
+IN_PROC_BROWSER_TEST_F(OmniboxViewTest,
+                       RendererInitiatedFocusSelectsAllWhenStartingBlurred) {
+  ASSERT_NO_FATAL_FAILURE(NavigateExpectUrl(GURL("about:blank")));
+
+  OmniboxView* omnibox_view = nullptr;
+  ASSERT_NO_FATAL_FAILURE(GetOmniboxView(&omnibox_view));
+  ASSERT_FALSE(omnibox_view->IsSelectAll());
+
+  // Simulate a renderer-initated focus event. Expect that everything is
+  // selected now.
+  browser()->SetFocusToLocationBar();
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+}
+
+// Verifies that https://crbug.com/924935 doesn't regress.
 IN_PROC_BROWSER_TEST_F(OmniboxViewTest,
                        RendererInitiatedFocusPreservesUserText) {
   OmniboxView* omnibox_view = nullptr;

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

@@ -1412,7 +1412,7 @@ LocationBar* BrowserView::GetLocationBar() const {
   return GetLocationBarView();
 }
 
-void BrowserView::SetFocusToLocationBar(bool select_all) {
+void BrowserView::SetFocusToLocationBar(bool is_user_initiated) {
   // On Windows, changing focus to the location bar causes the browser window to
   // become active. This can steal focus if the user has another window open
   // already. On Chrome OS, changing focus makes a view believe it has a focus
@@ -1424,7 +1424,7 @@ void BrowserView::SetFocusToLocationBar(bool select_all) {
 #endif
 
   LocationBarView* location_bar = GetLocationBarView();
-  location_bar->FocusLocation(select_all);
+  location_bar->FocusLocation(is_user_initiated);
   if (!location_bar->omnibox_view()->HasFocus()) {
     // If none of location bar got focus, then clear focus.
     views::FocusManager* focus_manager = GetFocusManager();

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

@@ -388,7 +388,7 @@ class BrowserView : public BrowserWindow,
   autofill::AutofillBubbleHandler* GetAutofillBubbleHandler() override;
   void ExecutePageActionIconForTesting(PageActionIconType type) override;
   LocationBar* GetLocationBar() const override;
-  void SetFocusToLocationBar(bool select_all) override;
+  void SetFocusToLocationBar(bool is_user_initiated) override;
   void UpdateReloadStopState(bool is_loading, bool force) override;
   void UpdateToolbar(content::WebContents* contents) override;
   void UpdateCustomTabBarVisibility(bool visible, bool animate) override;

chrome/browser/ui/views/location_bar/location_bar_view.cc

@@ -385,7 +385,20 @@ void LocationBarView::FocusLocation(bool is_user_initiated) {
   if (omnibox_already_focused)
     omnibox_view()->model()->ClearKeyword();
 
-  if (is_user_initiated)
+  // If the user initiated the focus, then we always select-all, even if the
+  // omnibox is already focused. This can happen if the user pressed Ctrl+L
+  // while already typing in the omnibox.
+  //
+  // For renderer initiated focuses (like NTP or about:blank page load finish):
+  //  - If the omnibox was not already focused, select-all. This handles the
+  //    about:blank homepage case, where the location bar has initial focus.
+  //    It annoys users if the URL is not pre-selected. https://crbug.com/45260.
+  //  - If the omnibox is already focused, DO NOT select-all. This can happen
+  //    if the user starts typing before the NTP finishes loading. If the NTP
+  //    finishes loading and then does a renderer-initiated focus, performing
+  //    a select-all here would surprisingly overwrite the user's first few
+  //    typed characters. https://crbug.com/924935.
+  if (is_user_initiated || !omnibox_already_focused)
     omnibox_view_->SelectAll(true);
 }