Group new tabs opened from external links or ctrl+forward/back

Opening from an external link (target=_blank or window.open) eagerly creates a group, similar to ctrl+click on a regular link.

Opening with ctrl+forward/back does not eagerly create a group, but it adds the new tab to the same group as the source tab.

Bug: 1013532, 1013537
Change-Id: I5823f572f9727c810a67333091cc294df5ec65e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1879762
Commit-Queue: Connie Wan <connily@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Reviewed-by: Charlene Yan <cyan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710077}

chrome/browser/browser_commands_unittest.cc

@@ -206,7 +206,7 @@ TEST_F(BrowserCommandsTest, BackForwardInNewTab) {
   // Select the second tab and make it go forward in a new background tab.
   browser()->tab_strip_model()->ActivateTabAt(
       1, {TabStripModel::GestureType::kOther});
-  // TODO(brettw) bug 11055: It should not be necessary to commit the load here,
+  // TODO(crbug.com/11055): It should not be necessary to commit the load here,
   // but because of this bug, it will assert later if we don't. When the bug is
   // fixed, one of the three commits here related to this bug should be removed
   // (to test both codepaths).
@@ -233,7 +233,7 @@ TEST_F(BrowserCommandsTest, BackForwardInNewTab) {
   // thing above, just validate that it's opening properly.
   browser()->tab_strip_model()->ActivateTabAt(
       2, {TabStripModel::GestureType::kOther});
-  // TODO(brettw) bug 11055: see the comment above about why we need this.
+  // TODO(crbug.com/11055): see the comment above about why we need this.
   CommitPendingLoad(&second->GetController());
   chrome::GoBack(browser(), WindowOpenDisposition::NEW_FOREGROUND_TAB);
   ASSERT_EQ(3, browser()->tab_strip_model()->active_index());
@@ -242,7 +242,7 @@ TEST_F(BrowserCommandsTest, BackForwardInNewTab) {
                 GetVisibleURL());
 
   // Same thing again for forward.
-  // TODO(brettw) bug 11055: see the comment above about why we need this.
+  // TODO(crbug.com/11055): see the comment above about why we need this.
   CommitPendingLoad(&
       browser()->tab_strip_model()->GetActiveWebContents()->GetController());
   chrome::GoForward(browser(), WindowOpenDisposition::NEW_FOREGROUND_TAB);
@@ -252,6 +252,39 @@ TEST_F(BrowserCommandsTest, BackForwardInNewTab) {
                 GetVisibleURL());
 }
 
+// Tests back/forward in new tab (Control + Back/Forward button in the UI)
+// with Tab Groups enabled.
+TEST_F(BrowserCommandsTest, BackForwardInNewTabWithGroup) {
+  GURL url1("http://foo/1");
+  GURL url2("http://foo/2");
+
+  // Make a tab with the two pages navigated in it.
+  AddTab(browser(), url1);
+  NavigateAndCommitActiveTab(url2);
+
+  // Add the tab to a Tab Group.
+  const TabGroupId group_id = browser()->tab_strip_model()->AddToNewGroup({0});
+
+  // Go back in a new background tab.
+  chrome::GoBack(browser(), WindowOpenDisposition::NEW_BACKGROUND_TAB);
+  ASSERT_EQ(0, browser()->tab_strip_model()->active_index());
+  ASSERT_EQ(2, browser()->tab_strip_model()->count());
+
+  // The new tab should have inherited the tab group from the old tab.
+  EXPECT_EQ(group_id, browser()->tab_strip_model()->GetTabGroupForTab(1));
+
+  // Select the second tab and make it go forward in a new background tab.
+  browser()->tab_strip_model()->ActivateTabAt(
+      1, {TabStripModel::GestureType::kOther});
+  // TODO(crbug.com/11055): see the comment above about why we need this.
+  CommitPendingLoad(
+      &browser()->tab_strip_model()->GetActiveWebContents()->GetController());
+  chrome::GoForward(browser(), WindowOpenDisposition::NEW_BACKGROUND_TAB);
+
+  // The new tab should have inherited the tab group from the old tab.
+  EXPECT_EQ(group_id, browser()->tab_strip_model()->GetTabGroupForTab(2));
+}
+
 TEST_F(BrowserCommandsTest, OnMaxZoomIn) {
   TabStripModel* tab_strip_model = browser()->tab_strip_model();
 

chrome/browser/ui/browser.cc

@@ -1443,7 +1443,7 @@ WebContents* Browser::OpenURLFromTab(WebContents* source,
     return nullptr;
   }
 
-  ConfigureTabGroupForNavigation(source, &nav_params);
+  chrome::ConfigureTabGroupForNavigation(&nav_params);
 
   Navigate(&nav_params);
 
@@ -1509,6 +1509,7 @@ void Browser::AddNewContents(WebContents* source,
   // the same logic for determining if the popup tracker needs to be attached.
   if (source && ConsiderForPopupBlocking(disposition))
     PopupTracker::CreateForWebContents(new_contents.get(), source);
+
   chrome::AddWebContents(this, source, std::move(new_contents), disposition,
                          initial_rect);
 }
@@ -2460,31 +2461,6 @@ void Browser::RemoveScheduledUpdatesFor(WebContents* contents) {
     scheduled_updates_.erase(i);
 }
 
-void Browser::ConfigureTabGroupForNavigation(content::WebContents* source,
-                                             NavigateParams* nav_params) {
-  if (!base::FeatureList::IsEnabled(features::kTabGroups))
-    return;
-
-  if (!source)
-    return;
-
-  if (!SupportsWindowFeature(WindowFeature::FEATURE_TABSTRIP))
-    return;
-
-  const int source_index = tab_strip_model_->GetIndexOfWebContents(source);
-
-  // If the source tab is pinned, don't create a group.
-  if (tab_strip_model_->IsTabPinned(source_index))
-    return;
-
-  if (nav_params->disposition == WindowOpenDisposition::NEW_FOREGROUND_TAB ||
-      nav_params->disposition == WindowOpenDisposition::NEW_BACKGROUND_TAB) {
-    nav_params->group = tab_strip_model_->GetTabGroupForTab(source_index);
-    if (!nav_params->group)
-      nav_params->group = tab_strip_model_->AddToNewGroup({source_index});
-  }
-}
-
 ///////////////////////////////////////////////////////////////////////////////
 // Browser, Getters for UI (private):
 

chrome/browser/ui/browser.h

@@ -887,11 +887,6 @@ class Browser : public TabStripModelObserver,
   // Removes all entries from scheduled_updates_ whose source is contents.
   void RemoveScheduledUpdatesFor(content::WebContents* contents);
 
-  // Configures |nav_params| to create a new tab group with the source, if
-  // applicable.
-  void ConfigureTabGroupForNavigation(content::WebContents* source,
-                                      NavigateParams* nav_params);
-
   // Getters for UI ///////////////////////////////////////////////////////////
 
   // TODO(beng): remove, and provide AutomationProvider a better way to access

chrome/browser/ui/browser_browsertest.cc

@@ -920,6 +920,30 @@ IN_PROC_BROWSER_TEST_F(BrowserTestWithTabGroupsEnabled,
   EXPECT_EQ(group_id, browser()->tab_strip_model()->GetTabGroupForTab(1));
 }
 
+IN_PROC_BROWSER_TEST_F(BrowserTestWithTabGroupsEnabled,
+                       TargetBlankLinkOpensInGroup) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Add a grouped tab.
+  TabStripModel* const model = browser()->tab_strip_model();
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL(
+                     "/frame_tree/anchor_to_same_site_location.html"));
+  const TabGroupId group_id = model->AddToNewGroup({0});
+
+  // Click a target=_blank link.
+  WebContents* const contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(ExecuteScript(
+      contents, "simulateClick(\"test-anchor-with-blank-target\", {})"));
+
+  // The new tab should have inherited the tab group from the first tab.
+  EXPECT_EQ(group_id, browser()->tab_strip_model()->GetTabGroupForTab(1));
+}
+
+// TODO(crbug.com/997344): Test the above two scenarios with implicitly-created
+// links, if still applicable.
+
 // BeforeUnloadAtQuitWithTwoWindows is a regression test for
 // http://crbug.com/11842. It opens two windows, one of which has a
 // beforeunload handler and attempts to exit cleanly.

chrome/browser/ui/browser_commands.cc

@@ -240,11 +240,15 @@ WebContents* GetTabAndRevertIfNecessaryHelper(Browser* browser,
       WebContents* raw_new_tab = new_tab.get();
       if (disposition == WindowOpenDisposition::NEW_BACKGROUND_TAB)
         new_tab->WasHidden();
+      const int index =
+          browser->tab_strip_model()->GetIndexOfWebContents(current_tab);
+      const auto group = browser->tab_strip_model()->GetTabGroupForTab(index);
       browser->tab_strip_model()->AddWebContents(
           std::move(new_tab), -1, ui::PAGE_TRANSITION_LINK,
           (disposition == WindowOpenDisposition::NEW_FOREGROUND_TAB)
               ? TabStripModel::ADD_ACTIVE
-              : TabStripModel::ADD_NONE);
+              : TabStripModel::ADD_NONE,
+          group);
       return raw_new_tab;
     }
     case WindowOpenDisposition::NEW_WINDOW: {

chrome/browser/ui/browser_tabstrip.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/tab_contents/core_tab_helper.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/ui_features.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/navigation_entry.h"
@@ -68,6 +69,9 @@ void AddWebContents(Browser* browser,
   // was created without a user gesture, we have to set |user_gesture| to true,
   // so it gets correctly focused.
   params.user_gesture = true;
+
+  ConfigureTabGroupForNavigation(&params);
+
   Navigate(&params);
 }
 
@@ -85,4 +89,34 @@ void CloseWebContents(Browser* browser,
                             : TabStripModel::CLOSE_NONE);
 }
 
+void ConfigureTabGroupForNavigation(NavigateParams* nav_params) {
+  if (!base::FeatureList::IsEnabled(features::kTabGroups))
+    return;
+
+  if (!nav_params->source_contents)
+    return;
+
+  if (!nav_params->browser || !nav_params->browser->SupportsWindowFeature(
+                                  Browser::WindowFeature::FEATURE_TABSTRIP)) {
+    return;
+  }
+
+  TabStripModel* model = nav_params->browser->tab_strip_model();
+  const int source_index =
+      model->GetIndexOfWebContents(nav_params->source_contents);
+
+  // If the source tab is pinned, don't create a group.
+  if (model->IsTabPinned(source_index))
+    return;
+
+  if (nav_params->disposition == WindowOpenDisposition::NEW_FOREGROUND_TAB ||
+      nav_params->disposition == WindowOpenDisposition::NEW_BACKGROUND_TAB) {
+    nav_params->group = model->GetTabGroupForTab(source_index);
+    // TODO(crbug.com / 997344): Re-evaluate implicit link creation, and either
+    // remove this or add tests.
+    if (!nav_params->group)
+      nav_params->group = model->AddToNewGroup({source_index});
+  }
+}
+
 }  // namespace chrome

chrome/browser/ui/browser_tabstrip.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_UI_BROWSER_TABSTRIP_H_
 
 #include "base/optional.h"
+#include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/tabs/tab_group_id.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/web_contents.h"
@@ -52,6 +53,10 @@ void CloseWebContents(Browser* browser,
                       content::WebContents* contents,
                       bool add_to_history);
 
+// Configures |nav_params| to create a new tab group with the source, if
+// applicable.
+void ConfigureTabGroupForNavigation(NavigateParams* nav_params);
+
 }  // namespace chrome
 
 #endif  // CHROME_BROWSER_UI_BROWSER_TABSTRIP_H_