Create new commands and keyboard shortcuts to change a tab's display …

…order up or down one position. Fix brave#11310 Keyboard shortcut is ctrl-shift-pageup and ctrl-shift-pagedown Auditors: @bsclifton @cezaraugusto
syuan100 · Nov 9, 2017 · b83b183 · b83b183
1 parent b3d20c3
commit b83b183
Show file tree

Hide file tree

Showing 9 changed files with 151 additions and 5 deletions.
diff --git a/app/localShortcuts.js b/app/localShortcuts.js
@@ -7,6 +7,7 @@ const BrowserWindow = electron.BrowserWindow
 const electronLocalshortcut = require('electron-localshortcut')
 const messages = require('../js/constants/messages')
 const appActions = require('../js/actions/appActions')
+const windowActions = require('../js/actions/windowActions')
 const isDarwin = process.platform === 'darwin'
 
 module.exports.register = (win) => {
@@ -73,6 +74,20 @@ module.exports.register = (win) => {
       isPartitioned: true
     })
   })
+
+  electronLocalshortcut.register(win, 'Ctrl+Shift+PageUp', () => {
+    const win = BrowserWindow.getFocusedWindow()
+    if (win) {
+      windowActions.tabMoveIncrementalRequested(win.id, false)
+    }
+  })
+
+  electronLocalshortcut.register(win, 'Ctrl+Shift+PageDown', () => {
+    const win = BrowserWindow.getFocusedWindow()
+    if (win) {
+      windowActions.tabMoveIncrementalRequested(win.id)
+    }
+  })
 }
 
 module.exports.unregister = (win) => {

diff --git a/app/renderer/components/tabs/pinnedTabs.js b/app/renderer/components/tabs/pinnedTabs.js
@@ -89,6 +89,7 @@ class PinnedTabs extends React.Component {
   render () {
     this.tabRefs = []
     return <div
+      data-test-pinnedTabs
       className='pinnedTabs'
       onDragOver={this.onDragOver}
       onDrop={this.onDrop}>

diff --git a/app/renderer/components/tabs/tabs.js b/app/renderer/components/tabs/tabs.js
@@ -159,7 +159,9 @@ class Tabs extends React.Component {
 
   render () {
     this.tabRefs = []
-    return <div className='tabs'
+    return <div
+      data-test-tabs
+      className='tabs'
       onMouseLeave={this.onMouseLeave}>
       <span className={cx({
         tabStripContainer: true,

diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js
@@ -313,6 +313,20 @@ const windowActions = {
     })
   },
 
+  /**
+   * A request to increase or decrease the display order of the active tab relative to other tabs in its window
+   * @param {Boolean} moveNext
+   */
+  tabMoveIncrementalRequested: function (windowId, moveNext = true) {
+    dispatch({
+      actionType: windowConstants.WINDOW_TAB_MOVE_INCREMENTAL_REQUESTED,
+      moveNext,
+      queryInfo: {
+        windowId
+      }
+    })
+  },
+
   /**
    * The active URL bar suggestion was clicked
    * @param {boolean} isForSecondaryAction - Whether the secondary action is expected

diff --git a/js/constants/windowConstants.js b/js/constants/windowConstants.js
@@ -16,6 +16,7 @@ const windowConstants = {
   WINDOW_SET_TAB_PAGE_HOVER_STATE: _,
   WINDOW_SET_TAB_CONTENT_INTERSECTION_STATE: _,
   WINDOW_TAB_MOVE: _,
+  WINDOW_TAB_MOVE_INCREMENTAL_REQUESTED: _,
   WINDOW_SET_THEME_COLOR: _,
   WINDOW_WEBVIEW_LOAD_END: _,
   WINDOW_SET_FULL_SCREEN: _,

diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js
@@ -773,6 +773,24 @@ const doAction = (action) => {
     case appConstants.APP_WINDOW_RESIZED:
       windowState = windowState.set('windowInfo', action.windowValue)
       break
+    case windowConstants.WINDOW_TAB_MOVE_INCREMENTAL_REQUESTED:
+      const sourceFrame = frameStateUtil.getActiveFrame(windowState)
+      const sourceIsPinned = sourceFrame.get('pinnedLocation')
+      const frameGroup = sourceIsPinned ? frameStateUtil.getPinnedFrames(windowState) : frameStateUtil.getNonPinnedFrames(windowState)
+      const sourceFrameGroupIndex = frameGroup.indexOf(sourceFrame)
+      const destinationFrameGroupIndex = sourceFrameGroupIndex + (action.moveNext ? 1 : -1)
+      // conditions we cannot move tab:
+      // - if we can't find it,
+      // - if we ask to move to previous and tab is first,
+      // - or we ask to move to next and tab is last
+      if (destinationFrameGroupIndex < 0 || destinationFrameGroupIndex >= frameGroup.count()) {
+        break
+      }
+      const destinationFrame = frameGroup.get(destinationFrameGroupIndex)
+      const destinationFrameIndex = frameStateUtil.getFrameIndex(windowState, destinationFrame.get('key'))
+      const sourceFrameTabId = sourceFrame.get('tabId')
+      appActions.tabIndexChangeRequested(sourceFrameTabId, destinationFrameIndex)
+      break
     default:
       break
   }

diff --git a/test/lib/brave.js b/test/lib/brave.js
@@ -675,6 +675,13 @@ var exports = {
       }, sourcePinKey, destinationPinKey, prepend)
     })
 
+    this.app.client.addCommand('moveTabIncrementally', function (moveNext, windowId = 1) {
+      logVerbose(`moveTabIncrementally(${moveNext}, ${windowId}`)
+      return this.execute(function (moveNext, windowId) {
+        return devTools('electron').testData.windowActions.tabMoveIncrementalRequested(windowId, moveNext)
+      }, moveNext, windowId)
+    })
+
     this.app.client.addCommand('ipcOn', function (message, fn) {
       logVerbose('ipcOn("' + message + '")')
       return this.execute(function (message, fn) {
@@ -719,6 +726,15 @@ var exports = {
       })
     })
 
+    this.app.client.addCommand('activateTabByFrameKey', function (key) {
+      return this.getAppState().then((val) => {
+        const tab = val.value.tabs.find((tab) => tab.frame.key === key)
+        return this.execute(function (tabId) {
+          devTools('appActions').tabActivateRequested(tabId)
+        }, tab.tabId)
+      })
+    })
+
     /**
      * Adds a bookmark
      *

diff --git a/test/lib/selectors.js b/test/lib/selectors.js
@@ -7,11 +7,13 @@ module.exports = {
   activeTab: '[data-test-active-tab]',
   activeTabTitle: '[data-test-active-tab] [data-test-id="tabTitle"]',
   activeTabFavicon: '[data-test-active-tab] [data-test-favicon]',
-  pinnedTabsTabs: '.pinnedTabs [data-test-id="tab"]',
-  tabsTabs: '.tabs [data-test-id="tab"]',
+  pinnedTabs: '[data-test-pinnedTabs]',
+  pinnedTabsTabs: '[data-test-pinnedTabs] [data-test-id="tab"]',
+  tabs: '[data-test-tabs]',
+  tabsTabs: '[data-test-tabs] [data-test-id="tab"]',
   navigator: '#navigator',
   navigatorLoadTime: '#navigator .loadTime',
-  newFrameButton: '.tabs .newFrameButton',
+  newFrameButton: '[data-test-tabs] .newFrameButton',
   tabPage: '.tabPage',
   tabPage1: '[data-tab-page="0"]',
   tabPage2: '[data-tab-page="1"]',

diff --git a/test/tab-components/tabTest.js b/test/tab-components/tabTest.js
@@ -4,7 +4,7 @@ const Brave = require('../lib/brave')
 const messages = require('../../js/constants/messages')
 const settings = require('../../js/constants/settings')
 const {tabPreviewTiming} = require('../../app/common/constants/settingsEnums')
-const {urlInput, backButton, forwardButton, activeTab, activeTabTitle, activeTabFavicon, newFrameButton, notificationBar, contextMenu, pinnedTabsTabs, tabsTabs} = require('../lib/selectors')
+const {urlInput, backButton, forwardButton, activeTab, activeTabTitle, activeTabFavicon, newFrameButton, notificationBar, contextMenu, pinnedTabsTabs, tabsTabs, pinnedTabs, tabs} = require('../lib/selectors')
 
 const newTabUrl = 'chrome-extension://mnojpmjdmbbfmejpflffifhffcmidifd/about-newtab.html'
 
@@ -207,6 +207,83 @@ describe('tab tests', function () {
     })
   })
 
+  describe('tab order index change', function () {
+    Brave.beforeAll(this)
+    before(function * () {
+      this.page1 = Brave.server.url('page1.html')
+      this.page2 = Brave.server.url('page2.html')
+      yield setup(this.app.client)
+      yield this.app.client
+        .waitForExist('.tabArea:nth-of-type(1) [data-frame-key="1"]') // original newtab
+        .newTab({ url: this.page1, pinned: true })
+        .waitForExist(pinnedTabs + ' [data-frame-key="2"]')
+        .newTab({ url: this.page2, pinned: true })
+        .waitForExist(pinnedTabs + ' [data-frame-key="3"]')
+        .newTab({ url: this.page1 })
+        .waitForExist(tabs + ' [data-frame-key="4"]')
+        .newTab({ url: this.page2 })
+        .waitForExist(tabs + ' [data-frame-key="5"]')
+        // check initial order
+        .waitForExist(pinnedTabs + ' [data-test-id="tab-area"][data-frame-key="2"] + [data-test-id="tab-area"][data-frame-key="3"]')
+        .waitForExist(tabs + ' [data-test-id="tab-area"][data-frame-key="1"] + [data-test-id="tab-area"][data-frame-key="4"] + [data-test-id="tab-area"][data-frame-key="5"]')
+    })
+
+    it('does not move first unpinned index backwards', function * () {
+      yield this.app.client
+      .activateTabByFrameKey(1) // first created tab
+      .waitForExist('[data-test-active-tab][data-frame-key="1"]')
+      // attempt to move the first tab backwards
+      .moveTabIncrementally(false)
+      // should not have changed anything
+      .waitForExist(tabs + ' [data-test-id="tab-area"][data-frame-key="1"] + [data-test-id="tab-area"][data-frame-key="4"] + [data-test-id="tab-area"][data-frame-key="5"]')
+      .waitForExist(pinnedTabs + ' [data-test-id="tab-area"][data-frame-key="2"] + [data-test-id="tab-area"][data-frame-key="3"]')
+    })
+
+    it('does not move last unpinned index forwards', function * () {
+      yield this.app.client
+      .activateTabByFrameKey(5)
+      .waitForExist('[data-test-active-tab][data-frame-key="4"]')
+      // attempt to move the last tab forwards
+      .moveTabIncrementally(true)
+      // should not have changed anything
+      .waitForExist(tabs + ' [data-test-id="tab-area"][data-frame-key="1"] + [data-test-id="tab-area"][data-frame-key="4"] + [data-test-id="tab-area"][data-frame-key="5"]')
+      .waitForExist(pinnedTabs + ' [data-test-id="tab-area"][data-frame-key="2"] + [data-test-id="tab-area"][data-frame-key="3"]')
+    })
+
+    it('moves from first unpinned position to next', function * () {
+      yield this.app.client
+        .activateTabByFrameKey(1)
+        .waitForExist('[data-test-active-tab][data-frame-key="1"]')
+        // move first tab forwards
+        .moveTabIncrementally(true)
+        // check tabs are changed order and pinned remain unchanged
+        .waitForExist(tabs + ' [data-test-id="tab-area"][data-frame-key="4"] + [data-test-id="tab-area"][data-frame-key="1"] + [data-test-id="tab-area"][data-frame-key="5"]')
+        .waitForExist(pinnedTabs + ' [data-test-id="tab-area"][data-frame-key="2"] + [data-test-id="tab-area"][data-frame-key="3"]')
+    })
+
+    it('moves from last unpinned index backwards', function * () {
+      yield this.app.client
+      .activateTabByFrameKey(5)
+      .waitForExist('[data-test-active-tab][data-frame-key="5"]')
+      // move last tab backwards
+      .moveTabIncrementally(false)
+      // check tabs are changed order and pinned remain unchanged
+      .waitForExist(tabs + ' [data-test-id="tab-area"][data-frame-key="4"] + [data-test-id="tab-area"][data-frame-key="5"] + [data-test-id="tab-area"][data-frame-key="1"]')
+      .waitForExist(pinnedTabs + ' [data-test-id="tab-area"][data-frame-key="2"] + [data-test-id="tab-area"][data-frame-key="3"]')
+    })
+
+    it('moves from first pinned position to next', function * () {
+      yield this.app.client
+        .activateTabByFrameKey(2)
+        .waitForExist('[data-test-active-tab][data-frame-key="2"]')
+        // move first pinned tab forwards
+        .moveTabIncrementally(true)
+        // check pinned tabs are changed order and unpinned remain unchanged
+        .waitForExist(tabs + ' [data-test-id="tab-area"][data-frame-key="4"] + [data-test-id="tab-area"][data-frame-key="5"] + [data-test-id="tab-area"][data-frame-key="1"]')
+        .waitForExist(pinnedTabs + ' [data-test-id="tab-area"][data-frame-key="3"] + [data-test-id="tab-area"][data-frame-key="2"]')
+    })
+  })
+
   describe('new private tab signal', function () {
     Brave.beforeAll(this)
     before(function * () {