From c008832a07a734f08cd479aff8f11770b26aa156 Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Sat, 22 Jul 2017 22:55:18 -0400 Subject: [PATCH] Use muon's tab_strip_model for driving index and active Fix #10436 Fix #9385 Fix #9722 Fix #10561 Fix #9083 Fix #9671 Fix #10038 Fix #10384 Fix #10532 and probably several others. --- app/browser/reducers/tabsReducer.js | 107 +++++------ app/browser/reducers/windowsReducer.js | 9 +- app/browser/tabs.js | 179 ++++++++++++++++-- app/browser/windows.js | 15 +- app/common/state/tabState.js | 31 +-- app/renderer/components/frame/frame.js | 8 +- app/renderer/components/tabs/tab.js | 12 +- app/renderer/lib/tabUtil.js | 6 +- app/renderer/reducers/frameReducer.js | 29 ++- app/sessionStore.js | 29 ++- js/actions/appActions.js | 48 ++++- js/actions/windowActions.js | 7 - js/constants/appConstants.js | 4 + js/contextMenus.js | 2 +- js/state/frameStateUtil.js | 32 +++- js/stores/windowStore.js | 46 +---- test/app/sessionStoreTest.js | 4 +- test/lib/brave.js | 2 +- test/tab-components/pinnedTabTest.js | 12 +- test/tab-components/tabTest.js | 7 +- .../app/browser/reducers/tabsReducerTest.js | 49 +++-- test/unit/app/browser/tabsTest.js | 5 +- test/unit/js/stores/windowStoreTest.js | 25 ++- 23 files changed, 423 insertions(+), 245 deletions(-) diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index 78665f7e378..ba54fbafbc6 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -11,7 +11,6 @@ const windows = require('../windows') const {getWebContents} = require('../webContentsCache') const {BrowserWindow} = require('electron') const tabState = require('../../common/state/tabState') -const windowState = require('../../common/state/windowState') const tabActions = require('../../common/actions/tabActions') const siteSettings = require('../../../js/state/siteSettings') const siteSettingsState = require('../../common/state/siteSettingsState') @@ -22,63 +21,9 @@ const {getFlashResourceId} = require('../../../js/flash') const {l10nErrorText} = require('../../common/lib/httpUtil') const Immutable = require('immutable') const dragTypes = require('../../../js/constants/dragTypes') -const getSetting = require('../../../js/settings').getSetting -const settings = require('../../../js/constants/settings') -const {tabCloseAction} = require('../../common/constants/settingsEnums') const {frameOptsFromFrame} = require('../../../js/state/frameStateUtil') const {isSourceAboutUrl, isTargetAboutUrl, isNavigatableAboutPage} = require('../../../js/lib/appUrlUtil') -const updateActiveTab = (state, closeTabId) => { - if (!tabState.getByTabId(state, closeTabId)) { - return - } - - const index = tabState.getIndex(state, closeTabId) - if (index === -1) { - return - } - - const windowId = tabState.getWindowId(state, closeTabId) - if (windowId === windowState.WINDOW_ID_NONE) { - return - } - - const lastActiveTabId = tabState.getTabsByLastActivated(state, windowId).last() - if (lastActiveTabId !== closeTabId && !tabState.isActive(state, closeTabId)) { - return - } - - let nextTabId = tabState.TAB_ID_NONE - switch (getSetting(settings.TAB_CLOSE_ACTION)) { - case tabCloseAction.LAST_ACTIVE: - nextTabId = tabState.getLastActiveTabId(state, windowId) - break - case tabCloseAction.PARENT: - { - const openerTabId = tabState.getOpenerTabId(state, closeTabId) - if (openerTabId !== tabState.TAB_ID_NONE) { - nextTabId = openerTabId - } - break - } - } - - // DEFAULT: always fall back to NEXT - if (nextTabId === tabState.TAB_ID_NONE) { - nextTabId = tabState.getNextTabIdByIndex(state, windowId, index) - if (nextTabId === tabState.TAB_ID_NONE) { - // no unpinned tabs so find the next pinned tab - nextTabId = tabState.getNextTabIdByIndex(state, windowId, index, true) - } - } - - if (nextTabId !== tabState.TAB_ID_NONE) { - setImmediate(() => { - tabs.setActive(nextTabId) - }) - } -} - const WEBRTC_DEFAULT = 'default' const WEBRTC_DISABLE_NON_PROXY = 'disable_non_proxied_udp' @@ -126,7 +71,23 @@ const tabsReducer = (state, action, immutableAction) => { case appConstants.APP_TAB_CREATED: state = tabState.maybeCreateTab(state, action) break - case appConstants.APP_TAB_MOVED: { + case appConstants.APP_TAB_ATTACHED: + state = tabs.updateTabsStateForAttachedTab(state, action.get('tabId')) + break + case appConstants.APP_TAB_WILL_ATTACH: { + const tabId = action.get('tabId') + const tabValue = tabState.getByTabId(state, tabId) + if (!tabValue) { + break + } + const oldWindowId = tabState.getWindowId(state, tabId) + state = tabs.updateTabsStateForWindow(state, oldWindowId) + break + } + case appConstants.APP_TAB_MOVED: + state = tabs.updateTabsStateForAttachedTab(state, action.get('tabId')) + break + case appConstants.APP_TAB_DETACH_MENU_ITEM_CLICKED: { setImmediate(() => { const tabId = action.get('tabId') const frameOpts = frameOptsFromFrame(action.get('frameOpts')) @@ -158,6 +119,7 @@ const tabsReducer = (state, action, immutableAction) => { break case appConstants.APP_TAB_UPDATED: state = tabState.maybeCreateTab(state, action) + // tabs.debugTabs(state) break case appConstants.APP_TAB_CLOSE_REQUESTED: { @@ -194,7 +156,7 @@ const tabsReducer = (state, action, immutableAction) => { tabs.closeTab(tabId, action.get('forceClosePinned')) }) } else { - state = windows.closeWindow(state, windowId) + windows.closeWindow(windowId) } } } @@ -206,8 +168,22 @@ const tabsReducer = (state, action, immutableAction) => { if (tabId === tabState.TAB_ID_NONE) { break } - updateActiveTab(state, tabId) + const nextActiveTabId = tabs.getNextActiveTab(state, tabId) + + // Must be called before tab is removed + // But still check for no tabId because on tab detach there's a dummy tabId + const tabValue = tabState.getByTabId(state, tabId) + if (tabValue) { + const windowIdOfTabBeingRemoved = tabState.getWindowId(state, tabId) + state = tabs.updateTabsStateForWindow(state, windowIdOfTabBeingRemoved) + } + console.log('calling removeTabByTabId ofr tabId:', tabId) state = tabState.removeTabByTabId(state, tabId) + setImmediate(() => { + if (nextActiveTabId !== tabState.TAB_ID_NONE) { + tabs.setActive(nextActiveTabId) + } + }) } break case appConstants.APP_ALLOW_FLASH_ONCE: @@ -225,9 +201,7 @@ const tabsReducer = (state, action, immutableAction) => { }) break case appConstants.APP_TAB_PINNED: - setImmediate(() => { - tabs.pin(state, action.get('tabId'), action.get('pinned')) - }) + state = tabs.pin(state, action.get('tabId'), action.get('pinned')) break case windowConstants.WINDOW_SET_AUDIO_MUTED: setImmediate(() => { @@ -372,10 +346,15 @@ const tabsReducer = (state, action, immutableAction) => { const dragData = state.get('dragData') if (dragData && dragData.get('type') === dragTypes.TAB) { const frame = dragData.get('data') - const frameOpts = frameOptsFromFrame(frame).toJS() + let frameOpts = frameOptsFromFrame(frame) const browserOpts = { positionByMouseCursor: true, checkMaximized: true } - frameOpts.indexByFrameKey = dragData.getIn(['dragOverData', 'draggingOverKey']) - frameOpts.prependIndexByFrameKey = dragData.getIn(['dragOverData', 'draggingOverLeftHalf']) + const tabIdForIndex = dragData.getIn(['dragOverData', 'draggingOverKey']) + const tabForIndex = tabState.getByTabId(state, tabIdForIndex) + const dropWindowId = dragData.get('dropWindowId') + if (dropWindowId != null && dropWindowId !== -1 && tabForIndex) { + const prependIndexByTabId = dragData.getIn(['dragOverData', 'draggingOverLeftHalf']) + frameOpts = frameOpts.set('index', tabForIndex.get('index') + (prependIndexByTabId ? 0 : 1)) + } tabs.moveTo(state, frame.get('tabId'), frameOpts, browserOpts, dragData.get('dropWindowId')) } break diff --git a/app/browser/reducers/windowsReducer.js b/app/browser/reducers/windowsReducer.js index 3487c4e496f..1528a0b5b76 100644 --- a/app/browser/reducers/windowsReducer.js +++ b/app/browser/reducers/windowsReducer.js @@ -324,15 +324,20 @@ const windowsReducer = (state, action, immutableAction) => { } break case appConstants.APP_CLOSE_WINDOW: - state = windows.closeWindow(state, action.get('windowId')) + windows.closeWindow(action.get('windowId')) break case appConstants.APP_WINDOW_CLOSED: state = windowState.removeWindow(state, action) - sessionStoreShutdown.removeWindowFromCache(action.getIn(['windowValue', 'windowId'])) + const windowId = action.getIn(['windowValue', 'windowId']) + sessionStoreShutdown.removeWindowFromCache(windowId) + windows.cleanupWindow(windowId) break case appConstants.APP_WINDOW_CREATED: state = windowState.maybeCreateWindow(state, action) break + case appConstants.APP_TAB_STRIP_EMPTY: + windows.closeWindow(action.get('windowId')) + break case appConstants.APP_WINDOW_UPDATED: state = windowState.maybeCreateWindow(state, action) if (action.get('updateDefault')) { diff --git a/app/browser/tabs.js b/app/browser/tabs.js index 905b1dd1935..f32a021acdb 100644 --- a/app/browser/tabs.js +++ b/app/browser/tabs.js @@ -8,6 +8,7 @@ const tabActions = require('../common/actions/tabActions') const config = require('../../js/constants/config') const Immutable = require('immutable') const tabState = require('../common/state/tabState') +const windowState = require('../common/state/windowState') const {app, BrowserWindow, extensions, session, ipcMain} = require('electron') const {makeImmutable} = require('../common/state/immutableUtil') const {getTargetAboutUrl, getSourceAboutUrl, isSourceAboutUrl, newFrameUrl, isTargetAboutUrl, isIntermediateAboutPage, isTargetMagnetUrl, getSourceMagnetUrl} = require('../../js/lib/appUrlUtil') @@ -23,6 +24,7 @@ const aboutHistoryState = require('../common/state/aboutHistoryState') const appStore = require('../../js/stores/appStore') const appConfig = require('../../js/constants/appConfig') const {newTabMode} = require('../common/constants/settingsEnums') +const {tabCloseAction} = require('../common/constants/settingsEnums') const {cleanupWebContents, currentWebContents, getWebContents, updateWebContents} = require('./webContentsCache') const {FilterOptions} = require('ad-block') const {isResourceEnabled} = require('../filtering') @@ -428,8 +430,7 @@ const api = { if (tab.isBackgroundPage() || !tab.isGuest()) { return } - let tabId = tab.getId() - + const tabId = tab.getId() tab.on('did-start-navigation', (e, navigationHandle) => { if (!tab.isDestroyed() && navigationHandle.isValid() && navigationHandle.isInMainFrame()) { const controller = tab.controller() @@ -465,10 +466,40 @@ const api = { tab.on('unresponsive', () => { console.log('unresponsive') }) + tab.on('responsive', () => { console.log('responsive') }) + tab.on('tab-changed-at', () => { + updateTab(tabId) + }) + + tab.on('tab-moved', () => { + appActions.tabMoved(tabId) + }) + + tab.on('will-attach', () => { + appActions.tabWillAttach(tab.getId()) + }) + + tab.on('tab-strip-empty', () => { + // It's only safe to close a window when the last web-contents tab has been + // re-attached. A detach which already happens by this point is not enough. + // Otherwise the closing window will destroy the tab web-contents and it'll + // lead to a dead tab. The destroy will happen because the old main window + // webcontents is still the embedder. + const tabValue = getTabValue(tabId) + const windowId = tabValue.get('windowId') + tab.once('will-attach', () => { + appActions.tabStripEmpty(windowId) + }) + }) + + tab.on('did-attach', () => { + appActions.tabAttached(tab.getId()) + }) + tab.on('save-password', (e, username, origin) => { appActions.savePassword(username, origin, tabId) }) @@ -610,8 +641,11 @@ const api = { const tabId = action.get('tabId') let options = action.get('options') || Immutable.Map() const tabValue = getTabValue(tabId) - if (tabValue && tabValue.get('index') !== undefined) { - options = options.set('index', tabValue.get('index') + 1) + if (tabValue) { + const index = tabValue.get('index') + if (index !== undefined) { + options = options.set('index', index + 1) + } } const tab = getWebContents(tabId) if (tab && !tab.isDestroyed()) { @@ -628,11 +662,13 @@ const api = { const alreadyPinnedTab = tabState.queryTab(state, { url, pinned: true, partition: tab.session.partition }) if (pinned && alreadyPinnedTab) { api.closeTab(tabId) - return + return state } tab.setPinned(pinned) + state = api.updateTabsStateForAttachedTab(state, tabId) } + return state }, isDevToolsFocused: (tabId) => { @@ -722,7 +758,7 @@ const api = { win.loadURL('about:blank') }, - moveTo: (state, tabId, frameOpts, browserOpts, windowId) => { + moveTo: (state, tabId, frameOpts, browserOpts, toWindowId) => { frameOpts = makeImmutable(frameOpts) browserOpts = makeImmutable(browserOpts) const tab = getWebContents(tabId) @@ -734,11 +770,11 @@ const api = { } const currentWindowId = tabValue && tabValue.get('windowId') - if (windowId != null && currentWindowId === windowId) { + if (toWindowId != null && currentWindowId === toWindowId) { return } - if (windowId == null || windowId === -1) { + if (toWindowId == null || toWindowId === -1) { // If there's only one tab and we're dragging outside the window, then disallow // a new window to be created. const windowTabCount = tabState.getTabsByWindowId(state, currentWindowId).size @@ -751,13 +787,27 @@ const api = { // If the current tab is pinned, then don't allow to drag out return } - + const nextActiveTabIdForOldWindow = api.getNextActiveTab(state, tabId) tab.detach(() => { - if (windowId == null || windowId === -1) { - appActions.newWindow(makeImmutable(frameOpts), browserOpts) + if (toWindowId == null || toWindowId === -1) { + frameOpts = frameOpts.set('index', 0) + appActions.newWindow(frameOpts, browserOpts) } else { - appActions.newWebContentsAdded(windowId, frameOpts, tabValue) + appActions.newWebContentsAdded(toWindowId, frameOpts, tabValue) } + + // Setting the next active tab for the old window must happen after re-attach of the new tab. + // This is because muon's tab_strip index for the tab would not be consistent with browser-laptop's + // expectation and it would try to set an invalid index as active, possibly leaivng nothing active. + tab.once('did-attach', () => { + if (nextActiveTabIdForOldWindow !== tabState.TAB_ID_NONE) { + api.setActive(nextActiveTabIdForOldWindow) + } + const index = frameOpts.get('index') + if (index !== undefined) { + api.setTabIndex(tabId, frameOpts.get('index')) + } + }) }) } }, @@ -856,6 +906,111 @@ const api = { } return null + }, + + getNextActiveTab: (state, closeTabId) => { + if (!tabState.getByTabId(state, closeTabId)) { + return + } + + const index = tabState.getIndex(state, closeTabId) + if (index === -1) { + return + } + + const windowId = tabState.getWindowId(state, closeTabId) + if (windowId === windowState.WINDOW_ID_NONE) { + return + } + + const lastActiveTabId = tabState.getTabsByLastActivated(state, windowId).last() + if (lastActiveTabId !== closeTabId && !tabState.isActive(state, closeTabId)) { + return + } + + let nextTabId = tabState.TAB_ID_NONE + switch (getSetting(settings.TAB_CLOSE_ACTION)) { + case tabCloseAction.LAST_ACTIVE: + nextTabId = tabState.getLastActiveTabId(state, windowId) + break + case tabCloseAction.PARENT: + { + const openerTabId = tabState.getOpenerTabId(state, closeTabId) + if (openerTabId !== tabState.TAB_ID_NONE) { + nextTabId = openerTabId + } + break + } + } + + // DEFAULT: always fall back to NEXT + if (nextTabId === tabState.TAB_ID_NONE) { + nextTabId = tabState.getNextTabIdByIndex(state, windowId, index) + if (nextTabId === tabState.TAB_ID_NONE) { + // no unpinned tabs so find the next pinned tab + nextTabId = tabState.getNextTabIdByIndex(state, windowId, index, true) + } + } + + return nextTabId + }, + debugTabs: (state) => { + console.log(tabState.getTabs(state) + .toJS() + .map((tab) => { + return { + tabId: tab.tabId, + index: tab.index, + windowId: tab.windowId, + active: tab.active, + pinned: tab.pinned + } + }) + .sort((tab1, tab2) => { + if (tab1.windowId !== tab2.windowId) { + return tab1.windowId - tab2.windowId + } + if (tab1.index !== tab2.index) { + return tab1.index - tab2.index + } + return 0 + })) + }, + updateTabsStateForAttachedTab: (state, tabId) => { + const tabValue = getTabValue(tabId) + if (!tabValue) { + return state + } + return api.updateTabsStateForWindow(state, tabValue.get('windowId')) + }, + updateTabsStateForWindow: (state, windowId) => { + tabState.getTabsByWindowId(state, windowId).forEach((tabValue) => { + const tabId = tabValue.get('tabId') + + const oldTabValue = tabState.getByTabId(state, tabId) + const newTabValue = getTabValue(tabId) + + // For now the renderer needs to know about index and pinned changes + // communicate those out here. + if (newTabValue && oldTabValue) { + const changeInfo = {} + const rendererAwareProps = ['index', 'pinned', 'url', 'active'] + rendererAwareProps.forEach((prop) => { + const newPropVal = newTabValue.get(prop) + if (oldTabValue.get(prop) !== newPropVal) { + changeInfo[prop] = newPropVal + } + }) + if (Object.keys(changeInfo).length > 0) { + updateTab(tabId, changeInfo) + } + } + + if (newTabValue) { + state = tabState.updateTabValue(state, newTabValue, false) + } + }) + return state } } diff --git a/app/browser/windows.js b/app/browser/windows.js index 5feeb046460..63836dfdec8 100644 --- a/app/browser/windows.js +++ b/app/browser/windows.js @@ -21,11 +21,6 @@ const windowActions = require('../../js/actions/windowActions') // TODO(bridiver) - set window uuid let currentWindows = {} -const cleanupWindow = (windowId) => { - delete currentWindows[windowId] - appActions.windowClosed({ windowId }) -} - const getWindowState = (win) => { if (win.isFullScreen()) { return 'fullscreen' @@ -38,6 +33,10 @@ const getWindowState = (win) => { } } +const cleanupWindow = (windowId) => { + delete currentWindows[windowId] +} + const getWindowValue = (windowId) => { let win = BrowserWindow.fromId(windowId) if (win) { @@ -121,6 +120,7 @@ const api = { win.webContents.once('close', () => { LocalShortcuts.unregister(win) }) + win.once('close', () => { LocalShortcuts.unregister(win) }) @@ -185,7 +185,6 @@ const api = { windowActions.onWindowUpdate(windowId, windowValue) }) win.once('closed', () => { - cleanupWindow(windowId) }) win.on('blur', () => { appActions.windowBlurred(windowId) @@ -307,7 +306,7 @@ const api = { }) }, - closeWindow: (state, windowId) => { + closeWindow: (windowId) => { let win = api.getWindow(windowId) try { setImmediate(() => { @@ -318,7 +317,6 @@ const api = { } catch (e) { // ignore } - return windowState.removeWindowByWindowId(state, windowId) }, getWindow: (windowId) => { @@ -329,7 +327,6 @@ const api = { if (BrowserWindow.getFocusedWindow()) { return BrowserWindow.getFocusedWindow().id } - return windowState.WINDOW_ID_NONE }, diff --git a/app/common/state/tabState.js b/app/common/state/tabState.js index 28e77dd8ad7..ada6e13fc20 100644 --- a/app/common/state/tabState.js +++ b/app/common/state/tabState.js @@ -96,27 +96,11 @@ const updateLastActive = (state, oldTabValue, newTabValue) => { const getTabIdByDisplayIndex = (state, windowId, index) => { index = validateIndex(index) windowId = validateId('windowId', windowId) - - const tabId = state.getIn(['tabsInternal', 'displayIndex', windowId.toString(), index.toString()]) - return tabId == null ? tabState.TAB_ID_NONE : tabId -} - -const updateTabsDisplayIndex = (state, oldTabValue, newTabValue) => { - const oldTabId = validateId('tabId', oldTabValue.get('tabId')) - const oldWindowId = validateId('windowId', oldTabValue.get('windowId')) - const oldIndex = validateIndex(oldTabValue.get('index')) - if (oldIndex !== -1 && oldTabId !== tabState.TAB_ID_NONE && oldWindowId !== windowState.WINDOW_ID_NONE) { - state = state.deleteIn(['tabsInternal', 'displayIndex', oldWindowId.toString(), oldIndex.toString()]) - } - - const tabId = validateId('tabId', newTabValue.get('tabId')) - const windowId = validateId('windowId', newTabValue.get('windowId')) - const index = validateIndex(newTabValue.get('index')) - if (index !== -1 && tabId !== tabState.TAB_ID_NONE && windowId !== windowState.WINDOW_ID_NONE) { - return state.setIn(['tabsInternal', 'displayIndex', windowId.toString(), index.toString()], tabId) + const tabValue = tabState.queryTab(state, {windowId, index}) + if (!tabValue) { + return tabState.TAB_ID_NONE } - - return state + return tabValue.get('tabId') } // The internal index is the location of the tabId in the tabs array @@ -133,12 +117,7 @@ const deleteTabsInternalIndex = (state, tabValue) => { if (tabId === tabState.TAB_ID_NONE) { return state } - const windowId = validateId('windowId', tabValue.get('windowId')) - const displayIndex = validateIndex(tabValue.get('index')) - if (displayIndex !== -1 && windowId !== windowState.WINDOW_ID_NONE) { - state = state.deleteIn(['tabsInternal', 'displayIndex', windowId.toString(), displayIndex.toString()]) - } if (windowId !== windowState.WINDOW_ID_NONE) { let activeList = state.getIn(['tabsInternal', 'lastActive', windowId.toString()], Immutable.OrderedSet()) activeList = activeList.remove(tabId) @@ -210,7 +189,6 @@ const tabState = { let tabValue = validateTabValue(action.get('tabValue')) assert.ok(!tabState.getTab(state, tabValue), 'Tab already exists') state = state.set('tabs', state.get('tabs').push(tabValue)) - state = updateTabsDisplayIndex(state, tabValue, tabValue) state = updateLastActive(state, tabValue, tabValue) return updateTabsInternalIndex(state, state.get('tabs').size - 1) }, @@ -422,7 +400,6 @@ const tabState = { tabValue = currentTabValue.mergeDeep(tabValue) } - state = updateTabsDisplayIndex(state, currentTabValue, tabValue) state = updateLastActive(state, currentTabValue, tabValue) return state.set('tabs', tabs.delete(index).insert(index, tabValue)) }, diff --git a/app/renderer/components/frame/frame.js b/app/renderer/components/frame/frame.js index 26be02a572e..1cf1ceb3371 100644 --- a/app/renderer/components/frame/frame.js +++ b/app/renderer/components/frame/frame.js @@ -162,12 +162,14 @@ class Frame extends React.Component { this.addEventListeners() if (cb) { this.runOnDomReady = cb - let eventCallback = (e) => { - this.webview.removeEventListener(e.type, eventCallback) + let didAttachCallback = (e) => { + this.webview.removeEventListener(e.type, didAttachCallback) this.runOnDomReady() delete this.runOnDomReady } - this.webview.addEventListener('did-attach', eventCallback, { passive: true }) + this.webview.addEventListener('will-attach', () => { + }) + this.webview.addEventListener('did-attach', didAttachCallback, { passive: true }) } if (!this.props.guestInstanceId || !this.webview.attachGuest(this.props.guestInstanceId)) { diff --git a/app/renderer/components/tabs/tab.js b/app/renderer/components/tabs/tab.js index 63cc64f7e01..cdd0c29896a 100644 --- a/app/renderer/components/tabs/tab.js +++ b/app/renderer/components/tabs/tab.js @@ -72,7 +72,7 @@ class Tab extends React.Component { get draggingOverData () { const draggingOverData = this.props.dragData && this.props.dragData.get('dragOverData') if (!draggingOverData || - draggingOverData.get('draggingOverKey') !== this.props.frameKey || + draggingOverData.get('draggingOverKey') !== this.props.tabId || draggingOverData.get('draggingOverWindowId') !== getCurrentWindowId()) { return } @@ -82,8 +82,8 @@ class Tab extends React.Component { return } const location = sourceDragData.get('location') - const key = draggingOverData.get('draggingOverKey') - const draggingOverFrame = windowStore.getFrame(key) + const tabId = draggingOverData.get('draggingOverKey') + const draggingOverFrame = windowStore.getFrameByTabId(tabId) if ((location === 'about:blank' || location === 'about:newtab' || isIntermediateAboutPage(location)) && (draggingOverFrame && draggingOverFrame.get('pinnedLocation'))) { return @@ -95,7 +95,7 @@ class Tab extends React.Component { get isDragging () { const sourceDragData = dnd.getInterBraveDragData() return sourceDragData && - sourceDragData.get('key') === this.props.frameKey && + sourceDragData.get('tabId') === this.props.tabId && sourceDragData.get('draggingOverWindowId') === getCurrentWindowId() } @@ -105,7 +105,7 @@ class Tab extends React.Component { if (!draggingOverData || !sourceDragData) { return false } - return draggingOverData.get('draggingOverKey') === sourceDragData.get('key') + return draggingOverData.get('draggingOverKey') === sourceDragData.get('tabId') } get isDraggingOverLeft () { @@ -131,7 +131,7 @@ class Tab extends React.Component { } onDragOver (e) { - dnd.onDragOver(dragTypes.TAB, this.tabNode.getBoundingClientRect(), this.props.frameKey, this.draggingOverData, e) + dnd.onDragOver(dragTypes.TAB, this.tabNode.getBoundingClientRect(), this.props.tabId, this.draggingOverData, e) } onMouseLeave (e) { diff --git a/app/renderer/lib/tabUtil.js b/app/renderer/lib/tabUtil.js index d122e738432..ffc3849b7d8 100644 --- a/app/renderer/lib/tabUtil.js +++ b/app/renderer/lib/tabUtil.js @@ -46,7 +46,9 @@ module.exports.hasBreakpoint = (breakpoint, arr) => { module.exports.hasTabAsRelatedTarget = (event) => { const relatedTarget = event.relatedTarget const hasDataset = relatedTarget.parentNode && relatedTarget.parentNode.dataset + if (!hasDataset) { + return false + } const tabAsRelatedTarget = hasDataset.tab || hasDataset.tabArea - - return hasDataset && tabAsRelatedTarget + return tabAsRelatedTarget } diff --git a/app/renderer/reducers/frameReducer.js b/app/renderer/reducers/frameReducer.js index 768b3f2761f..b140925bea7 100644 --- a/app/renderer/reducers/frameReducer.js +++ b/app/renderer/reducers/frameReducer.js @@ -16,7 +16,6 @@ const appActions = require('../../../js/actions/appActions') // Utils const frameStateUtil = require('../../../js/state/frameStateUtil') -const {getCurrentWindowId} = require('../currentWindow') const {getSourceAboutUrl, getSourceMagnetUrl} = require('../../../js/lib/appUrlUtil') const {isURL, isPotentialPhishingUrl, getUrlFromInput} = require('../../../js/lib/urlutil') const bookmarkUtil = require('../../common/lib/bookmarkUtil') @@ -41,16 +40,14 @@ const closeFrame = (state, action) => { state = state.merge(frameStateUtil.removeFrame( state, - frameProps.set('closedAtIndex', index), + frameProps + .set('closedAtIndex', index) + .delete('openerTabId'), index )) state = frameStateUtil.deleteFrameInternalIndex(state, frameProps) state = frameStateUtil.updateFramesInternalIndex(state, index) - if (state.get('frames', Immutable.List()).size === 0) { - appActions.closeWindow(getCurrentWindowId()) - } - const nextFrame = frameStateUtil.getFrameByIndex(state, index) if (nextFrame) { @@ -92,11 +89,29 @@ const frameReducer = (state, action, immutableAction) => { break } const tabId = tab.get('tabId') - const frame = frameStateUtil.getFrameByTabId(state, tabId) + let frame = frameStateUtil.getFrameByTabId(state, tabId) if (!frame) { break } + let frames = state.get('frames') + const changeInfoIndex = changeInfo.get('index') + const sourceFrameIndex = frameStateUtil.getFrameIndex(state, frame.get('key')) + // When cloning index can be largerthan number of created frames, in this + // case we don't want to do anything. + if (changeInfoIndex !== undefined && + sourceFrameIndex !== changeInfoIndex && + changeInfoIndex < frames.size) { + frame = frame.set('index', changeInfoIndex) + frames = frames + .splice(sourceFrameIndex, 1) + .splice(changeInfoIndex, 0, frame) + state = state.set('frames', frames) + // Since the tab could have changed pages, update the tab page as well + state = frameStateUtil.updateFramesInternalIndex(state, Math.min(sourceFrameIndex, changeInfoIndex)) + state = frameStateUtil.moveFrame(state, tabId, changeInfoIndex) + } + const index = frameStateUtil.getIndexByTabId(state, tabId) const pinned = immutableAction.getIn(['changeInfo', 'pinned']) if (pinned != null) { diff --git a/app/sessionStore.js b/app/sessionStore.js index b3ede211cce..ba2214e0b3c 100644 --- a/app/sessionStore.js +++ b/app/sessionStore.js @@ -154,11 +154,15 @@ module.exports.cleanPerWindowData = (immutablePerWindowData, isShutdown) => { } let newKey = 0 - const cleanFrame = (immutableFrame) => { + let activeFrameKey = immutablePerWindowData.get('activeFrameKey') + // If adjustActive is set to true then activeFrameKey will be set to the new frame key. + // We re-use this function for both closedFrames and frames, and we only want to adjust the active for frames. + const cleanFrame = (immutableFrame, adjustActive) => { newKey++ // Reset the ids back to sequential numbers - if (immutableFrame.get('key') === immutablePerWindowData.get('activeFrameKey')) { - immutablePerWindowData = immutablePerWindowData.set('activeFrameKey', newKey) + if (adjustActive && + immutableFrame.get('key') === immutablePerWindowData.get('activeFrameKey')) { + activeFrameKey = newKey } else { // For now just set everything to unloaded unless it's the active frame immutableFrame = immutableFrame.set('unloaded', true) @@ -193,6 +197,7 @@ module.exports.cleanPerWindowData = (immutablePerWindowData, isShutdown) => { 'guestInstanceId', // Tab ids are per-session and should not be persisted 'tabId', + 'openerTabId', // Do not show the audio indicator until audio starts playing 'audioMuted', 'audioPlaybackActive', @@ -246,20 +251,24 @@ module.exports.cleanPerWindowData = (immutablePerWindowData, isShutdown) => { if (immutablePerWindowData.get('closedFrames')) { immutablePerWindowData = immutablePerWindowData.get('closedFrames').reduce((immutablePerWindowData, immutableFrame, index) => { - const cleanImmutableFrame = cleanFrame(immutableFrame) + const cleanImmutableFrame = cleanFrame(immutableFrame, false) return immutablePerWindowData.setIn(['closedFrames', index], cleanImmutableFrame) }, immutablePerWindowData) } - if (immutablePerWindowData.get('frames')) { + let immutableFrames = immutablePerWindowData.get('frames') + if (immutableFrames) { // Don't restore pinned locations because they will be auto created by the app state change event - immutablePerWindowData = immutablePerWindowData.set('frames', - immutablePerWindowData.get('frames') - .filter((frame) => !frame.get('pinnedLocation'))) + immutableFrames = immutableFrames + .filter((frame) => !frame.get('pinnedLocation')) + immutablePerWindowData = immutablePerWindowData.set('frames', immutableFrames) immutablePerWindowData = - immutablePerWindowData.get('frames').reduce((immutablePerWindowData, immutableFrame, index) => { - const cleanImmutableFrame = cleanFrame(immutableFrame) + immutableFrames.reduce((immutablePerWindowData, immutableFrame, index) => { + const cleanImmutableFrame = cleanFrame(immutableFrame, true) return immutablePerWindowData.setIn(['frames', index], cleanImmutableFrame) }, immutablePerWindowData) + if (activeFrameKey !== undefined) { + immutablePerWindowData = immutablePerWindowData.set('activeFrameKey', activeFrameKey) + } } return immutablePerWindowData } diff --git a/js/actions/appActions.js b/js/actions/appActions.js index 7f94b24d972..6a49cafe0d7 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -84,6 +84,17 @@ const appActions = { }) }, + /** + * The tab strip is empty + * @param {Number} windowId + */ + tabStripEmpty: function (windowId) { + dispatch({ + actionType: appConstants.APP_TAB_STRIP_EMPTY, + windowId + }) + }, + /** * A new tab has been created * @param {Object} tabValue @@ -95,6 +106,39 @@ const appActions = { }) }, + /** + * Tab moved event fired from muon + * @param {Object} tabValue + */ + tabMoved: function (tabId) { + dispatch({ + actionType: appConstants.APP_TAB_MOVED, + tabId + }) + }, + + /** + * A tab has been attached + * @param {Object} tabValue + */ + tabAttached: function (tabId) { + dispatch({ + actionType: appConstants.APP_TAB_ATTACHED, + tabId + }) + }, + + /** + * A tab will be attached + * @param {Object} tabValue + */ + tabWillAttach: function (tabId) { + dispatch({ + actionType: appConstants.APP_TAB_WILL_ATTACH, + tabId + }) + }, + /** * A tab has been moved to another window * @param {Number} tabId @@ -102,9 +146,9 @@ const appActions = { * @param {Object} browserOpts * @param {Number} windowId */ - tabMoved: function (tabId, frameOpts, browserOpts, windowId) { + tabDetachMenuItemClicked: function (tabId, frameOpts, browserOpts, windowId) { dispatch({ - actionType: appConstants.APP_TAB_MOVED, + actionType: appConstants.APP_TAB_DETACH_MENU_ITEM_CLICKED, tabId, frameOpts, browserOpts, diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index 21a001542ee..7b22093df33 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -223,13 +223,6 @@ const windowActions = { }) }, - activeFrameChanged: function (frameProps) { - dispatch({ - actionType: windowConstants.WINDOW_ACTIVE_FRAME_CHANGED, - frameProps: frameProps - }) - }, - /** * Dispatches a message to the store when the frame is active and the window is focused * diff --git a/js/constants/appConstants.js b/js/constants/appConstants.js index 4e1f60a52e9..83468ed6bad 100644 --- a/js/constants/appConstants.js +++ b/js/constants/appConstants.js @@ -11,6 +11,7 @@ const appConstants = { APP_WINDOW_CLOSED: _, APP_WINDOW_CREATED: _, APP_WINDOW_UPDATED: _, + APP_TAB_STRIP_EMPTY: _, APP_TAB_CLOSED: _, APP_TAB_UPDATED: _, APP_ADD_HISTORY_SITE: _, @@ -54,6 +55,9 @@ const appConstants = { APP_FRAME_CHANGED: _, APP_TAB_CREATED: _, APP_TAB_MOVED: _, + APP_TAB_DETACH_MENU_ITEM_CLICKED: _, + APP_TAB_ATTACHED: _, + APP_TAB_WILL_ATTACH: _, APP_TAB_ACTIVATE_REQUESTED: _, APP_TAB_INDEX_CHANGED: _, APP_TAB_CLOSE_REQUESTED: _, diff --git a/js/contextMenus.js b/js/contextMenus.js index db4a34b0229..273bbf9da33 100644 --- a/js/contextMenus.js +++ b/js/contextMenus.js @@ -489,7 +489,7 @@ function tabTemplateInit (frameProps) { label: locale.translation('detach'), click: (item) => { const browserOpts = { positionByMouseCursor: true, checkMaximized: true } - appActions.tabMoved(tabId, frameProps.toJS(), browserOpts, -1) + appActions.tabDetachMenuItemClicked(tabId, frameProps.toJS(), browserOpts, -1) } }) } diff --git a/js/state/frameStateUtil.js b/js/state/frameStateUtil.js index 41cc48f25f1..759c68b6b22 100644 --- a/js/state/frameStateUtil.js +++ b/js/state/frameStateUtil.js @@ -11,7 +11,6 @@ const settings = require('../constants/settings') // Actions const windowActions = require('../actions/windowActions') const webviewActions = require('../actions/webviewActions') -const appActions = require('../actions/appActions') // State const {makeImmutable} = require('../../app/common/state/immutableUtil') @@ -319,7 +318,7 @@ const frameOptsFromFrame = (frame) => { * Adds a frame specified by frameOpts and newKey and sets the activeFrameKey * @return Immutable top level application state ready to merge back in */ -function addFrame (state, frameOpts, newKey, partitionNumber, openInForeground, insertionIndex) { +function addFrame (state, frameOpts, newKey, partitionNumber, openInForeground, insertionIndex, active) { const frames = state.get('frames') const location = frameOpts.location // page url @@ -523,18 +522,30 @@ const deleteFrameInternalIndex = (state, frame) => { const updateFramesInternalIndex = (state, fromIndex) => { let framesInternal = state.get('framesInternal') || Immutable.Map() - state.get('frames').slice(fromIndex).forEach((frame, idx) => { + state.get('frames').slice(fromIndex).reduceRight((result, frame, idx) => { + const tabId = frame.get('tabId') + const frameKey = frame.get('key') const realIndex = idx + fromIndex - if (frame.get('key')) { - framesInternal = framesInternal.setIn(['index', frame.get('key').toString()], realIndex) + if (frameKey) { + framesInternal = framesInternal.setIn(['index', frameKey.toString()], realIndex) } - if (frame.get('tabId') !== -1) { - framesInternal = framesInternal.setIn(['tabIndex', frame.get('tabId').toString()], realIndex) + if (tabId != null && tabId !== -1) { + framesInternal = framesInternal.setIn(['tabIndex', tabId.toString()], realIndex) } + }, 0) + return state.set('framesInternal', framesInternal) +} - appActions.tabIndexChanged(frame.get('tabId'), realIndex) - }) - +const moveFrame = (state, tabId, index) => { + let framesInternal = state.get('framesInternal') || Immutable.Map() + const frame = getFrameByTabId(state, tabId) + const frameKey = frame.get('key') + if (frameKey) { + framesInternal = framesInternal.setIn(['index', frameKey.toString()], index) + } + if (tabId != null && tabId !== -1) { + framesInternal = framesInternal.setIn(['tabIndex', tabId.toString()], index) + } return state.set('framesInternal', framesInternal) } @@ -716,6 +727,7 @@ module.exports = { deleteTabInternalIndex, deleteFrameInternalIndex, updateFramesInternalIndex, + moveFrame, query, find, isAncestorFrameKey, diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index ccf4ce0882d..c8348bb9990 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -72,6 +72,10 @@ class WindowStore extends EventEmitter { return frameStateUtil.getFrameByKey(windowState, key) } + getFrameByTabId (tabId) { + return frameStateUtil.getFrameByTabId(windowState, tabId) + } + emitChanges () { if (lastEmittedState !== windowState) { lastEmittedState = windowState @@ -105,7 +109,7 @@ const newFrame = (state, frameOpts) => { // handle tabs.create properties let insertionIndex = frameOpts.index !== undefined ? frameOpts.index - : undefined + : 0 if (frameOpts.partition) { frameOpts.isPrivate = frameStateUtil.isPrivatePartition(frameOpts.partition) @@ -117,7 +121,6 @@ const newFrame = (state, frameOpts) => { const active = frameOpts.active delete frameOpts.active - delete frameOpts.openInForeground // clean up any legacy openInForeground props let openInForeground = active if (openInForeground == null && frameOpts.disposition) { @@ -144,45 +147,11 @@ const newFrame = (state, frameOpts) => { frameOpts.location = '' } } - - // TODO: longer term get rid of parentFrameKey completely instead of - // calculating it here. - let parentFrameKey = frameOpts.parentFrameKey - if (frameOpts.openerTabId) { - parentFrameKey = frameStateUtil.getFrameKeyByTabId(state, frameOpts.openerTabId) - } - - // Find the closest index to the current frame's index which has - // a different ancestor frame key. - const frames = frameStateUtil.getFrames(state) - if (insertionIndex === undefined) { - insertionIndex = frameStateUtil.getFrameIndex(state, frameOpts.indexByFrameKey || parentFrameKey) - if (frameOpts.prependIndexByFrameKey === false) { - insertionIndex++ - } - if (insertionIndex === -1) { - insertionIndex = frames.size - // frameOpts.indexByFrameKey is used when the insertionIndex should be used exactly - } else if (!frameOpts.indexByFrameKey) { - while (insertionIndex < frames.size) { - ++insertionIndex - if (!frameStateUtil.isAncestorFrameKey(state, frames.get(insertionIndex), parentFrameKey)) { - break - } - } - } - } - if (frameStateUtil.isFrameKeyPinned(state, parentFrameKey)) { - insertionIndex = 0 - } - const nextKey = incrementNextKey() - state = state.merge( frameStateUtil.addFrame( state, frameOpts, nextKey, frameOpts.partitionNumber, openInForeground, insertionIndex)) - state = frameStateUtil.updateFramesInternalIndex(state, insertionIndex) if (openInForeground) { @@ -213,7 +182,8 @@ const frameTabIdChanged = (state, action) => { const index = frameStateUtil.getFrameIndex(state, action.getIn(['frameProps', 'key'])) state = state.mergeIn(['frames', index], newFrameProps) state = frameStateUtil.deleteTabInternalIndex(state, oldTabId) - return frameStateUtil.updateFramesInternalIndex(state, index) + state = frameStateUtil.updateFramesInternalIndex(state, index) + return state } const frameGuestInstanceIdChanged = (state, action) => { @@ -436,7 +406,9 @@ const doAction = (action) => { windowState = windowState.set('frames', frames) // Since the tab could have changed pages, update the tab page as well windowState = frameStateUtil.updateFramesInternalIndex(windowState, Math.min(sourceFrameIndex, newIndex)) + windowState = frameStateUtil.moveFrame(windowState, sourceFrameProps.get('tabId'), newIndex) windowState = frameStateUtil.updateTabPageIndex(windowState, activeFrame.get('tabId')) + appActions.tabIndexChanged(activeFrame.get('tabId'), newIndex) break } case windowConstants.WINDOW_SET_LINK_HOVER_PREVIEW: diff --git a/test/app/sessionStoreTest.js b/test/app/sessionStoreTest.js index 9d375585ac1..2790ceca3e9 100644 --- a/test/app/sessionStoreTest.js +++ b/test/app/sessionStoreTest.js @@ -178,8 +178,8 @@ describe('sessionStore test', function () { .waitForExist(pinnedTabsTabs) .waitForElementCount(pinnedTabsTabs, 1) .waitForElementCount(tabsTabs, 2) - .activateTabByIndex(0) - .waitForExist(this.activeTabSelector) + .activateTabByIndex(1) + .waitForExist(this.activeTabSelector, 30000) yield Brave.stopApp(false) yield Brave.startApp() yield setupBrave(Brave.app.client) diff --git a/test/lib/brave.js b/test/lib/brave.js index 2a4af7aa21f..10766c86ceb 100644 --- a/test/lib/brave.js +++ b/test/lib/brave.js @@ -587,7 +587,7 @@ var exports = { const frame = val.value.frames[index] return this.execute(function (tabId, windowId, frame) { const browserOpts = { positionByMouseCursor: true } - devTools('appActions').tabMoved(tabId, frame, browserOpts, windowId) + devTools('appActions').tabDetachMenuItemClicked(tabId, frame, browserOpts, windowId) }, frame.tabId, windowId, frame) }) }) diff --git a/test/tab-components/pinnedTabTest.js b/test/tab-components/pinnedTabTest.js index 0710b78d5a9..8f05dcba4e8 100644 --- a/test/tab-components/pinnedTabTest.js +++ b/test/tab-components/pinnedTabTest.js @@ -14,8 +14,8 @@ describe('pinnedTabs', function () { } describe('Pins an existing frame', function () { - Brave.beforeAll(this) - before(function * () { + Brave.beforeEach(this) + beforeEach(function * () { yield setup(this.app.client) this.page1 = Brave.server.url('page1.html') yield this.app.client @@ -34,7 +34,7 @@ describe('pinnedTabs', function () { }) it('unpins and creates a non-pinned tab', function * () { yield this.app.client - .pinTabByIndex(1, false) + .pinTabByIndex(0, false) .waitForExist('[data-test-pinned-tab="false"][data-frame-key="2"]') .waitForElementCount(pinnedTabsTabs, 0) .waitForElementCount(tabsTabs, 2) @@ -42,7 +42,6 @@ describe('pinnedTabs', function () { }) it('pinning the same site again combines it', function * () { yield this.app.client - .pinTabByIndex(1, true) .newTab({ url: this.page1 }) .waitForUrl(this.page1) .windowByUrl(Brave.browserWindowUrl) @@ -58,7 +57,7 @@ describe('pinnedTabs', function () { .url(pdfUrl) .pause(1000) // wait for PDF load .windowByUrl(Brave.browserWindowUrl) - .pinTabByIndex(0, true) + .pinTabByIndex(1, true) .waitForElementCount(pinnedTabsTabs, 2) .waitForElementCount(tabsTabs, 0) }) @@ -176,9 +175,10 @@ describe('pinnedTabs', function () { it('from same window as pinned', function * () { yield this.app.client + .windowByIndex(0) .pinTabByIndex(1, false) .waitForElementCount(pinnedTabsTabs, 0) - .windowByIndex(0) + .windowByIndex(1) .waitForElementCount(pinnedTabsTabs, 0) }) diff --git a/test/tab-components/tabTest.js b/test/tab-components/tabTest.js index 3a35e8fff75..b3bd13c1718 100644 --- a/test/tab-components/tabTest.js +++ b/test/tab-components/tabTest.js @@ -270,8 +270,8 @@ describe('tab tests', function () { describe('close tab', function () { const tabCountBeforeTabClose = 2 const tabCountAfterTabClose = 1 - Brave.beforeAll(this) - before(function * () { + Brave.beforeEach(this) + beforeEach(function * () { this.page1 = Brave.server.url('page1.html') yield setup(this.app.client) }) @@ -467,7 +467,8 @@ describe('tab tests', function () { // no preview should be shown .waitForElementCount('.frameWrapper.isPreview webview', 0) }) - it('preview the next tab if preview option is on', function * () { + // Failing on master + it.skip('preview the next tab if preview option is on', function * () { yield this.app.client .moveToObject('[data-test-id="tab"][data-frame-key="2"]') .moveToObject('[data-test-id="tab"][data-frame-key="2"]', 3, 3) diff --git a/test/unit/app/browser/reducers/tabsReducerTest.js b/test/unit/app/browser/reducers/tabsReducerTest.js index 667478a9f3a..be53e402f7c 100644 --- a/test/unit/app/browser/reducers/tabsReducerTest.js +++ b/test/unit/app/browser/reducers/tabsReducerTest.js @@ -63,17 +63,6 @@ describe('tabsReducer unit tests', function () { 4: 3, 5: 4 }, - displayIndex: { - 1: { - 0: 1, - 1: 2 - }, - 2: { - 2: 3, - 3: 4, - 4: 5 - } - }, lastActive: { 1: [0, 1], 2: [4, 3, 2] @@ -100,6 +89,7 @@ describe('tabsReducer unit tests', function () { setActive: sinon.spy(), moveTo: sinon.mock(), reload: sinon.mock(), + updateTabsStateForWindow: sinon.mock(), create: sinon.mock() } @@ -112,13 +102,16 @@ describe('tabsReducer unit tests', function () { mockery.registerMock('../tabs', this.tabsAPI) mockery.registerMock('../windows', this.windowsAPI) mockery.registerMock('../../common/state/tabState', this.tabStateAPI) - mockery.registerMock('../../../js/settings', { getSetting: (settingKey, settingsCollection, value) => { + mockery.registerMock('../../js/settings', { getSetting: (settingKey, settingsCollection, value) => { if (settingKey === settings.TAB_CLOSE_ACTION) { return tabCloseSetting } return false }}) tabsReducer = require('../../../../../app/browser/reducers/tabsReducer') + + this.realTabsAPI = require('../../../../../app/browser/tabs') + this.tabsAPI.getNextActiveTab = this.realTabsAPI.getNextActiveTab }) after(function () { @@ -223,8 +216,14 @@ describe('tabsReducer unit tests', function () { }) }) + describe.skip('APP_TAB_DETACH_MENU_ITEM_CLICKED', function () { + it('Someone clicked the detach menu item', function () { + // TODO + }) + }) + describe.skip('APP_TAB_MOVED', function () { - it('moves a tab', function () { + it('A tab has moved', function () { // TODO }) }) @@ -272,11 +271,13 @@ describe('tabsReducer unit tests', function () { afterEach(function () { this.removeTabByTabIdSpy.restore() + this.tabsAPI.updateTabsStateForWindow.reset() }) it('calls tabState.removeTabByTabId', function () { tabsReducer(this.state, action) - assert(this.tabStateAPI.removeTabByTabId.withArgs(this.state, action.tabId).calledOnce) + assert.equal(this.tabStateAPI.removeTabByTabId.getCall(0).args[1], action.tabId) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) it('does nothing if tabId is TAB_ID_NONE', function () { @@ -301,6 +302,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(this.state, action) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(3).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) }) @@ -324,6 +326,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(testState, pickNextAction) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(4).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) it('chooses next unpinned tab', function () { @@ -338,6 +341,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(testState, pickNextAction) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(5).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) it('chooses previous unpinned tab', function () { @@ -348,6 +352,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(testState, action) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(3).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) describe('if no unpinned tabs come after this', function () { @@ -364,6 +369,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(testState, pickNextAction) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(4).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) }) }) @@ -373,6 +379,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(this.state, action) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(4).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) it('chooses parent tab id (even if parent tab was NOT last active)', function () { @@ -382,6 +389,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(testState, action) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(3).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) }) }) @@ -577,13 +585,6 @@ describe('tabsReducer unit tests', function () { beforeEach(function () { this.pinnedTabs = sinon.stub(this.tabStateAPI, 'getPinnedTabsByWindowId', (state, windowId) => Immutable.fromJS([])) }) - - it('closes window when there are no tabs left', function () { - tabsReducer(this.state, action) - this.clock.tick(1510) - assert(this.tabsAPI.toggleDevTools.notCalled) - assert(this.windowsAPI.closeWindow.withArgs(this.state, 1).calledOnce) - }) }) }) }) @@ -706,12 +707,10 @@ describe('tabsReducer unit tests', function () { assert.equal(args[0], state) // State is passed in as first arg assert.equal(args[1], 1) // tabId is 1 for first tab // frameOpts being dragged is for the first tab - assert.deepEqual(args[2], { tabId: 1, + assert.deepEqual(args[2].toJS(), { tabId: 1, windowId: 1, pinned: false, - active: true, - indexByFrameKey: undefined, - prependIndexByFrameKey: undefined + active: true }) // Passes browser options for position by mouse cursor assert.deepEqual(args[3], { diff --git a/test/unit/app/browser/tabsTest.js b/test/unit/app/browser/tabsTest.js index 409865d388c..4f900c33f85 100644 --- a/test/unit/app/browser/tabsTest.js +++ b/test/unit/app/browser/tabsTest.js @@ -81,7 +81,10 @@ describe('tabs API unit tests', function () { tabValue: () => this.state.get('tabs').find((tab) => tab.get('tabId') === tabId), isDestroyed: () => false, - detach: (cb) => cb() + detach: (cb) => cb(), + once: (event, cb) => { + setImmediate(cb) + } } if (tabId === 1) { Object.assign(webContents, this.tabWithDevToolsClosed) diff --git a/test/unit/js/stores/windowStoreTest.js b/test/unit/js/stores/windowStoreTest.js index f161f63186d..f3b258e0ee7 100644 --- a/test/unit/js/stores/windowStoreTest.js +++ b/test/unit/js/stores/windowStoreTest.js @@ -136,7 +136,7 @@ describe('Window store unit tests', function () { describe('APP_NEW_WEB_CONTENTS_ADDED', function () { let windowState - let tabIndexChangedStub + let tabDetachMenuItemClickedStub const demoWindowState = { frames: [{ security: { @@ -183,7 +183,15 @@ describe('Window store unit tests', function () { loading: false, unloaded: true, key: 2 - }] + }], + framesInternal: { + index: { + 8: 0 + }, + tabIndex: { + 8: 0 + } + } } const demoAction = { actionType: appConstants.APP_NEW_WEB_CONTENTS_ADDED, @@ -194,7 +202,8 @@ describe('Window store unit tests', function () { location: 'about:blank', partition: 'persist:default', active: true, - guestInstanceId: 8, + guestInstanceId: 2, + tabId: 8, isPinned: false, openerTabId: 8, disposition: 'foreground-tab', @@ -205,14 +214,14 @@ describe('Window store unit tests', function () { width: 300, active: true, height: 300, - guestInstanceId: 8, + guestInstanceId: 2, autoDiscardable: true, partition: 'persist:default', windowId: -1, incognito: false, canGoForward: false, url: '', - tabId: 33, + tabId: 8, index: -1, status: 'complete', highlighted: false, @@ -223,7 +232,7 @@ describe('Window store unit tests', function () { muted: false, reason: 'user' }, - id: 33, + id: 8, selected: true, discarded: false, canGoBack: false @@ -231,7 +240,7 @@ describe('Window store unit tests', function () { } beforeEach(function () { - tabIndexChangedStub = sinon.stub(appActions, 'tabIndexChanged') + tabDetachMenuItemClickedStub = sinon.stub(appActions, 'tabDetachMenuItemClicked') }) afterEach(function () { @@ -239,7 +248,7 @@ describe('Window store unit tests', function () { mockery.deregisterMock(reducer) }) - tabIndexChangedStub.restore() + tabDetachMenuItemClickedStub.restore() }) describe('when tab being opened is active', function () {