diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index 8f055e12720..c24fff54f7d 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -50,15 +50,14 @@ const updateActiveTab = (state, closeTabId) => { case tabCloseAction.PARENT: { const openerTabId = tabState.getOpenerTabId(state, closeTabId) - const lastActiveTabId = tabState.getLastActiveTabId(state, windowId) - if (openerTabId === lastActiveTabId) { + if (openerTabId !== tabState.TAB_ID_NONE) { nextTabId = openerTabId } break } } - // always fall back to NEXT + // DEFAULT: always fall back to NEXT if (nextTabId === tabState.TAB_ID_NONE) { nextTabId = tabState.getNextTabIdByIndex(state, windowId, index) if (nextTabId === tabState.TAB_ID_NONE) { diff --git a/app/common/state/tabState.js b/app/common/state/tabState.js index 72dbc44f6c1..befa69ce5c5 100644 --- a/app/common/state/tabState.js +++ b/app/common/state/tabState.js @@ -499,7 +499,15 @@ const tabState = { }, getOpenerTabId: (state, tabId) => { - return tabState.getTabPropertyByTabId(state, tabId, 'openerTabId', tabState.TAB_ID_NONE) + const openerTabId = tabState.getTabPropertyByTabId(state, tabId, 'openerTabId', tabState.TAB_ID_NONE) + if (openerTabId !== tabState.TAB_ID_NONE) { + // Validate that tab exists + const tab = tabState.getByTabId(state, openerTabId) + if (tab) { + return openerTabId + } + } + return tabState.TAB_ID_NONE }, getIndex: (state, tabId) => { diff --git a/test/unit/app/browser/reducers/tabsReducerTest.js b/test/unit/app/browser/reducers/tabsReducerTest.js index 2c6c0bf89d2..1fc58dc6fad 100644 --- a/test/unit/app/browser/reducers/tabsReducerTest.js +++ b/test/unit/app/browser/reducers/tabsReducerTest.js @@ -221,6 +221,7 @@ describe('tabsReducer unit tests', function () { after(function () { tabCloseSetting = tabCloseAction.PARENT }) + it('chooses the next tab', function () { const pickNextAction = { actionType: action.actionType, @@ -234,25 +235,8 @@ describe('tabsReducer unit tests', function () { this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(4).calledOnce) }) - }) - - describe('when TAB_CLOSE_ACTION is set to PARENT', function () { - it('chooses parent tab id (if parent tab was last active)', function () { - tabsReducer(this.state, action) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(4).calledOnce) - }) - }) - - describe('when last active tab is not set', function () { - beforeEach(function () { - this.getLastActiveTabIdStub = sinon.stub(this.tabStateAPI, 'getLastActiveTabId') - }) - afterEach(function () { - this.getLastActiveTabIdStub.restore() - }) - it('chooses next unpinned tab if nextTabId is TAB_ID_NONE', function () { + it('chooses next unpinned tab', function () { const pickNextAction = { actionType: action.actionType, tabId: 3 @@ -266,7 +250,7 @@ describe('tabsReducer unit tests', function () { assert(this.tabsAPI.setActive.withArgs(5).calledOnce) }) - it('chooses previous unpinned tab if nextTabId is TAB_ID_NONE', function () { + it('chooses previous unpinned tab', function () { const testState = this.state .setIn(['tabs', 2, 'active'], true) .setIn(['tabs', 3, 'active'], false) @@ -293,6 +277,23 @@ describe('tabsReducer unit tests', function () { }) }) }) + + describe('when TAB_CLOSE_ACTION is set to PARENT', function () { + it('chooses parent tab id (if parent tab was last active)', function () { + tabsReducer(this.state, action) + this.clock.tick(1510) + assert(this.tabsAPI.setActive.withArgs(4).calledOnce) + }) + + it('chooses parent tab id (even if parent tab was NOT last active)', function () { + const testState = this.state + .setIn(['tabs', 4, 'openerTabId'], 3) + .setIn(['tabsInternal', 'lastActive', '2'], Immutable.fromJS([])) + tabsReducer(testState, action) + this.clock.tick(1510) + assert(this.tabsAPI.setActive.withArgs(3).calledOnce) + }) + }) }) }) diff --git a/test/unit/app/common/state/tabStateTest.js b/test/unit/app/common/state/tabStateTest.js index 75fc8020a05..075e09962b0 100644 --- a/test/unit/app/common/state/tabStateTest.js +++ b/test/unit/app/common/state/tabStateTest.js @@ -19,8 +19,8 @@ const defaultAppState = Immutable.fromJS({ const twoTabsAppState = defaultAppState .set('tabs', Immutable.fromJS([ - { tabId: 1, index: 0, windowId: 1, frameKey: 2 }, - { tabId: 2, index: 1, windowId: 1, frameKey: 1 } + { tabId: 1, index: 0, windowId: 1 }, + { tabId: 2, index: 1, windowId: 1 } ])) // NOTE: null check can be optional since resolveTabId sets a default if null @@ -213,7 +213,6 @@ describe('tabState unit tests', function () { let tab = tabState.getByTabId(this.appState, 2) assert(tab) assert.equal(1, tab.get('windowId')) - assert.equal(1, tab.get('frameKey')) assert.equal(2, tab.get('tabId')) }) @@ -342,7 +341,6 @@ describe('tabState unit tests', function () { .set('tabs', Immutable.fromJS([ { windowId: 1, - frameKey: 1, tabId: 1, index: 0, myProp: 'test1', @@ -350,7 +348,6 @@ describe('tabState unit tests', function () { }, { windowId: 1, - frameKey: 1, tabId: 2, index: 1, myProp: 'test2', @@ -373,13 +370,11 @@ describe('tabState unit tests', function () { index: 0, test: 'blue', windowId: 1, - frameKey: 1, myProp: 'test2', myProp2: 'blah' }, { windowId: 1, - frameKey: 1, tabId: 2, index: 1, myProp: 'test2', @@ -400,7 +395,6 @@ describe('tabState unit tests', function () { }, { windowId: 1, - frameKey: 1, tabId: 2, index: 1, myProp: 'test2', @@ -416,7 +410,6 @@ describe('tabState unit tests', function () { assert.equal('test1', tab.get('myProp')) assert.equal('blah', tab.get('myProp2')) assert.equal(1, tab.get('windowId')) - assert.equal(1, tab.get('frameKey')) assert.equal(1, tab.get('tabId')) assert.equal(true, state.get('otherProp')) }) @@ -438,7 +431,6 @@ describe('tabState unit tests', function () { it('removes the field specified', function () { const tab = Immutable.fromJS({ windowId: 1, - frameKey: 1, tabId: 2, loginRequiredDetail: { request: { url: 'someurl' }, @@ -465,7 +457,6 @@ describe('tabState unit tests', function () { before(function () { const tabs = Immutable.fromJS([{ windowId: 1, - frameKey: 1, tabId: 2, loginRequiredDetail: { request: { url: 'someurl' }, @@ -631,7 +622,7 @@ describe('tabState unit tests', function () { ) assert.throws( () => { - tabState.setTabs(defaultAppState, [{ frameKey: 1 }]) + tabState.setTabs(defaultAppState, [{ index: 0 }]) }, AssertionError ) @@ -704,6 +695,24 @@ describe('tabState unit tests', function () { }) }) + describe('getOpenerTabId', function () { + it('returns tabId of the opener', function () { + const tempState = twoTabsAppState + .setIn(['tabs', 1, 'openerTabId'], 1) + assert.equal(tabState.getOpenerTabId(tempState, 2), 1) + }) + + it('defaults to TAB_ID_NONE if not found', function () { + assert.equal(tabState.getOpenerTabId(twoTabsAppState, 2), tabState.TAB_ID_NONE) + }) + + it('returns TAB_ID_NONE if tabId is invalid', function () { + const tempState = twoTabsAppState + .setIn(['tabs', 1, 'openerTabId'], 17) + assert.equal(tabState.getOpenerTabId(tempState, 2), tabState.TAB_ID_NONE) + }) + }) + describe('getActiveTabId', function () { before(function () { this.appState = defaultAppState