Support moving a group left/right (Tab Group a11y)

This follows a similar pattern to how tabs are moved left/right: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.cc?l=1591&rcl=3e23198594e08a63cc5e091e1eefe79e3dd48454

I deliberately chose not to support MoveGroupFirst() and MoveGroupLast(), since the implementation seemed too finicky and the feature seemed very niche. We can return to it if we decide it's higher priority for accessibility.

Bug: 1039921
Change-Id: Ifcd078d640a84fe96f15c41a38daa82a5090641c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2006219
Commit-Queue: Connie Wan <connily@chromium.org>
Reviewed-by: Taylor Bergquist <tbergquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734224}

chrome/browser/ui/tabs/tab_strip_model.cc

@@ -668,6 +668,24 @@ void TabStripModel::MoveSelectedTabsTo(int index) {
                          selected_count - selected_pinned_count);
 }
 
+void TabStripModel::MoveGroupTo(const tab_groups::TabGroupId& group,
+                                int to_index) {
+  ReentrancyCheck reentrancy_check(&reentrancy_guard_);
+
+  DCHECK_NE(to_index, kNoTab);
+
+  std::vector<int> tabs_in_group = group_model_->GetTabGroup(group)->ListTabs();
+
+  int from_index = tabs_in_group.front();
+  if (to_index < from_index)
+    from_index = tabs_in_group.back();
+
+  for (size_t i = 0; i < tabs_in_group.size(); ++i)
+    MoveWebContentsAtImpl(from_index, to_index, false);
+
+  MoveTabGroup(group);
+}
+
 WebContents* TabStripModel::GetActiveWebContents() const {
   return GetWebContentsAt(active_index());
 }

chrome/browser/ui/tabs/tab_strip_model.h

@@ -285,6 +285,10 @@ class TabStripModel : public TabGroupController {
   // index + selected-pinned tab-count (3 + 1).
   void MoveSelectedTabsTo(int index);
 
+  // Moves all tabs in |group| to |to_index|. This has no checks to make sure
+  // the position is valid for a group to move to.
+  void MoveGroupTo(const tab_groups::TabGroupId& group, int to_index);
+
   // Returns the currently active WebContents, or NULL if there is none.
   content::WebContents* GetActiveWebContents() const;
 

chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc

@@ -345,6 +345,11 @@ void BrowserTabStripController::MoveTab(int start_index, int final_index) {
   model_->MoveWebContentsAt(start_index, final_index, false);
 }
 
+void BrowserTabStripController::MoveGroup(const tab_groups::TabGroupId& group,
+                                          int final_index) {
+  model_->MoveGroupTo(group, final_index);
+}
+
 void BrowserTabStripController::ShowContextMenuForTab(
     Tab* tab,
     const gfx::Point& p,

chrome/browser/ui/views/tabs/browser_tab_strip_controller.h

@@ -70,6 +70,7 @@ class BrowserTabStripController : public TabStripController,
                      const tab_groups::TabGroupId& group) override;
   void RemoveTabFromGroup(int model_index) override;
   void MoveTab(int start_index, int final_index) override;
+  void MoveGroup(const tab_groups::TabGroupId& group, int final_index) override;
   void ShowContextMenuForTab(Tab* tab,
                              const gfx::Point& p,
                              ui::MenuSourceType source_type) override;

chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc

@@ -49,6 +49,8 @@ void FakeBaseTabStripController::MoveTab(int from_index, int to_index) {
   tab_groups_.insert(tab_groups_.begin() + to_index, prev_group);
   tab_strip_->MoveTab(from_index, to_index, TabRendererData());
 }
+void FakeBaseTabStripController::MoveGroup(const tab_groups::TabGroupId& group,
+                                           int to_index) {}
 
 void FakeBaseTabStripController::RemoveTab(int index) {
   num_tabs_--;

chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.h

@@ -47,6 +47,7 @@ class FakeBaseTabStripController : public TabStripController {
   bool BeforeCloseTab(int index, CloseTabSource source) override;
   void CloseTab(int index, CloseTabSource source) override;
   void MoveTab(int from_index, int to_index) override;
+  void MoveGroup(const tab_groups::TabGroupId&, int to_index) override;
   void ShowContextMenuForTab(Tab* tab,
                              const gfx::Point& p,
                              ui::MenuSourceType source_type) override;

chrome/browser/ui/views/tabs/tab_group_header.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/strings/utf_string_conversions.h"
+#include "build/build_config.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/tabs/tab_style.h"
 #include "chrome/browser/ui/views/tabs/tab_controller.h"
@@ -107,7 +108,23 @@ bool TabGroupHeader::OnKeyPressed(const ui::KeyEvent& event) {
     return true;
   }
 
-  // TODO(connily): Handle moving the entire group left/right, similar to tabs.
+  constexpr int kModifiedFlag =
+#if defined(OS_MACOSX)
+      ui::EF_COMMAND_DOWN;
+#else
+      ui::EF_CONTROL_DOWN;
+#endif
+
+  if (event.type() == ui::ET_KEY_PRESSED && (event.flags() & kModifiedFlag)) {
+    if (event.key_code() == ui::VKEY_RIGHT) {
+      tab_strip_->ShiftGroupRight(group().value());
+      return true;
+    }
+    if (event.key_code() == ui::VKEY_LEFT) {
+      tab_strip_->ShiftGroupLeft(group().value());
+      return true;
+    }
+  }
 
   return false;
 }

chrome/browser/ui/views/tabs/tab_strip.cc

@@ -1306,6 +1306,14 @@ void TabStrip::OnGroupClosed(const tab_groups::TabGroupId& group) {
   // The group_views_ mapping is erased in OnGroupCloseAnimationCompleted().
 }
 
+void TabStrip::ShiftGroupLeft(const tab_groups::TabGroupId& group) {
+  ShiftGroupRelative(group, -1);
+}
+
+void TabStrip::ShiftGroupRight(const tab_groups::TabGroupId& group) {
+  ShiftGroupRelative(group, 1);
+}
+
 bool TabStrip::ShouldTabBeVisible(const Tab* tab) const {
   // Detached tabs should always be invisible (as they close).
   if (tab->detached())
@@ -2964,6 +2972,39 @@ void TabStrip::ShiftTabRelative(Tab* tab, int offset) {
   controller_->MoveTab(start_index, target_index);
 }
 
+void TabStrip::ShiftGroupRelative(const tab_groups::TabGroupId& group,
+                                  int offset) {
+  DCHECK(offset == ShiftOffset::kLeft || offset == ShiftOffset::kRight);
+  std::vector<int> tabs_in_group = controller_->ListTabsInGroup(group);
+
+  const int start_index = tabs_in_group.front();
+  int target_index = start_index + offset;
+
+  if (offset > 0)
+    target_index += tabs_in_group.size() - 1;
+
+  if (!IsValidModelIndex(start_index) || !IsValidModelIndex(target_index))
+    return;
+
+  // Avoid moving into the middle of another group by accounting for its size.
+  base::Optional<tab_groups::TabGroupId> target_group =
+      tab_at(target_index)->group();
+  if (target_group.has_value()) {
+    target_index +=
+        offset *
+        (controller_->ListTabsInGroup(target_group.value()).size() - 1);
+  }
+
+  if (!IsValidModelIndex(target_index))
+    return;
+
+  if (controller_->IsTabPinned(start_index) !=
+      controller_->IsTabPinned(target_index))
+    return;
+
+  controller_->MoveGroup(group, target_index);
+}
+
 void TabStrip::ResizeLayoutTabs() {
   // We've been called back after the TabStrip has been emptied out (probably
   // just prior to the window being destroyed). We need to do nothing here or

chrome/browser/ui/views/tabs/tab_strip.h

@@ -192,6 +192,12 @@ class TabStrip : public views::AccessiblePaneView,
   // associated view mappings are erased in OnGroupCloseAnimationCompleted().
   void OnGroupClosed(const tab_groups::TabGroupId& group);
 
+  // Attempts to move the specified group to the left.
+  void ShiftGroupLeft(const tab_groups::TabGroupId& group);
+
+  // Attempts to move the specified group to the right.
+  void ShiftGroupRight(const tab_groups::TabGroupId& group);
+
   // Returns true if the tab is not partly or fully clipped (due to overflow),
   // and the tab couldn't become partly clipped due to changing the selected tab
   // (for example, if currently the strip has the last tab selected, and
@@ -529,6 +535,10 @@ class TabStrip : public views::AccessiblePaneView,
   // and moves it if possible.
   void ShiftTabRelative(Tab* tab, int offset);
 
+  // Determines whether a group can be shifted by one in the direction of
+  // |offset| and moves it if possible.
+  void ShiftGroupRelative(const tab_groups::TabGroupId& group, int offset);
+
   // -- Tab Resize Layout -----------------------------------------------------
 
   // Perform an animated resize-relayout of the TabStrip immediately.

chrome/browser/ui/views/tabs/tab_strip_browsertest.cc

@@ -31,6 +31,10 @@ class TabStripBrowsertest : public InProcessBrowserTest {
     return tab_strip_model()->GetTabGroupForTab(tab_index).value();
   }
 
+  void AddTabToExistingGroup(int tab_index, tab_groups::TabGroupId group) {
+    tab_strip_model()->AddToExistingGroup({tab_index}, group);
+  }
+
   std::vector<content::WebContents*> GetWebContentses() {
     std::vector<content::WebContents*> contentses;
     for (int i = 0; i < tab_strip()->tab_count(); ++i)
@@ -397,3 +401,101 @@ IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, MoveTabLast_AllPinnedTabs_Failure) {
   // No changes expected.
   EXPECT_EQ(contentses, GetWebContentses());
 }
+
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftGroupLeft_Success) {
+  AppendTab();
+  AppendTab();
+
+  tab_groups::TabGroupId group = AddTabToNewGroup(1);
+  AddTabToExistingGroup(2, group);
+
+  const auto expected = GetWebContentsesInOrder({1, 2, 0});
+  tab_strip()->ShiftGroupLeft(group);
+  EXPECT_EQ(expected, GetWebContentses());
+}
+
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftGroupLeft_OtherGroup) {
+  AppendTab();
+  AppendTab();
+  AppendTab();
+
+  tab_groups::TabGroupId group1 = AddTabToNewGroup(2);
+  AddTabToExistingGroup(3, group1);
+
+  tab_groups::TabGroupId group2 = AddTabToNewGroup(0);
+  AddTabToExistingGroup(1, group2);
+
+  const auto expected = GetWebContentsesInOrder({2, 3, 0, 1});
+  tab_strip()->ShiftGroupLeft(group1);
+  EXPECT_EQ(expected, GetWebContentses());
+}
+
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
+                       ShiftGroupLeft_Failure_EdgeOfTabstrip) {
+  AppendTab();
+  AppendTab();
+
+  tab_groups::TabGroupId group = AddTabToNewGroup(0);
+  AddTabToExistingGroup(1, group);
+
+  const auto contentses = GetWebContentses();
+  tab_strip()->ShiftGroupLeft(group);
+  // No change expected.
+  EXPECT_EQ(contentses, GetWebContentses());
+}
+
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftGroupLeft_Failure_Pinned) {
+  AppendTab();
+  AppendTab();
+  tab_strip_model()->SetTabPinned(0, true);
+
+  tab_groups::TabGroupId group = AddTabToNewGroup(1);
+  AddTabToExistingGroup(2, group);
+
+  const auto contentses = GetWebContentses();
+  tab_strip()->ShiftGroupLeft(group);
+  // No change expected.
+  EXPECT_EQ(contentses, GetWebContentses());
+}
+
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftGroupRight_Success) {
+  AppendTab();
+  AppendTab();
+
+  tab_groups::TabGroupId group = AddTabToNewGroup(0);
+  AddTabToExistingGroup(1, group);
+
+  const auto expected = GetWebContentsesInOrder({2, 0, 1});
+  tab_strip()->ShiftGroupRight(group);
+  EXPECT_EQ(expected, GetWebContentses());
+}
+
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, ShiftGroupRight_OtherGroup) {
+  AppendTab();
+  AppendTab();
+  AppendTab();
+
+  tab_groups::TabGroupId group1 = AddTabToNewGroup(0);
+  AddTabToExistingGroup(1, group1);
+
+  tab_groups::TabGroupId group2 = AddTabToNewGroup(2);
+  AddTabToExistingGroup(3, group2);
+
+  const auto expected = GetWebContentsesInOrder({2, 3, 0, 1});
+  tab_strip()->ShiftGroupRight(group1);
+  EXPECT_EQ(expected, GetWebContentses());
+}
+
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest,
+                       ShiftGroupRight_Failure_EdgeOfTabstrip) {
+  AppendTab();
+  AppendTab();
+
+  tab_groups::TabGroupId group = AddTabToNewGroup(1);
+  AddTabToExistingGroup(2, group);
+
+  const auto contentses = GetWebContentses();
+  tab_strip()->ShiftGroupRight(group);
+  // No change expected.
+  EXPECT_EQ(contentses, GetWebContentses());
+}

chrome/browser/ui/views/tabs/tab_strip_controller.h

@@ -99,6 +99,11 @@ class TabStripController {
   // any tabs in between left or right as appropriate.
   virtual void MoveTab(int start_index, int final_index) = 0;
 
+  // Moves all the tabs in |group| so that it is now at |final_index|, sliding
+  // any tabs in between left or right as appropriate.
+  virtual void MoveGroup(const tab_groups::TabGroupId& group,
+                         int final_index) = 0;
+
   // Shows a context menu for the tab at the specified point in screen coords.
   virtual void ShowContextMenuForTab(Tab* tab,
                                      const gfx::Point& p,