Tab Strip WebUI: Implement 'Most Recently Used' ordering

- When the tab strip is closed, the newly activated tab should be
moved to the beginning of the list.
- When the tab strip is open, the newly activated tab should just
be scrolled to.
- Whenever the tab strip switches from visible to hidden, the active
tab should be moved to the beginning. The idea is what when a tab is
activated, the tab strip will close and the active tab will be then
be moved to avoid jumping while the tab strip is animating in and out.
- Regardless of whether or not the tab strip is visible, newly created
active tabs should be created at the beginning of the list to enforce
the MRU model.
- Disable drag and drop for unpinned tabs.

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

chrome/browser/resources/tab_strip/BUILD.gn

@@ -11,8 +11,8 @@ js_type_check("closure_compile") {
     ":custom_element",
     ":tab",
     ":tab_list",
+    ":tab_strip_view_proxy",
     ":tabs_api_proxy",
-    ":theme_proxy",
   ]
 }
 
@@ -43,7 +43,7 @@ js_library("tab_list") {
   externs_list = [ "$externs_path/chrome.js" ]
 }
 
-js_library("theme_proxy") {
+js_library("tab_strip_view_proxy") {
   deps = [
     "//ui/webui/resources/js:cr.m",
   ]

chrome/browser/resources/tab_strip/tab.js

@@ -59,10 +59,6 @@ export class TabElement extends CustomElement {
     this.closeButtonEl_.addEventListener('click', this.onClose_.bind(this));
   }
 
-  connectedCallback() {
-    this.setAttribute('draggable', 'true');
-  }
-
   /** @return {!Tab} */
   get tab() {
     return this.tab_;
@@ -75,6 +71,7 @@ export class TabElement extends CustomElement {
     // the tab is navigating to a new document.
     this.toggleAttribute('loading', tab.status === STATUS.LOADING);
     this.toggleAttribute('pinned', tab.pinned);
+    this.setAttribute('draggable', tab.pinned);
 
     if (!this.tab_ || this.tab_.title !== tab.title) {
       this.titleTextEl_.textContent = tab.title;

chrome/browser/resources/tab_strip/tab_list.js

@@ -4,11 +4,13 @@
 
 import './tab.js';
 
+import {assert} from 'chrome://resources/js/assert.m.js';
 import {addWebUIListener} from 'chrome://resources/js/cr.m.js';
+
 import {CustomElement} from './custom_element.js';
 import {TabElement} from './tab.js';
+import {TabStripViewProxy} from './tab_strip_view_proxy.js';
 import {TabsApiProxy} from './tabs_api_proxy.js';
-import {ThemeProxy} from './theme_proxy.js';
 
 /**
  * The amount of padding to leave between the edge of the screen and the active
@@ -68,6 +70,9 @@ class TabListElement extends CustomElement {
     /** @private {!Element} */
     this.scrollingParent_ = document.documentElement;
 
+    /** @private {!TabStripViewProxy} */
+    this.tabStripViewProxy_ = TabStripViewProxy.getInstance();
+
     /** @private {!TabsApiProxy} */
     this.tabsApi_ = TabsApiProxy.getInstance();
 
@@ -79,14 +84,11 @@ class TabListElement extends CustomElement {
         /** @type {!Element} */ (
             this.shadowRoot.querySelector('#tabsContainer'));
 
-    /** @private {!ThemeProxy} */
-    this.themeProxy_ = ThemeProxy.getInstance();
-
     /** @private {number} */
     this.windowId_;
 
     addWebUIListener('theme-changed', () => this.fetchAndUpdateColors_());
-    this.themeProxy_.startObserving();
+    this.tabStripViewProxy_.observeThemeChanges();
 
     addWebUIListener(
         'tab-thumbnail-updated', this.tabThumbnailUpdated_.bind(this));
@@ -97,6 +99,8 @@ class TabListElement extends CustomElement {
         'dragend', (e) => this.onDragEnd_(/** @type {!DragEvent} */ (e)));
     this.addEventListener(
         'dragover', (e) => this.onDragOver_(/** @type {!DragEvent} */ (e)));
+    document.addEventListener(
+        'visibilitychange', () => this.moveOrScrollToActiveTab_());
   }
 
   /**
@@ -124,10 +128,7 @@ class TabListElement extends CustomElement {
           }
         }
 
-        const activeTab = this.getActiveTab_();
-        if (activeTab) {
-          this.scrollToTab_(activeTab);
-        }
+        this.moveOrScrollToActiveTab_();
       }
 
       this.tabsApiHandler_.onAttached.addListener(
@@ -171,7 +172,7 @@ class TabListElement extends CustomElement {
 
   /** @private */
   fetchAndUpdateColors_() {
-    this.themeProxy_.getColors().then(colors => {
+    this.tabStripViewProxy_.getColors().then(colors => {
       for (const [cssVariable, rgbaValue] of Object.entries(colors)) {
         this.style.setProperty(cssVariable, rgbaValue);
       }
@@ -209,6 +210,22 @@ class TabListElement extends CustomElement {
     }
   }
 
+  /** @private */
+  moveOrScrollToActiveTab_() {
+    const activeTab = this.getActiveTab_();
+    if (!activeTab) {
+      return;
+    }
+
+    if (!this.tabStripViewProxy_.isVisible() && !activeTab.tab.pinned &&
+        this.tabsContainerElement_.firstChild !== activeTab) {
+      this.tabsApi_.moveTab(
+          activeTab.tab.id, this.pinnedTabsContainerElement_.childElementCount);
+    } else {
+      this.scrollToTab_(activeTab);
+    }
+  }
+
   /**
    * @param {!DragEvent} event
    * @private
@@ -232,8 +249,7 @@ class TabListElement extends CustomElement {
       return pathItem !== this.draggedItem_ && isTabElement(pathItem);
     });
 
-    if (!dragOverItem || !this.draggedItem_ ||
-        dragOverItem.tab.pinned !== this.draggedItem_.tab.pinned) {
+    if (!dragOverItem || !this.draggedItem_ || !dragOverItem.tab.pinned) {
       return;
     }
 
@@ -254,6 +270,7 @@ class TabListElement extends CustomElement {
       return;
     }
 
+    assert(draggedItem.tab.pinned);
     this.draggedItem_ = /** @type {!TabElement} */ (draggedItem);
     this.draggedItem_.setDragging(true);
     event.dataTransfer.effectAllowed = 'move';
@@ -282,7 +299,7 @@ class TabListElement extends CustomElement {
     if (newlyActiveTab) {
       newlyActiveTab.tab = /** @type {!Tab} */ (
           Object.assign({}, newlyActiveTab.tab, {active: true}));
-      this.scrollToTab_(newlyActiveTab);
+      this.moveOrScrollToActiveTab_();
     }
   }
 
@@ -314,8 +331,22 @@ class TabListElement extends CustomElement {
     }
 
     const tabElement = this.createTabElement_(tab);
-    this.insertTabOrMoveTo_(tabElement, tab.index);
-    this.addAnimationPromise_(tabElement.slideIn());
+
+    if (tab.active && !tab.pinned &&
+        tab.index !== this.pinnedTabsContainerElement_.childElementCount) {
+      // Newly created active tabs should first be moved to the very beginning
+      // of the tab strip to enforce the tab strip's most recently used ordering
+      this.tabsApi_
+          .moveTab(tab.id, this.pinnedTabsContainerElement_.childElementCount)
+          .then(() => {
+            this.insertTabOrMoveTo_(
+                tabElement, this.pinnedTabsContainerElement_.childElementCount);
+            this.addAnimationPromise_(tabElement.slideIn());
+          });
+    } else {
+      this.insertTabOrMoveTo_(tabElement, tab.index);
+      this.addAnimationPromise_(tabElement.slideIn());
+    }
   }
 
   /**

chrome/browser/resources/tab_strip/tab_strip_resources.grd

@@ -46,8 +46,8 @@
           type="chrome_html"
           compress="gzip"/>
       <structure
-          name="IDR_TAB_STRIP_THEME_PROXY_JS"
-          file="theme_proxy.js"
+          name="IDR_TAB_STRIP_VIEW_PROXY_JS"
+          file="tab_strip_view_proxy.js"
           type="chrome_html"
           compress="gzip"/>
     </structures>

chrome/browser/resources/tab_strip/theme_proxy.js → chrome/browser/resources/tab_strip/tab_strip_view_proxy.js

@@ -4,7 +4,12 @@
 
 import {addSingletonGetter, addWebUIListener, sendWithPromise} from 'chrome://resources/js/cr.m.js';
 
-export class ThemeProxy {
+export class TabStripViewProxy {
+  /** @return {boolean} */
+  isVisible() {
+    return document.visibilityState === 'visible';
+  }
+
   /**
    * @return {!Promise<!Object<string, string>>} Object with CSS variables
    *     as keys and rgba strings as values
@@ -13,9 +18,9 @@ export class ThemeProxy {
     return sendWithPromise('getThemeColors');
   }
 
-  startObserving() {
+  observeThemeChanges() {
     chrome.send('observeThemeChanges');
   }
 }
 
-addSingletonGetter(ThemeProxy);
+addSingletonGetter(TabStripViewProxy);

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

@@ -5,11 +5,11 @@
 import 'chrome://tab-strip/tab_list.js';
 
 import {webUIListenerCallback} from 'chrome://resources/js/cr.m.js';
+import {TabStripViewProxy} from 'chrome://tab-strip/tab_strip_view_proxy.js';
 import {TabsApiProxy} from 'chrome://tab-strip/tabs_api_proxy.js';
-import {ThemeProxy} from 'chrome://tab-strip/theme_proxy.js';
 
+import {TestTabStripViewProxy} from './test_tab_strip_view_proxy.js';
 import {TestTabsApiProxy} from './test_tabs_api_proxy.js';
-import {TestThemeProxy} from './test_theme_proxy.js';
 
 class MockDataTransfer extends DataTransfer {
   constructor() {
@@ -52,8 +52,8 @@ suite('TabList', () => {
   let callbackRouter;
   let optionsCalled;
   let tabList;
+  let testTabStripViewProxy;
   let testTabsApiProxy;
-  let testThemeProxy;
 
   const currentWindow = {
     id: 1001,
@@ -103,15 +103,14 @@ suite('TabList', () => {
     testTabsApiProxy = new TestTabsApiProxy();
     testTabsApiProxy.setCurrentWindow(currentWindow);
     TabsApiProxy.instance_ = testTabsApiProxy;
+    callbackRouter = testTabsApiProxy.callbackRouter;
 
-    testThemeProxy = new TestThemeProxy();
-    testThemeProxy.setColors({
+    testTabStripViewProxy = new TestTabStripViewProxy();
+    testTabStripViewProxy.setColors({
       '--background-color': 'white',
       '--foreground-color': 'black',
     });
-    ThemeProxy.instance_ = testThemeProxy;
-
-    callbackRouter = testTabsApiProxy.callbackRouter;
+    TabStripViewProxy.instance_ = testTabStripViewProxy;
 
     tabList = document.createElement('tabstrip-tab-list');
     document.body.appendChild(tabList);
@@ -119,20 +118,24 @@ suite('TabList', () => {
     return testTabsApiProxy.whenCalled('getCurrentWindow');
   });
 
+  teardown(() => {
+    testTabsApiProxy.reset();
+    testTabStripViewProxy.reset();
+  });
+
   test('sets theme colors on init', async () => {
-    await testThemeProxy.whenCalled('getColors');
+    await testTabStripViewProxy.whenCalled('getColors');
     assertEquals(tabList.style.getPropertyValue('--background-color'), 'white');
     assertEquals(tabList.style.getPropertyValue('--foreground-color'), 'black');
   });
 
   test('updates theme colors when theme changes', async () => {
-    testThemeProxy.resetResolver('getColors');
-    testThemeProxy.setColors({
+    testTabStripViewProxy.setColors({
       '--background-color': 'pink',
       '--foreground-color': 'blue',
     });
     webUIListenerCallback('theme-changed');
-    await testThemeProxy.whenCalled('getColors');
+    await testTabStripViewProxy.whenCalled('getColors');
     assertEquals(tabList.style.getPropertyValue('--background-color'), 'pink');
     assertEquals(tabList.style.getPropertyValue('--foreground-color'), 'blue');
   });
@@ -169,6 +172,20 @@ suite('TabList', () => {
     assertEquals(tabElements[0].tab, prependedTab);
   });
 
+  test('adds a new tab element to the start when it is active', async () => {
+    const newActiveTab = {
+      active: true,
+      id: 3,
+      index: 3,
+      title: 'New tab',
+      windowId: currentWindow.id,
+    };
+    callbackRouter.onCreated.dispatchEvent(newActiveTab);
+    const [tabId, newIndex] = await testTabsApiProxy.whenCalled('moveTab');
+    assertEquals(tabId, newActiveTab.id);
+    assertEquals(newIndex, 0);
+  });
+
   test(
       'does not add a new tab element when a tab is added in a different ' +
           'window',
@@ -264,6 +281,8 @@ suite('TabList', () => {
   });
 
   test('activating a tab off-screen scrolls to it', async () => {
+    testTabStripViewProxy.setVisible(true);
+
     const scrollPadding = 32;
 
     // Mock the width of each tab element
@@ -365,7 +384,10 @@ suite('TabList', () => {
       });
 
   test('dragstart sets a drag image offset by the event coordinates', () => {
-    const draggedTab = getUnpinnedTabs()[0];
+    // Drag and drop only works for pinned tabs
+    currentWindow.tabs.forEach(pinTabAt);
+
+    const draggedTab = getPinnedTabs()[0];
     const mockDataTransfer = new MockDataTransfer();
     const dragStartEvent = new DragEvent('dragstart', {
       bubbles: true,
@@ -385,10 +407,13 @@ suite('TabList', () => {
   });
 
   test('dragover moves tabs', async () => {
+    // Drag and drop only works for pinned tabs
+    currentWindow.tabs.forEach(pinTabAt);
+
     const draggedIndex = 0;
     const dragOverIndex = 1;
-    const draggedTab = getUnpinnedTabs()[draggedIndex];
-    const dragOverTab = getUnpinnedTabs()[dragOverIndex];
+    const draggedTab = getPinnedTabs()[draggedIndex];
+    const dragOverTab = getPinnedTabs()[dragOverIndex];
     const mockDataTransfer = new MockDataTransfer();
 
     // Dispatch a dragstart event to start the drag process
@@ -413,4 +438,23 @@ suite('TabList', () => {
     assertEquals(tabId, currentWindow.tabs[draggedIndex].id);
     assertEquals(newIndex, dragOverIndex);
   });
+
+  test(
+      'when the tab strip closes, the active tab should move to the start',
+      async () => {
+        // Mock activating the 2nd tab
+        callbackRouter.onActivated.dispatchEvent({
+          tabId: currentWindow.tabs[1].id,
+          windowId: currentWindow.id,
+        });
+        testTabsApiProxy.resetResolver('moveTab');
+
+        // Mock tab strip going from visible to hidden
+        testTabStripViewProxy.setVisible(false);
+        document.dispatchEvent(new Event('visibilitychange'));
+
+        const [moveId, newIndex] = await testTabsApiProxy.whenCalled('moveTab');
+        assertEquals(moveId, currentWindow.tabs[1].id);
+        assertEquals(newIndex, 0);
+      });
 });

chrome/test/data/webui/tab_strip/test_theme_proxy.js → chrome/test/data/webui/tab_strip/test_tab_strip_view_proxy.js

@@ -4,14 +4,15 @@
 
 import {TestBrowserProxy} from 'chrome://test/test_browser_proxy.m.js';
 
-export class TestThemeProxy extends TestBrowserProxy {
+export class TestTabStripViewProxy extends TestBrowserProxy {
   constructor() {
     super([
       'getColors',
-      'startObserving',
+      'isVisible',
+      'observeThemeChanges',
     ]);
 
-    this.colors_ = {};
+    this.visible_ = false;
   }
 
   getColors() {
@@ -19,11 +20,20 @@ export class TestThemeProxy extends TestBrowserProxy {
     return Promise.resolve(this.colors_);
   }
 
+  isVisible() {
+    this.methodCalled('isVisible');
+    return this.visible_;
+  }
+
   setColors(colors) {
     this.colors_ = colors;
   }
 
-  startObserving() {
-    this.methodCalled('startObserving');
+  setVisible(visible) {
+    this.visible_ = visible;
+  }
+
+  observeThemeChanges() {
+    this.methodCalled('observeThemeChanges');
   }
 }