More robust next tab selection after closing an active tab #12193

petemill · 2017-12-06T00:59:04Z

Set next active tab before closing, instead of after. Avoids race-condition with muon's index-change and active-change tab update events.

Fix #11981
Fix #11526

Notable changes:

remove setting next active tab id in tabReducer, due to out-of-date tab state
tab creation: store the opener tabId
tab set-active: remember the history of active tabs
tab will-destroy: remove the tabId from the history of active tabs, and override muon's next active tab, if appropriate
tab did-detach: remove the opener for the detached tab, and remove the tab from active tabs window history

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Test Plan:

Automatic Tests

npm run unittest -- --grep "tabs API"

Manual Tests

Manual steps (perform on all platforms since this bug was timing-specific and affected Windows mostly).

All automatic tab active changes should be observed as immediate, without visually seeing one tab become active and then quickly another tab getting focus.

Parent tab

Preferences: detault (When closing an active tab, select its parent tab)
Open the new tab page
Cmd-click all the top sites icons to open 'child' tabs
Switch active focus to the 3rd tab
Close the 3rd (current) tab
Expected: Tab active focus switches to the 1st tab (new tab page)

Next tab

Close the (active) new tab page
Expected: Tab active focus switches to the tab that was 2nd, and now is 1st
Preferences: When closing an active tab Select the next tab
Open the new tab page
Cmd-click all the top sites icons to open 'child' tabs
Switch active focus to the 3rd tab
Close the 3rd (current) tab
Expected: Tab active focus switches to the tab that was 4th, and is now 3rd

Last active tab

Preferences: When closing an active tab Select the last viewed tab
Open at least 5 tabs
Make the 1st tab active
Make the 3rd tab active
Make the last tab active
Close the currently-active tab (the last tab)
Expected: Tab active focus switches to the 3rd tab
Close the currently-active tab (the 3rd tab)
Expected: Tab active focus switches to the 1st tab
Close the currently-active tab (the 1st tab)
Expected: Tab active focus switches to the 2nd tab

Detached parent tab

Preferences: detault (When closing an active tab, select its parent tab)
Open the new tab page
Cmd-click all the top sites icons to open 'child' tabs
Detach the parent tab (the first tab) to a new window
Open another tab in the new window and keep it active (so the original parent is inactive in the new window)
In the original window, switch active focus to the 3rd tab in the original window
Close the 3rd (current) tab
Expected: Tab active focus switches to the 4th tab in the first window, and no change of active tab changes in the second window, i.e. the parent tab is not made active.

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

bsclifton · 2017-12-20T16:26:37Z

app/browser/activeTabHistory.js

@@ -0,0 +1,57 @@
+// static


can we put the standard preamble at the top of this file?

done, and rebased

bsclifton · 2017-12-20T16:32:51Z

app/browser/tabs.js

 const {app, BrowserWindow, extensions, session, ipcMain} = require('electron')
 const {makeImmutable, makeJS} = require('../common/state/immutableUtil')
 const {getTargetAboutUrl, getSourceAboutUrl, isSourceAboutUrl, newFrameUrl, isTargetAboutUrl, isIntermediateAboutPage, isTargetMagnetUrl, getSourceMagnetUrl} = require('../../js/lib/appUrlUtil')
 const {isURL, getUrlFromInput, toPDFJSLocation, getDefaultFaviconUrl, isHttpOrHttps, getLocationIfPDF} = require('../../js/lib/urlutil')
 const {isSessionPartition} = require('../../js/state/frameStateUtil')
 const {getOrigin} = require('../../js/lib/urlutil')
-const {getSetting} = require('../../js/settings')
+const settingsStore = require('../../js/settings')


Any specific reason for changing this? (just curious - I didn't see any other functions from settingsStore being called)

@bsclifton I changed this so I could register a stub for getSetting. I couldn't see another way to do it unfortunately, but if there's a way to do it whilst preserving the ability to directly reference the child function of the settingsStore object in the module, I'd be keen to learn. The function is stubbed with the following:

+ this.stubGetSettings = sinon.stub(this.settings, 'getSetting', + (settingKey, settingsCollection, value) => { + if (settingKey === settings.TAB_CLOSE_ACTION) { + return tabCloseSettingValue + } + return false + } + ) + })

bsclifton

Left two questions- otherwise looks great 😄 I ran through STR for both issues and confirmed it works as expected. Code looks great too! Nice to have all the logic in app/browser/tabs now (instead of being split up)

…nt from the tab muon will set. Avoids race condition that can occur after will-destroy, before appState has been fully updated with any tab details that change from muon as a result of a tab closing. Fix #11981 Fix #11526

codecov-io · 2017-12-22T03:31:38Z

Codecov Report

Merging #12193 into master will decrease coverage by 0.28%.
The diff coverage is 51.85%.

@@            Coverage Diff             @@
##           master   #12193      +/-   ##
==========================================
- Coverage   55.36%   55.07%   -0.29%     
==========================================
  Files         276      277       +1     
  Lines       26764    26803      +39     
  Branches     4315     4325      +10     
==========================================
- Hits        14819    14763      -56     
- Misses      11945    12040      +95

Flag	Coverage Δ
#unittest	`55.07% <51.85%> (-0.29%)`	⬇️

Impacted Files	Coverage Δ
app/browser/reducers/tabsReducer.js	`42.52% <ø> (-1%)`	⬇️
app/browser/windows.js	`37.59% <100%> (+0.06%)`	⬆️
app/browser/tabs.js	`25.3% <32.18%> (-1.14%)`	⬇️
app/browser/webContentsCache.js	`75% <81.25%> (+18.75%)`	⬆️
app/browser/activeTabHistory.js	`90.32% <90%> (ø)`
app/common/state/tabState.js	`64.57% <0%> (-14.32%)`	⬇️

bsclifton

Nice work 😄 👍

More robust next tab selection after closing an active tab

bsclifton · 2017-12-22T05:55:25Z

master 9b4ccd0
0.21.x cb0b638

petemill added the PR/work-in-progress ⚒ label Dec 6, 2017

petemill self-assigned this Dec 6, 2017

petemill force-pushed the fix/tab-close-index-jump-windows branch 3 times, most recently from d2db4c6 to dd96de6 Compare December 7, 2017 06:03

petemill removed the PR/work-in-progress ⚒ label Dec 7, 2017

petemill added this to the 0.19.x Hotfix 7 (Release channel) milestone Dec 7, 2017

petemill added regression bug feature/tabsbar priority/P4 Minor loss of function. Workaround usually present. and removed priority/P4 Minor loss of function. Workaround usually present. labels Dec 7, 2017

petemill requested review from bsclifton and bridiver December 7, 2017 06:06

petemill force-pushed the fix/tab-close-index-jump-windows branch from dd96de6 to bf11d30 Compare December 7, 2017 06:45

petemill changed the title ~~Set next active tab before closing~~ More robust next tab selection after closing an active tab Dec 7, 2017

petemill force-pushed the fix/tab-close-index-jump-windows branch from bf11d30 to 0acef5a Compare December 7, 2017 07:12

bsclifton force-pushed the fix/tab-close-index-jump-windows branch from 0acef5a to 30dc2f1 Compare December 20, 2017 05:45

bsclifton reviewed Dec 20, 2017

View reviewed changes

bsclifton previously approved these changes Dec 20, 2017

View reviewed changes

bsclifton modified the milestones: 0.19.x Hotfix 9, 0.21.x (Developer Channel) Dec 20, 2017

petemill dismissed bsclifton’s stale review via b714140 December 22, 2017 03:05

petemill force-pushed the fix/tab-close-index-jump-windows branch from 30dc2f1 to b714140 Compare December 22, 2017 03:05

bsclifton approved these changes Dec 22, 2017

View reviewed changes

bsclifton merged commit 9b4ccd0 into master Dec 22, 2017

bsclifton deleted the fix/tab-close-index-jump-windows branch December 22, 2017 05:54

bsclifton added a commit that referenced this pull request Dec 22, 2017

Merge pull request #12193 from brave/fix/tab-close-index-jump-windows

cb0b638

More robust next tab selection after closing an active tab

srirambv mentioned this pull request Dec 22, 2017

Closing tab switches to the second tab from root tab #11981

Closed

NumDeP mentioned this pull request Dec 22, 2017

Tabs Settings option doesn't work correctly after 0.19.48 update #11526

Closed

This was referenced Feb 20, 2018

Make sure we only forget a tab once it is destroyed. #13211

Merged

Fix tab auto-discarding ability #13120

Merged

LaurenWags mentioned this pull request Apr 4, 2018

'when closing an active tab' setting does not work #13733

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More robust next tab selection after closing an active tab #12193

More robust next tab selection after closing an active tab #12193

petemill commented Dec 6, 2017 •

edited by bsclifton

Loading

bsclifton Dec 20, 2017

petemill Dec 22, 2017

bsclifton Dec 20, 2017

petemill Dec 22, 2017

bsclifton left a comment

codecov-io commented Dec 22, 2017 •

edited

Loading

bsclifton left a comment

bsclifton commented Dec 22, 2017

More robust next tab selection after closing an active tab #12193

More robust next tab selection after closing an active tab #12193

Conversation

petemill commented Dec 6, 2017 • edited by bsclifton Loading

Test Plan:

Automatic Tests

Manual Tests

Parent tab

Next tab

Last active tab

Detached parent tab

bsclifton Dec 20, 2017

Choose a reason for hiding this comment

petemill Dec 22, 2017

Choose a reason for hiding this comment

bsclifton Dec 20, 2017

Choose a reason for hiding this comment

petemill Dec 22, 2017

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 22, 2017 • edited Loading

Codecov Report

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Dec 22, 2017

petemill commented Dec 6, 2017 •

edited by bsclifton

Loading

codecov-io commented Dec 22, 2017 •

edited

Loading