WebUI Tab Strip: Handle group move events

Currently, when a group is moved, there is a tab-moved event that is
fired for every single tab in the group. This leads to issues when
trying to preserve the parent-child relationship of the TabGroup
and the TabElement when certain tab-moved events have fired before
others and leads to temporary re-ordering of tabs within the group.

This CL coalesces multiple tab-moved events to a synthetic group-moved
event only after every tab-moved event is fired, so that the TabGroup
element is only moved once.

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

chrome/browser/resources/tab_strip/tab_list.js

@@ -8,7 +8,7 @@ import 'chrome://resources/cr_elements/cr_icon_button/cr_icon_button.m.js';
 import 'chrome://resources/cr_elements/icons.m.js';
 
 import {assert} from 'chrome://resources/js/assert.m.js';
-import {addWebUIListener} from 'chrome://resources/js/cr.m.js';
+import {addWebUIListener, removeWebUIListener, WebUIListener} from 'chrome://resources/js/cr.m.js';
 import {FocusOutlineManager} from 'chrome://resources/js/cr/ui/focus_outline_manager.m.js';
 import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
 import {isRTL} from 'chrome://resources/js/util.m.js';
@@ -144,18 +144,21 @@ class TabListElement extends CustomElement {
         /** @type {!Element} */ (
             this.shadowRoot.querySelector('#unpinnedTabs'));
 
+    /** @private {!Array<!WebUIListener>} */
+    this.webUIListeners_ = [];
+
     /** @private {!Function} */
     this.windowBlurListener_ = () => this.onWindowBlur_();
 
     /** @private {!Function} */
     this.contextMenuListener_ = e => this.onContextMenu_(e);
 
-    addWebUIListener(
+    this.addWebUIListener_(
         'layout-changed', layout => this.applyCSSDictionary_(layout));
-    addWebUIListener('theme-changed', () => this.fetchAndUpdateColors_());
+    this.addWebUIListener_('theme-changed', () => this.fetchAndUpdateColors_());
     this.tabStripEmbedderProxy_.observeThemeChanges();
 
-    addWebUIListener(
+    this.addWebUIListener_(
         'tab-thumbnail-updated', this.tabThumbnailUpdated_.bind(this));
 
     this.addEventListener(
@@ -168,7 +171,7 @@ class TabListElement extends CustomElement {
     document.addEventListener('contextmenu', this.contextMenuListener_);
     document.addEventListener(
         'visibilitychange', this.documentVisibilityChangeListener_);
-    addWebUIListener(
+    this.addWebUIListener_(
         'received-keyboard-focus', () => this.onReceivedKeyboardFocus_());
     window.addEventListener('blur', this.windowBlurListener_);
 
@@ -196,6 +199,15 @@ class TabListElement extends CustomElement {
     this.animationPromises = this.animationPromises.then(() => promise);
   }
 
+  /**
+   * @param {string} eventName
+   * @param {!Function} callback
+   * @private
+   */
+  addWebUIListener_(eventName, callback) {
+    this.webUIListeners_.push(addWebUIListener(eventName, callback));
+  }
+
   /**
    * @param {number} scrollBy
    * @private
@@ -265,21 +277,24 @@ class TabListElement extends CustomElement {
       this.tabStripEmbedderProxy_.reportTabCreationDuration(
           tabs.length, Date.now() - createTabsStartTimestamp);
 
-      addWebUIListener('tab-created', tab => this.onTabCreated_(tab));
-      addWebUIListener(
+      this.addWebUIListener_('tab-created', tab => this.onTabCreated_(tab));
+      this.addWebUIListener_(
           'tab-moved', (tabId, newIndex) => this.onTabMoved_(tabId, newIndex));
-      addWebUIListener('tab-removed', tabId => this.onTabRemoved_(tabId));
-      addWebUIListener(
+      this.addWebUIListener_('tab-removed', tabId => this.onTabRemoved_(tabId));
+      this.addWebUIListener_(
           'tab-replaced', (oldId, newId) => this.onTabReplaced_(oldId, newId));
-      addWebUIListener('tab-updated', tab => this.onTabUpdated_(tab));
-      addWebUIListener(
+      this.addWebUIListener_('tab-updated', tab => this.onTabUpdated_(tab));
+      this.addWebUIListener_(
           'tab-active-changed', tabId => this.onTabActivated_(tabId));
-      addWebUIListener(
+      this.addWebUIListener_(
           'tab-group-state-changed',
           (tabId, index, groupId) =>
               this.onTabGroupStateChanged_(tabId, index, groupId));
-      addWebUIListener(
+      this.addWebUIListener_(
           'tab-group-closed', groupId => this.onTabGroupClosed_(groupId));
+      this.addWebUIListener_(
+          'tab-group-moved',
+          (groupId, index) => this.onTabGroupMoved_(groupId, index));
     });
   }
 
@@ -288,6 +303,7 @@ class TabListElement extends CustomElement {
     document.removeEventListener(
         'visibilitychange', this.documentVisibilityChangeListener_);
     window.removeEventListener('blur', this.windowBlurListener_);
+    this.webUIListeners_.forEach(removeWebUIListener);
   }
 
   /**
@@ -569,6 +585,28 @@ class TabListElement extends CustomElement {
     tabGroupElement.remove();
   }
 
+  /**
+   * @param {string} groupId
+   * @param {number} index
+   * @private
+   */
+  onTabGroupMoved_(groupId, index) {
+    const tabGroupElement = this.findTabGroupElement_(groupId);
+    if (!tabGroupElement) {
+      return;
+    }
+    tabGroupElement.remove();
+
+    let elementAtIndex =
+        this.shadowRoot.querySelectorAll('tabstrip-tab')[index];
+    if (elementAtIndex && elementAtIndex.parentElement &&
+        isTabGroupElement(elementAtIndex.parentElement)) {
+      elementAtIndex = elementAtIndex.parentElement;
+    }
+
+    this.unpinnedTabsElement_.insertBefore(tabGroupElement, elementAtIndex);
+  }
+
   /**
    * @param {number} tabId
    * @param {number} index

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

@@ -279,6 +279,31 @@ void TabStripUIHandler::OnTabStripModelChanged(
     }
     case TabStripModelChange::kMoved: {
       auto* move = change.GetMove();
+
+      base::Optional<tab_groups::TabGroupId> tab_group_id =
+          tab_strip_model->GetTabGroupForTab(move->to_index);
+      if (tab_group_id.has_value()) {
+        const std::vector<int> tabs_in_group =
+            tab_strip_model->group_model()
+                ->GetTabGroup(tab_group_id.value())
+                ->ListTabs();
+        if (tabs_in_group == selection.new_model.selected_indices()) {
+          // If the selection includes all the tabs within the changed tab's
+          // group, it is an indication that the entire group is being moved.
+          // To prevent sending multiple events for the same batch move, fire a
+          // separate single tab-group-moved event once all tabs have been
+          // moved. All tabs have moved only after all the indices in the group
+          // are in the correct continuous order.
+          if (tabs_in_group.back() - tabs_in_group.front() + 1 ==
+              static_cast<int>(tabs_in_group.size())) {
+            FireWebUIListener("tab-group-moved",
+                              base::Value(tab_group_id.value().ToString()),
+                              base::Value(tabs_in_group[0]));
+          }
+          break;
+        }
+      }
+
       FireWebUIListener(
           "tab-moved",
           base::Value(extensions::ExtensionTabUtil::GetTabId(move->contents)),

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

@@ -129,3 +129,65 @@ TEST_F(TabStripUIHandlerTest, GroupStateChangedEvents) {
   EXPECT_EQ(1, index);
   EXPECT_EQ(nullptr, ungrouped_data.arg4());
 }
+
+TEST_F(TabStripUIHandlerTest, GroupMovedEvents) {
+  // Create a tab group and a few other tabs to allow the group to move.
+  AddTab(browser(), GURL("http://foo/1"));
+  AddTab(browser(), GURL("http://foo/2"));
+  AddTab(browser(), GURL("http://foo/3"));
+  AddTab(browser(), GURL("http://foo/4"));
+  tab_groups::TabGroupId expected_group_id =
+      browser()->tab_strip_model()->AddToNewGroup({0, 1});
+
+  // Select all the tabs in the group.
+  ui::ListSelectionModel selection;
+  selection.AddIndexToSelection(0);
+  selection.AddIndexToSelection(1);
+  selection.set_active(0);
+  browser()->tab_strip_model()->SetSelectionFromModel(selection);
+
+  web_ui()->ClearTrackedCalls();
+
+  // Move the selected tabs to later in the tab strip. This should result in
+  // a single event that is fired to indicate the entire group has moved.
+  int expected_index = 2;
+  browser()->tab_strip_model()->MoveSelectedTabsTo(expected_index);
+
+  EXPECT_EQ(1U, web_ui()->call_data().size());
+
+  const content::TestWebUI::CallData& grouped_data =
+      *web_ui()->call_data().back();
+  EXPECT_EQ("cr.webUIListenerCallback", grouped_data.function_name());
+
+  std::string event_name;
+  ASSERT_TRUE(grouped_data.arg1()->GetAsString(&event_name));
+  EXPECT_EQ("tab-group-moved", event_name);
+
+  std::string actual_group_id;
+  ASSERT_TRUE(grouped_data.arg2()->GetAsString(&actual_group_id));
+  EXPECT_EQ(expected_group_id.ToString(), actual_group_id);
+
+  int actual_index;
+  ASSERT_TRUE(grouped_data.arg3()->GetAsInteger(&actual_index));
+  EXPECT_EQ(expected_index, actual_index);
+
+  web_ui()->ClearTrackedCalls();
+
+  // Move the selected tabs to earlier in the tab strip. This should also
+  // result in a single event that is fired to indicate the entire group has
+  // moved.
+  expected_index = 1;
+  browser()->tab_strip_model()->MoveSelectedTabsTo(expected_index);
+
+  EXPECT_EQ(1U, web_ui()->call_data().size());
+
+  const content::TestWebUI::CallData& grouped_data2 =
+      *web_ui()->call_data().back();
+  EXPECT_EQ("cr.webUIListenerCallback", grouped_data2.function_name());
+  ASSERT_TRUE(grouped_data2.arg1()->GetAsString(&event_name));
+  EXPECT_EQ("tab-group-moved", event_name);
+  ASSERT_TRUE(grouped_data2.arg2()->GetAsString(&actual_group_id));
+  EXPECT_EQ(expected_group_id.ToString(), actual_group_id);
+  ASSERT_TRUE(grouped_data2.arg3()->GetAsInteger(&actual_index));
+  EXPECT_EQ(expected_index, actual_index);
+}

chrome/test/BUILD.gn

@@ -3528,6 +3528,11 @@ test("unit_tests") {
     ]
   }
 
+  if (enable_webui_tab_strip) {
+    sources +=
+        [ "../browser/ui/webui/tab_strip/tab_strip_ui_handler_unittest.cc" ]
+  }
+
   configs += [ "//build/config:precompiled_headers" ]
 
   if (is_android && notouch_build) {

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

@@ -405,6 +405,17 @@ suite('TabList', () => {
         tabElements[anotherTabToGroup.index].parentElement);
   });
 
+  test('MoveTabGroup', () => {
+    const tabToGroup = tabs[1];
+    webUIListenerCallback(
+        'tab-group-state-changed', tabToGroup.id, tabToGroup.index, 'group0');
+    webUIListenerCallback('tab-group-moved', 'group0', 0);
+
+    const tabAtIndex0 = getUnpinnedTabs()[0];
+    assertEquals(tabAtIndex0.parentElement.tagName, 'TABSTRIP-TAB-GROUP');
+    assertEquals(tabAtIndex0.tab.id, tabToGroup.id);
+  });
+
   test('dragstart sets a drag image offset by the event coordinates', () => {
     // Drag and drop only works for pinned tabs
     tabs.forEach(pinTabAt);