WebUI Tab Strip: Allow dragging to move tab groups

Dragging a tab group around acts the same as dragging a tab. The
dragged tab group takes the index of the tab or group it is being
dragged over.

Bug: 1027373
Change-Id: Ie70883a16330e09732e1bad02e2112d2511cb51e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2018499
Commit-Queue: John Lee <johntlee@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735632}

chrome/browser/resources/tab_strip/tab.js

@@ -252,10 +252,10 @@ export class TabElement extends CustomElement {
   }
 
   /**
-   * @param {boolean} dragging
+   * @param {boolean} isDragging
    */
-  setDragging(dragging) {
-    this.toggleAttribute('dragging_', dragging);
+  setDragging(isDragging) {
+    this.toggleAttribute('dragging_', isDragging);
   }
 
   /**

chrome/browser/resources/tab_strip/tab_group.js

@@ -10,6 +10,21 @@ export class TabGroupElement extends CustomElement {
     return `{__html_template__}`;
   }
 
+  connectedCallback() {
+    this.setAttribute('draggable', 'true');
+  }
+
+  /** @return {!HTMLElement} */
+  getDragImage() {
+    // TODO(johntlee): Update drag image.
+    return this;
+  }
+
+  /** @param {boolean} isDragging */
+  setDragging(isDragging) {
+    // TODO(johntlee): Update UI when dragging.
+  }
+
   /**
    * @param {!TabGroupVisualData} visualData
    */

chrome/browser/resources/tab_strip/tab_list.js

@@ -91,8 +91,8 @@ class TabListElement extends CustomElement {
         this.onDocumentVisibilityChange_();
 
     /**
-     * The TabElement that is currently being dragged.
-     * @private {!TabElement|undefined}
+     * The element that is currently being dragged.
+     * @private {!TabElement|!TabGroupElement|undefined}
      */
     this.draggedItem_;
 
@@ -490,18 +490,56 @@ class TabListElement extends CustomElement {
     }
 
     event.dataTransfer.dropEffect = 'move';
+    if (isTabGroupElement(this.draggedItem_)) {
+      this.onDragOverWithGroupElement_(event);
+      return;
+    }
+    this.onDragOverWithTabElement_(event);
+  }
+
+  /**
+   * @param {!DragEvent} event
+   * @private
+   */
+  onDragOverWithGroupElement_(event) {
+    const composedPath = /** @type {!Array<!Element>} */ (event.composedPath());
+    if (composedPath.includes(assert(this.draggedItem_))) {
+      // Dragging over itself or a child of itself.
+      return;
+    }
+
+    const allTabElements =
+        Array.from(this.shadowRoot.querySelectorAll('tabstrip-tab'));
 
-    const composedPath = event.composedPath();
-    const dragOverTabElement =
-        composedPath.find(item => isTabElement(/** @type {!Element} */ (item)));
+    const dragOverTabElement = composedPath.find(isTabElement);
+    if (dragOverTabElement && !dragOverTabElement.tab.pinned) {
+      const dragOverIndex = allTabElements.indexOf(dragOverTabElement);
+      this.tabsApi_.moveGroup(this.draggedItem_.dataset.groupId, dragOverIndex);
+      return;
+    }
+
+    const dragOverGroupElement = composedPath.find(isTabGroupElement);
+    if (dragOverGroupElement) {
+      const dragOverIndex =
+          allTabElements.indexOf(dragOverGroupElement.firstElementChild);
+      this.tabsApi_.moveGroup(this.draggedItem_.dataset.groupId, dragOverIndex);
+    }
+  }
+
+  /**
+   * @param {!DragEvent} event
+   * @private
+   */
+  onDragOverWithTabElement_(event) {
+    const composedPath = /** @type {!Array<!Element>} */ (event.composedPath());
+    const dragOverTabElement = composedPath.find(isTabElement);
     if (dragOverTabElement &&
         dragOverTabElement.tab.pinned !== this.draggedItem_.tab.pinned) {
       // Can only drag between the same pinned states.
       return;
     }
 
-    const dragOverTabGroup = composedPath.find(
-        item => isTabGroupElement(/** @type {!Element} */ (item)));
+    const dragOverTabGroup = composedPath.find(isTabGroupElement);
     if (dragOverTabGroup &&
         dragOverTabGroup.dataset.groupId !== this.draggedItem_.tab.groupId) {
       this.tabsApi_.groupTab(
@@ -529,7 +567,7 @@ class TabListElement extends CustomElement {
    */
   onDragStart_(event) {
     const draggedItem = event.path[0];
-    if (!isTabElement(draggedItem)) {
+    if (!isTabElement(draggedItem) && !isTabGroupElement(draggedItem)) {
       return;
     }
 

chrome/browser/resources/tab_strip/tabs_api_proxy.js

@@ -129,6 +129,14 @@ export class TabsApiProxy {
     chrome.send('groupTab', [tabId, groupId]);
   }
 
+  /**
+   * @param {string} groupId
+   * @param {number} newIndex
+   */
+  moveGroup(groupId, newIndex) {
+    chrome.send('moveGroup', [groupId, newIndex]);
+  }
+
   /**
    * @param {number} tabId
    * @param {number} newIndex

chrome/browser/ui/webui/tab_strip/tab_strip_ui_handler.cc

@@ -201,6 +201,12 @@ void TabStripUIHandler::OnTabGroupChanged(const TabGroupChange& change) {
     }
 
     case TabGroupChange::kMoved: {
+      FireWebUIListener("tab-group-moved", base::Value(change.group.ToString()),
+                        base::Value(browser_->tab_strip_model()
+                                        ->group_model()
+                                        ->GetTabGroup(change.group)
+                                        ->ListTabs()
+                                        .front()));
       break;
     }
 
@@ -253,6 +259,8 @@ void TabStripUIHandler::OnTabStripModelChanged(
     case TabStripModelChange::kMoved: {
       auto* move = change.GetMove();
 
+      // TODO(johntlee): Investigate if this is still needed, when
+      // TabGroupChange::kMoved exists.
       base::Optional<tab_groups::TabGroupId> tab_group_id =
           tab_strip_model->GetTabGroupForTab(move->to_index);
       if (tab_group_id.has_value()) {
@@ -346,6 +354,9 @@ void TabStripUIHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
       "ungroupTab",
       base::Bind(&TabStripUIHandler::HandleUngroupTab, base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "moveGroup",
+      base::Bind(&TabStripUIHandler::HandleMoveGroup, base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "setThumbnailTracked",
       base::Bind(&TabStripUIHandler::HandleSetThumbnailTracked,
@@ -567,6 +578,18 @@ void TabStripUIHandler::HandleUngroupTab(const base::ListValue* args) {
   browser_->tab_strip_model()->RemoveFromGroup({tab_index});
 }
 
+void TabStripUIHandler::HandleMoveGroup(const base::ListValue* args) {
+  const std::string group_id_string = args->GetList()[0].GetString();
+
+  for (tab_groups::TabGroupId group_id :
+       browser_->tab_strip_model()->group_model()->ListTabGroups()) {
+    if (group_id.ToString() == group_id_string) {
+      int to_index = args->GetList()[1].GetInt();
+      browser_->tab_strip_model()->MoveGroupTo(group_id, to_index);
+    }
+  }
+}
+
 void TabStripUIHandler::HandleCloseContainer(const base::ListValue* args) {
   // We only autoclose for tab selection.
   RecordTabStripUICloseHistogram(TabStripUICloseAction::kTabSelected);

chrome/browser/ui/webui/tab_strip/tab_strip_ui_handler.h

@@ -51,6 +51,7 @@ class TabStripUIHandler : public content::WebUIMessageHandler,
  private:
   FRIEND_TEST_ALL_PREFIXES(TabStripUIHandlerTest, GetGroupVisualData);
   FRIEND_TEST_ALL_PREFIXES(TabStripUIHandlerTest, GroupTab);
+  FRIEND_TEST_ALL_PREFIXES(TabStripUIHandlerTest, MoveGroup);
   FRIEND_TEST_ALL_PREFIXES(TabStripUIHandlerTest, UngroupTab);
 
   void HandleCreateNewTab(const base::ListValue* args);
@@ -65,6 +66,7 @@ class TabStripUIHandler : public content::WebUIMessageHandler,
   void HandleGetLayout(const base::ListValue* args);
   void HandleGroupTab(const base::ListValue* args);
   void HandleUngroupTab(const base::ListValue* args);
+  void HandleMoveGroup(const base::ListValue* args);
   void HandleSetThumbnailTracked(const base::ListValue* args);
   void HandleReportTabActivationDuration(const base::ListValue* args);
   void HandleReportTabDataReceivedDuration(const base::ListValue* args);

chrome/browser/ui/webui/tab_strip/tab_strip_ui_handler_unittest.cc

@@ -302,15 +302,37 @@ TEST_F(TabStripUIHandlerTest, GroupTab) {
   tab_groups::TabGroupId group_id =
       browser()->tab_strip_model()->AddToNewGroup({0});
 
-  // Add another tab at index 1, and try to group it.
+  // Add another tab, and try to group it.
   AddTab(browser(), GURL("http://foo"));
   base::ListValue args;
   args.AppendInteger(extensions::ExtensionTabUtil::GetTabId(
-      browser()->tab_strip_model()->GetWebContentsAt(1)));
+      browser()->tab_strip_model()->GetWebContentsAt(0)));
   args.AppendString(group_id.ToString());
   handler()->HandleGroupTab(&args);
 
-  ASSERT_EQ(group_id, browser()->tab_strip_model()->GetTabGroupForTab(1));
+  ASSERT_EQ(group_id, browser()->tab_strip_model()->GetTabGroupForTab(0));
+}
+
+TEST_F(TabStripUIHandlerTest, MoveGroup) {
+  AddTab(browser(), GURL("http://foo/1"));
+  AddTab(browser(), GURL("http://foo/2"));
+  tab_groups::TabGroupId group_id =
+      browser()->tab_strip_model()->AddToNewGroup({0});
+
+  // Move the group to index 1.
+  int new_index = 1;
+  base::ListValue args;
+  args.AppendString(group_id.ToString());
+  args.AppendInteger(new_index);
+  handler()->HandleMoveGroup(&args);
+
+  std::vector<int> tabs_in_group = browser()
+                                       ->tab_strip_model()
+                                       ->group_model()
+                                       ->GetTabGroup(group_id)
+                                       ->ListTabs();
+  ASSERT_EQ(new_index, tabs_in_group.front());
+  ASSERT_EQ(new_index, tabs_in_group.back());
 }
 
 TEST_F(TabStripUIHandlerTest, UngroupTab) {

chrome/test/data/webui/tab_strip/tab_list_test.js

@@ -554,7 +554,7 @@ suite('TabList', () => {
     assertEquals(newIndex, dragOverIndex);
   });
 
-  test('DragOverTabGroup', async () => {
+  test('DragTabOverTabGroup', async () => {
     const tabElements = getUnpinnedTabs();
 
     // Group the first tab.
@@ -586,7 +586,7 @@ suite('TabList', () => {
     assertEquals('group0', groupId);
   });
 
-  test('DragOutOfTabGroup', async () => {
+  test('DragTabOutOfTabGroup', async () => {
     const tabElements = getUnpinnedTabs();
 
     // Group the first tab.
@@ -606,7 +606,6 @@ suite('TabList', () => {
     draggedTab.dispatchEvent(dragStartEvent);
 
     // Drag the first tab out.
-    const dragOverTabGroup = getTabGroups()[0];
     const dragOverEvent = new DragEvent('dragover', {
       bubbles: true,
       composed: true,
@@ -617,6 +616,72 @@ suite('TabList', () => {
     assertEquals(draggedTab.tab.id, tabId);
   });
 
+  test('DragGroupOverTab', async () => {
+    const tabElements = getUnpinnedTabs();
+
+    // Group the first tab.
+    webUIListenerCallback(
+        'tab-group-state-changed', tabElements[0].tab.id, 0, 'group0');
+
+    // Start dragging the group.
+    const draggedGroup = getTabGroups()[0];
+    const mockDataTransfer = new MockDataTransfer();
+    const dragStartEvent = new DragEvent('dragstart', {
+      bubbles: true,
+      composed: true,
+      clientX: 100,
+      clientY: 150,
+      dataTransfer: mockDataTransfer,
+    });
+    draggedGroup.dispatchEvent(dragStartEvent);
+
+    // Drag the group over the second tab.
+    const dragOverTab = tabElements[1];
+    const dragOverEvent = new DragEvent('dragover', {
+      bubbles: true,
+      composed: true,
+      dataTransfer: mockDataTransfer,
+    });
+    dragOverTab.dispatchEvent(dragOverEvent);
+    const [groupId, index] = await testTabsApiProxy.whenCalled('moveGroup');
+    assertEquals('group0', groupId);
+    assertEquals(1, index);
+  });
+
+  test('DragGroupOverGroup', async () => {
+    const tabElements = getUnpinnedTabs();
+
+    // Group the first tab and second tab separately.
+    webUIListenerCallback(
+        'tab-group-state-changed', tabElements[0].tab.id, 0, 'group0');
+    webUIListenerCallback(
+        'tab-group-state-changed', tabElements[1].tab.id, 1, 'group1');
+
+    // Start dragging the first group.
+    const draggedGroup = getTabGroups()[0];
+    const mockDataTransfer = new MockDataTransfer();
+    const dragStartEvent = new DragEvent('dragstart', {
+      bubbles: true,
+      composed: true,
+      clientX: 100,
+      clientY: 150,
+      dataTransfer: mockDataTransfer,
+    });
+    draggedGroup.dispatchEvent(dragStartEvent);
+
+    // Drag the group over the second tab.
+    const dragOverGroup = getTabGroups()[1];
+    const dragOverEvent = new DragEvent('dragover', {
+      bubbles: true,
+      composed: true,
+      dataTransfer: mockDataTransfer,
+    });
+    dragOverGroup.dispatchEvent(dragOverEvent);
+    const [groupId, index] = await testTabsApiProxy.whenCalled('moveGroup');
+    assertEquals('group0', groupId);
+    assertEquals(1, index);
+  });
+
   test('tracks and untracks thumbnails based on viewport', async () => {
     // Wait for slideIn animations to complete updating widths and reset
     // resolvers to track new calls.

chrome/test/data/webui/tab_strip/test_tabs_api_proxy.js

@@ -13,6 +13,7 @@ export class TestTabsApiProxy extends TestBrowserProxy {
       'getGroupVisualData',
       'getTabs',
       'groupTab',
+      'moveGroup',
       'moveTab',
       'setThumbnailTracked',
       'ungroupTab',
@@ -50,6 +51,10 @@ export class TestTabsApiProxy extends TestBrowserProxy {
     this.methodCalled('groupTab', [tabId, groupId]);
   }
 
+  moveGroup(groupId, newIndex) {
+    this.methodCalled('moveGroup', [groupId, newIndex]);
+  }
+
   moveTab(tabId, newIndex) {
     this.methodCalled('moveTab', [tabId, newIndex]);
     return Promise.resolve();