Fixes tab pages after setting change

Resolves #7806

Auditors: @bsclifton @cezaraugusto

Test Plan:
- Open about:preferences#tabs
- Set the number of tabs per tab set to 6
- Open new tabs to create a new tab set, which has just 2 tabs
- Increase the number to 8

app/renderer/reducers/frameReducer.js

@@ -128,7 +128,8 @@ const frameReducer = (state, action, immutableAction) => {
             state = state.deleteIn(['ui', 'tabs', 'previewTabPageIndex'])
           }
           state = state.setIn(['frames', index, 'lastAccessedTime'], new Date().getTime())
-          state = frameStateUtil.updateTabPageIndex(state, frame)
+
+          state = frameStateUtil.updateTabPageIndex(state, tabId)
         }
       }
       break

js/state/frameStateUtil.js

@@ -106,18 +106,18 @@ function getFrameKeysByDisplayIndex (state) {
   }, [])
 }
 
-function getFrameKeysByNonPinnedDisplayIndex (state) {
+function getTabIdsByNonPinnedDisplayIndex (state) {
   return state.get('frames')
     .filter((frame) => !frame.get('pinnedLocation'))
-    .map((frame) => frame.get('key'))
+    .map((frame) => frame.get('tabId'))
 }
 
  /**
- * Obtains the display index for the specified frame key excluding pins
+ * Obtains the display index for the specified tab ID excluding pins
  */
-function findNonPinnedDisplayIndexForFrameKey (state, key) {
-  return getFrameKeysByNonPinnedDisplayIndex(state)
-    .findIndex((displayKey) => displayKey === key)
+function findNonPinnedDisplayIndexForTabId (state, tabId) {
+  return getTabIdsByNonPinnedDisplayIndex(state)
+    .findIndex((displayKey) => displayKey === tabId)
 }
 
 function getFrameByDisplayIndex (state, i) {
@@ -399,8 +399,8 @@ function removeFrame (state, frameProps, framePropsIndex) {
   }
 }
 
-function getFrameTabPageIndex (state, frameKey, tabsPerTabPage = getSetting(settings.TABS_PER_PAGE)) {
-  const index = findNonPinnedDisplayIndexForFrameKey(state, frameKey)
+function getFrameTabPageIndex (state, tabId, tabsPerTabPage = getSetting(settings.TABS_PER_PAGE)) {
+  const index = findNonPinnedDisplayIndexForTabId(state, tabId)
   if (index === -1) {
     return -1
   }
@@ -449,11 +449,12 @@ function isPinned (state, frameKey) {
 
 /**
  * Updates the tab page index to the specified frameProps
- * @param frameProps Any frame belonging to the page
+ * @param state{Object} - Window state
+ * @param tabId{number} - Tab id for the frame
+ * @param tabsPerPage{string} - Current setting for tabs per page, with a default value
  */
-function updateTabPageIndex (state, frameProps) {
-  frameProps = makeImmutable(frameProps)
-  const index = getFrameTabPageIndex(state, frameProps.get('key'))
+function updateTabPageIndex (state, tabId, tabsPerPage = getSetting(settings.TABS_PER_PAGE)) {
+  const index = getFrameTabPageIndex(state, tabId, tabsPerPage)
   const isTabInHoverState = !!getHoverTabIndex(state)
 
   if (index === -1) {
@@ -466,7 +467,6 @@ function updateTabPageIndex (state, frameProps) {
   }
   return state.setIn(['ui', 'tabs', 'tabPageIndex'], index)
 }
-
 const frameStatePath = (state, frameKey) => {
   const index = getFrameIndex(state, frameKey)
   if (index === -1) {
@@ -598,7 +598,7 @@ const setPreviewFrameKey = (state, frameKey, immediate = false) => {
     }
   }
 
-  const index = getFrameTabPageIndex(state, frame)
+  const index = frame ? getFrameTabPageIndex(state, frame.get('tabId')) : -1
   if (index !== -1) {
     if (index !== state.getIn(['ui', 'tabs', 'tabPageIndex'])) {
       state = state.setIn(['ui', 'tabs', 'previewTabPageIndex'], index)

js/stores/windowStore.js

@@ -187,7 +187,7 @@ const newFrame = (state, frameOpts) => {
   if (openInForeground) {
     const tabId = frameOpts.tabId
     const frame = frameStateUtil.getFrameByTabId(state, tabId)
-    state = frameStateUtil.updateTabPageIndex(state, frame)
+    state = frameStateUtil.updateTabPageIndex(state, tabId)
     if (active) {
       // only set the activeFrameKey if the tab is already active
       state = state.set('activeFrameKey', frame.get('key'))
@@ -229,6 +229,18 @@ const frameGuestInstanceIdChanged = (state, action) => {
   })
 }
 
+function handleChangeSettingAction (state, settingKey, settingValue) {
+  switch (settingKey) {
+    case settings.TABS_PER_PAGE:
+      const activeFrame = frameStateUtil.getActiveFrame(state)
+      state = frameStateUtil.updateTabPageIndex(state, activeFrame.get('tabId'), settingValue)
+      break
+    default:
+  }
+
+  return state
+}
+
 const windowStore = new WindowStore()
 const emitChanges = debounce(windowStore.emitChanges.bind(windowStore), 5)
 
@@ -365,7 +377,7 @@ const doAction = (action) => {
         windowState = windowState.setIn(['ui', 'tabs', 'tabPageIndex'], action.index)
         windowState = windowState.deleteIn(['ui', 'tabs', 'previewTabPageIndex'])
       } else {
-        windowState = frameStateUtil.updateTabPageIndex(windowState, action.frameProps)
+        windowState = frameStateUtil.updateTabPageIndex(windowState, action.frameProps.get('tabId'))
       }
       break
     case windowConstants.WINDOW_SET_TAB_BREAKPOINT:
@@ -393,6 +405,7 @@ const doAction = (action) => {
       {
         const sourceFrameProps = frameStateUtil.getFrameByKey(windowState, action.sourceFrameKey)
         const sourceFrameIndex = frameStateUtil.getFrameIndex(windowState, action.sourceFrameKey)
+        const activeFrame = frameStateUtil.getActiveFrame(windowState)
         let newIndex = frameStateUtil.getFrameIndex(windowState, action.destinationFrameKey) + (action.prepend ? 0 : 1)
         let frames = frameStateUtil.getFrames(windowState).splice(sourceFrameIndex, 1)
         if (newIndex > sourceFrameIndex) {
@@ -402,7 +415,7 @@ 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.updateTabPageIndex(windowState, frameStateUtil.getActiveFrame(windowState))
+        windowState = frameStateUtil.updateTabPageIndex(windowState, activeFrame.get('tabId'))
         break
       }
     case windowConstants.WINDOW_SET_LINK_HOVER_PREVIEW:
@@ -704,6 +717,9 @@ const doAction = (action) => {
       }
       windowState = newFrame(windowState, action.frameOpts)
       break
+    case appConstants.APP_CHANGE_SETTING:
+      windowState = handleChangeSettingAction(windowState, action.key, action.value)
+      break
     case windowConstants.WINDOW_FRAME_MOUSE_ENTER:
       windowState = windowState.setIn(['ui', 'mouseInFrame'], true)
       break

test/lib/selectors.js

@@ -72,6 +72,7 @@ module.exports = {
   securityTab: '[data-l10n-id="security"]',
   paymentsTab: '[data-l10n-id="payments"]',
   syncTab: '[data-l10n-id="sync"]',
+  tabsTab: '[data-l10n-id="tabs"]',
   doneButton: '[data-l10n-id="done"]',
   homepageInput: '[data-l10n-id="homepageInput"]',
   compactBraveryPanelSwitch: '[data-test-id="compactBraveryPanelSwitch"] .switchBackground',

test/tab-components/tabPagesTest.js

@@ -12,7 +12,8 @@ const {
   tabPage1,
   tabPage2,
   activeWebview,
-  activeTab
+  activeTab,
+  tabsTab
 } = require('../lib/selectors')
 
 describe('tab pages', function () {
@@ -90,10 +91,21 @@ describe('tab pages', function () {
 
     describe('tabs per page setting', function () {
       it('takes effect immediately', function * () {
-        const defaultTabsPerPage = appConfig.defaultSettings[settings.TABS_PER_PAGE]
+        const newValue = 6
+        const tabs = appConfig.defaultSettings[settings.TABS_PER_PAGE]
+        const numberOfPages = Math.ceil(tabs / newValue)
+        const numberOfTabs = tabs - (Math.floor(tabs / newValue) * newValue)
         yield this.app.client
-          .changeSetting(settings.TABS_PER_PAGE, 1)
-          .waitForElementCount(tabPage, defaultTabsPerPage)
+          .tabByIndex(1)
+          .loadUrl('about:preferences')
+          .waitForVisible(tabsTab)
+          .click(tabsTab)
+          .waitForElementCount('[data-test-id="tabsPerTabPage"]', 1)
+          .click('[data-test-id="tabsPerTabPage"]')
+          .click(`option[value="${newValue}"]`)
+          .waitForBrowserWindow()
+          .waitForElementCount(tabPage, numberOfPages)
+          .waitForElementCount('.tabArea', numberOfTabs)
       })
     })
   })