WebUI Tab Strip: Update tab moves on drop, not drag update

This CLs updates the drag and drop interactions such that only
the DOM is updated as the user drags a tab or group element. The
tab strip model is only changed once the user commits the move by
dropping the element in place.

The DragManager now initializes a new DragSession every time a
drag begins, and uses the session to store and retrieve indices.

This will also make it easier and more consistent to drop a tab or
group from another window into a specific index. The idea is to create
a TabElement or TabGroupElement as a drag enters from another
window and keep most of the code the same.

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

chrome/browser/resources/tab_strip/tab_group.html

@@ -4,6 +4,11 @@
   --tabstrip-tab-group-title-margin: var(--tabstrip-tab-spacing);
 }
 
+:host(:empty) {
+  /* A tab group can temporarily become empty as a tab is being dragged out. */
+  display: none;
+}
+
 #tabGroup {
   border-radius: 8px;
   box-shadow: 0 0 0 1px rgba(var(--tabstrip-tab-group-color-rgb), .24);

chrome/browser/resources/tab_strip/tab_list.js

@@ -163,17 +163,7 @@ class TabListElement extends CustomElement {
     });
 
     const dragManager = new DragManager(this);
-    this.addEventListener(
-        'dragstart', e => dragManager.startDrag(/** @type {!DragEvent} */ (e)));
-    this.addEventListener(
-        'dragend', e => dragManager.stopDrag(/** @type {!DragEvent} */ (e)));
-    this.addEventListener('dragleave', () => dragManager.cancelDrag());
-    this.addEventListener(
-        'dragover',
-        e => dragManager.continueDrag(
-            /** @type {!DragEvent} */ (e)));
-    this.addEventListener(
-        'drop', e => dragManager.drop(/** @type {!DragEvent} */ (e)));
+    dragManager.startObserving();
 
     if (loadTimeData.getBoolean('showDemoOptions')) {
       this.$('#demoOptions').style.display = 'block';

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

@@ -10,15 +10,24 @@ import {TabsApiProxy} from 'chrome://tab-strip/tabs_api_proxy.js';
 import {TestTabsApiProxy} from './test_tabs_api_proxy.js';
 
 class MockDelegate extends HTMLElement {
-  constructor() {
-    super();
-  }
-
   getIndexOfTab(tabElement) {
     return Array.from(this.querySelectorAll('tabstrip-tab'))
         .indexOf(tabElement);
   }
 
+  placeTabElement(element, index, pinned, groupId) {
+    element.remove();
+
+    const parent =
+        groupId ? this.querySelector(`[data-group-id=${groupId}]`) : this;
+    parent.insertBefore(element, this.children[index]);
+  }
+
+  placeTabGroupElement(element, index) {
+    element.remove();
+    this.insertBefore(element, this.children[index]);
+  }
+
   showDropPlaceholder(element) {
     this.appendChild(element);
   }
@@ -117,11 +126,7 @@ suite('DragManager', () => {
       delegate.appendChild(tabElement);
     });
     dragManager = new DragManager(delegate);
-    delegate.addEventListener('dragstart', e => dragManager.startDrag(e));
-    delegate.addEventListener('dragend', e => dragManager.stopDrag(e));
-    delegate.addEventListener('dragleave', () => dragManager.cancelDrag());
-    delegate.addEventListener('dragover', (e) => dragManager.continueDrag(e));
-    delegate.addEventListener('drop', (e) => dragManager.drop(e));
+    dragManager.startObserving();
   });
 
   test('DragStartSetsDragImage', () => {
@@ -151,7 +156,7 @@ suite('DragManager', () => {
     const dragOverTab = delegate.children[dragOverIndex];
     const mockDataTransfer = new MockDataTransfer();
 
-    // Dispatch a dragstart event to start the drag process
+    // Dispatch a dragstart event to start the drag process.
     const dragStartEvent = new DragEvent('dragstart', {
       bubbles: true,
       composed: true,
@@ -161,7 +166,7 @@ suite('DragManager', () => {
     });
     draggedTab.dispatchEvent(dragStartEvent);
 
-    // Move the draggedTab over the 2nd tab
+    // Move the draggedTab over the 2nd tab.
     const dragOverEvent = new DragEvent('dragover', {
       bubbles: true,
       composed: true,
@@ -169,6 +174,12 @@ suite('DragManager', () => {
     });
     dragOverTab.dispatchEvent(dragOverEvent);
     assertEquals(dragOverEvent.dataTransfer.dropEffect, 'move');
+
+    // Dragover tab and dragged tab have now switched places in the DOM.
+    assertEquals(draggedTab, delegate.children[dragOverIndex]);
+    assertEquals(dragOverTab, delegate.children[draggedIndex]);
+
+    draggedTab.dispatchEvent(new DragEvent('drop', {bubbles: true}));
     const [tabId, newIndex] = await testTabsApiProxy.whenCalled('moveTab');
     assertEquals(tabId, tabs[draggedIndex].id);
     assertEquals(newIndex, dragOverIndex);
@@ -199,6 +210,11 @@ suite('DragManager', () => {
       dataTransfer: mockDataTransfer,
     });
     dragOverTabGroup.dispatchEvent(dragOverEvent);
+
+    // Tab is now in the group within the DOM.
+    assertEquals(dragOverTabGroup, draggedTab.parentElement);
+
+    draggedTab.dispatchEvent(new DragEvent('drop', {bubbles: true}));
     const [tabId, groupId] = await testTabsApiProxy.whenCalled('groupTab');
     assertEquals(draggedTab.tab.id, tabId);
     assertEquals('group0', groupId);
@@ -227,6 +243,11 @@ suite('DragManager', () => {
       dataTransfer: mockDataTransfer,
     });
     delegate.dispatchEvent(dragOverEvent);
+
+    // The tab is now outside of the group in the DOM.
+    assertEquals(delegate, draggedTab.parentElement);
+
+    draggedTab.dispatchEvent(new DragEvent('drop', {bubbles: true}));
     const [tabId] = await testTabsApiProxy.whenCalled('ungroupTab');
     assertEquals(draggedTab.tab.id, tabId);
   });
@@ -235,7 +256,8 @@ suite('DragManager', () => {
     const tabElements = delegate.children;
 
     // Start dragging the group.
-    const draggedGroup = groupTab(tabElements[0], 'group0');
+    const draggedGroupIndex = 0;
+    const draggedGroup = groupTab(tabElements[draggedGroupIndex], 'group0');
     const mockDataTransfer = new MockDataTransfer();
     const dragStartEvent = new DragEvent('dragstart', {
       bubbles: true,
@@ -247,13 +269,20 @@ suite('DragManager', () => {
     draggedGroup.dispatchEvent(dragStartEvent);
 
     // Drag the group over the second tab.
-    const dragOverTab = tabElements[1];
+    const dragOverIndex = 1;
+    const dragOverTab = tabElements[dragOverIndex];
     const dragOverEvent = new DragEvent('dragover', {
       bubbles: true,
       composed: true,
       dataTransfer: mockDataTransfer,
     });
     dragOverTab.dispatchEvent(dragOverEvent);
+
+    // Group and tab have now switched places.
+    assertEquals(draggedGroup, delegate.children[dragOverIndex]);
+    assertEquals(dragOverTab, delegate.children[draggedGroupIndex]);
+
+    draggedGroup.dispatchEvent(new DragEvent('drop', {bubbles: true}));
     const [groupId, index] = await testTabsApiProxy.whenCalled('moveGroup');
     assertEquals('group0', groupId);
     assertEquals(1, index);
@@ -263,8 +292,10 @@ suite('DragManager', () => {
     const tabElements = delegate.children;
 
     // Group the first tab and second tab separately.
-    const draggedGroup = groupTab(tabElements[0], 'group0');
-    const dragOverGroup = groupTab(tabElements[1], 'group1');
+    const draggedIndex = 0;
+    const draggedGroup = groupTab(tabElements[draggedIndex], 'group0');
+    const dragOverIndex = 1;
+    const dragOverGroup = groupTab(tabElements[dragOverIndex], 'group1');
 
     // Start dragging the first group.
     const mockDataTransfer = new MockDataTransfer();
@@ -284,6 +315,12 @@ suite('DragManager', () => {
       dataTransfer: mockDataTransfer,
     });
     dragOverGroup.dispatchEvent(dragOverEvent);
+
+    // Groups have now switched places.
+    assertEquals(draggedGroup, delegate.children[dragOverIndex]);
+    assertEquals(dragOverGroup, delegate.children[draggedIndex]);
+
+    draggedGroup.dispatchEvent(new DragEvent('drop', {bubbles: true}));
     const [groupId, index] = await testTabsApiProxy.whenCalled('moveGroup');
     assertEquals('group0', groupId);
     assertEquals(1, index);
@@ -321,4 +358,31 @@ suite('DragManager', () => {
     assertEquals(droppedTabId, tabId);
     assertEquals(-1, index);
   });
+
+  test('CancelDragResetsPosition', () => {
+    const draggedIndex = 0;
+    const draggedTab = delegate.children[draggedIndex];
+    const mockDataTransfer = new MockDataTransfer();
+
+    // Dispatch a dragstart event to start the drag process.
+    const dragStartEvent = new DragEvent('dragstart', {
+      bubbles: true,
+      composed: true,
+      clientX: 100,
+      clientY: 150,
+      dataTransfer: mockDataTransfer,
+    });
+    draggedTab.dispatchEvent(dragStartEvent);
+
+    // Move the draggedTab over the 2nd tab.
+    const dragOverEvent = new DragEvent('dragover', {
+      bubbles: true,
+      composed: true,
+      dataTransfer: mockDataTransfer,
+    });
+    delegate.children[1].dispatchEvent(dragOverEvent);
+
+    draggedTab.dispatchEvent(new DragEvent('dragend', {bubbles: true}));
+    assertEquals(draggedTab, delegate.children[draggedIndex]);
+  });
 });