Refactors MenuBar and MenuBarItem into redux component

Resolves #8652

Auditors: @bsclifton @bridiver

Test Plan:
- open menu on windows
- try clicking around
- everything should behave as did before

app/common/state/contextMenuState.js

@@ -12,25 +12,34 @@ const validateState = function (state) {
 }
 
 const contextMenuState = {
-  setContextMenu: (state, detail) => {
+  setContextMenu: (windowState, detail) => {
     detail = makeImmutable(detail)
-    state = validateState(state)
+    windowState = validateState(windowState)
 
     if (!detail) {
-      if (state.getIn(['contextMenuDetail', 'type']) === 'hamburgerMenu') {
-        state = state.set('hamburgerMenuWasOpen', true)
+      if (windowState.getIn(['contextMenuDetail', 'type']) === 'hamburgerMenu') {
+        windowState = windowState.set('hamburgerMenuWasOpen', true)
       } else {
-        state = state.set('hamburgerMenuWasOpen', false)
+        windowState = windowState.set('hamburgerMenuWasOpen', false)
       }
-      state = state.delete('contextMenuDetail')
+      windowState = windowState.delete('contextMenuDetail')
     } else {
-      if (!(detail.get('type') === 'hamburgerMenu' && state.get('hamburgerMenuWasOpen'))) {
-        state = state.set('contextMenuDetail', detail)
+      if (!(detail.get('type') === 'hamburgerMenu' && windowState.get('hamburgerMenuWasOpen'))) {
+        windowState = windowState.set('contextMenuDetail', detail)
       }
-      state = state.set('hamburgerMenuWasOpen', false)
+      windowState = windowState.set('hamburgerMenuWasOpen', false)
     }
 
-    return state
+    return windowState
+  },
+
+  selectedIndex: (windowState) => {
+    const selectedIndex = windowState.getIn(['ui', 'contextMenu', 'selectedIndex'])
+    return (typeof selectedIndex === 'object' &&
+      Array.isArray(selectedIndex) &&
+      selectedIndex.length > 0)
+      ? selectedIndex
+      : null
   }
 }
 

app/common/state/menuState.js

@@ -0,0 +1,15 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const {isWindows} = require('../lib/platformUtil')
+const {getSetting} = require('../../../js/settings')
+const settings = require('../../../js/constants/settings')
+
+const api = {
+  isMenuBarVisible: (windowState) => {
+    return isWindows() && (!getSetting(settings.AUTO_HIDE_MENU) || windowState.getIn(['ui', 'menubar', 'isVisible']))
+  }
+}
+
+module.exports = api

app/renderer/components/navigation/menuBar.js

@@ -5,7 +5,7 @@
 const React = require('react')
 
 // Components
-const ImmutableComponent = require('../immutableComponent')
+const ReduxComponent = require('../reduxComponent')
 const MenuBarItem = require('./menuBarItem')
 
 // Constants
@@ -14,6 +14,9 @@ const keyCodes = require('../../../common/constants/keyCodes')
 // Actions
 const windowActions = require('../../../../js/actions/windowActions')
 
+// State
+const contextMenuState = require('../../../common/state/contextMenuState.js')
+
 // Utils
 const {separatorMenuItem} = require('../../../common/commonMenu')
 const {showContextMenu} = require('../../../common/lib/menuUtil')
@@ -24,7 +27,7 @@ const {wrappingClamp} = require('../../../common/lib/formatUtil')
  * First intended use is with Windows to enable a slim titlebar.
  * NOTE: the system menu is still created and used in order to keep the accelerators working.
  */
-class MenuBar extends ImmutableComponent {
+class MenuBar extends React.Component {
   constructor () {
     super()
     this.onKeyDown = this.onKeyDown.bind(this)
@@ -43,11 +46,15 @@ class MenuBar extends ImmutableComponent {
    * Used to position the context menu object.
    */
   getMenubarItemBounds (index) {
-    if (typeof index !== 'number') index = this.props.selectedIndex
+    if (typeof index !== 'number') {
+      index = this.props.selectedIndex
+    }
+
     const selected = document.querySelectorAll('.menubar .menubarItem[data-index=\'' + index + '\']')
     if (selected.length === 1) {
       return selected.item(0).getBoundingClientRect()
     }
+
     return null
   }
 
@@ -112,31 +119,32 @@ class MenuBar extends ImmutableComponent {
     }
   }
 
-  shouldComponentUpdate (nextProps, nextState) {
-    return this.props.selectedIndex !== nextProps.selectedIndex ||
-      this.props.template !== nextProps.template
+  mergeProps (state, dispatchProps, ownProps) {
+    const currentWindow = state.get('currentWindow')
+    const contextMenuDetail = currentWindow.get('contextMenuDetail')
+
+    const props = {}
+    // used in renderer
+    props.template = state.getIn(['menu', 'template'])
+
+    // used in other functions
+    props.selectedIndex = currentWindow.getIn(['ui', 'menubar', 'selectedIndex'])
+    props.contextMenuSelectedIndex = contextMenuState.selectedIndex(currentWindow)
+    props.contextMenuDetail = !!contextMenuDetail
+    props.lastFocusedSelector = currentWindow.getIn(['ui', 'menubar', 'lastFocusedSelector'])
+
+    return Object.assign({}, ownProps, props)
   }
 
   render () {
-    let i = 0
     return <div className='menubar'>
       {
-        this.props.template.map((menubarItem) => {
-          let props = {
-            label: menubarItem.get('label'),
-            index: i++,
-            submenu: menubarItem.get('submenu').toJS(),
-            lastFocusedSelector: this.props.lastFocusedSelector,
-            menubar: this
-          }
-          if (props.index === this.props.selectedIndex) {
-            props.selected = true
-          }
-          return <MenuBarItem {...props} />
+        this.props.template.map((item, i) => {
+          return <MenuBarItem index={i} />
         })
       }
     </div>
   }
 }
 
-module.exports = MenuBar
+module.exports = ReduxComponent.connect(MenuBar)

app/renderer/components/navigation/menuBarItem.js

@@ -5,15 +5,15 @@
 const React = require('react')
 
 // Components
-const ImmutableComponent = require('../immutableComponent')
+const ReduxComponent = require('../reduxComponent')
 
 // Actions
 const windowActions = require('../../../../js/actions/windowActions')
 
 // Utils
 const {showContextMenu} = require('../../../common/lib/menuUtil')
 
-class MenuBarItem extends ImmutableComponent {
+class MenuBarItem extends React.Component {
   constructor () {
     super()
     this.onClick = this.onClick.bind(this)
@@ -24,27 +24,44 @@ class MenuBarItem extends ImmutableComponent {
     if (e && e.stopPropagation) {
       e.stopPropagation()
     }
+
     // If clicking on an already selected item, deselect it
-    const selected = this.props.menubar.props.selectedIndex
-    if (selected && selected === this.props.index) {
+    if (this.props.selected) {
       windowActions.setContextMenuDetail()
       windowActions.setMenuBarSelectedIndex()
       return
     }
+
     // Otherwise, mark item as selected and show its context menu
+    const rect = e.target.getBoundingClientRect()
     windowActions.setMenuBarSelectedIndex(this.props.index)
     windowActions.setContextMenuSelectedIndex()
-    const rect = e.target.getBoundingClientRect()
     showContextMenu(rect, this.props.submenu, this.props.lastFocusedSelector)
   }
 
   onMouseOver (e) {
-    const selected = this.props.menubar.props.selectedIndex
-    if (typeof selected === 'number' && selected !== this.props.index) {
+    if (!this.props.selected) {
       this.onClick(e)
     }
   }
 
+  mergeProps (state, dispatchProps, ownProps) {
+    const currentWindow = state.get('currentWindow')
+    const selectedIndex = currentWindow.getIn(['ui', 'menubar', 'selectedIndex'])
+    const template = state.getIn(['menu', 'template', ownProps.index])
+
+    const props = {}
+    // used in renderer
+    props.selected = ownProps.index === selectedIndex
+    props.label = template.get('label')
+
+    // used in other functions
+    props.submenu = template.get('submenu') && template.get('submenu').toJS()
+    props.lastFocusedSelector = currentWindow.getIn(['ui', 'menubar', 'lastFocusedSelector'])
+
+    return Object.assign({}, ownProps, props)
+  }
+
   render () {
     return <span
       className={'menubarItem' + (this.props.selected ? ' selected' : '')}
@@ -56,4 +73,4 @@ class MenuBarItem extends ImmutableComponent {
   }
 }
 
-module.exports = MenuBarItem
+module.exports = ReduxComponent.connect(MenuBarItem)

app/renderer/components/navigation/navigator.js

@@ -10,7 +10,6 @@ const ipc = electron.ipcRenderer
 // Actions
 const appActions = require('../../../../js/actions/appActions')
 const windowActions = require('../../../../js/actions/windowActions')
-const getSetting = require('../../../../js/settings').getSetting
 
 // Components
 const ImmutableComponent = require('../immutableComponent')
@@ -39,7 +38,6 @@ const cx = require('../../../../js/lib/classSet')
 
 // Constants
 const messages = require('../../../../js/constants/messages')
-const settings = require('../../../../js/constants/settings')
 const appConfig = require('../../../../js/constants/appConfig')
 
 class Navigator extends ImmutableComponent {
@@ -197,7 +195,6 @@ class Navigator extends ImmutableComponent {
     const activeTabShowingMessageBox = !!(activeTab && tabState.isShowingMessageBox(this.props.appState, activeTab.get('tabId')))
     const activeFrame = frameStateUtil.getActiveFrame(this.props.windowState)
     const totalBlocks = activeFrame ? this.getTotalBlocks(activeFrame) : false
-    const contextMenuDetail = this.props.windowState.get('contextMenuDetail')
     const braverySettings = siteSettings.activeSettings(this.props.activeSiteSettings, this.props.appState, appConfig)
     const shieldEnabled = braveShieldsEnabled(activeFrame)
 
@@ -209,13 +206,7 @@ class Navigator extends ImmutableComponent {
         {
           this.props.customTitlebar.menubarVisible
             ? <div className='menubarContainer'>
-              <MenuBar
-                template={this.props.customTitlebar.menubarTemplate}
-                selectedIndex={this.props.customTitlebar.menubarSelectedIndex}
-                contextMenuSelectedIndex={this.props.customTitlebar.contextMenuSelectedIndex}
-                contextMenuDetail={contextMenuDetail}
-                autohide={getSetting(settings.AUTO_HIDE_MENU)}
-                lastFocusedSelector={this.props.customTitlebar.lastFocusedSelector} />
+              <MenuBar />
               <WindowCaptionButtons windowMaximized={this.props.customTitlebar.isMaximized} />
             </div>
             : null

js/components/main.js

@@ -47,7 +47,6 @@ const settings = require('../constants/settings')
 const dragTypes = require('../constants/dragTypes')
 const keyCodes = require('../../app/common/constants/keyCodes')
 const keyLocations = require('../../app/common/constants/keyLocations')
-const isWindows = process.platform === 'win32'
 const {bookmarksToolbarMode} = require('../../app/common/constants/settingsEnums')
 
 // State handling
@@ -59,6 +58,7 @@ const defaultBrowserState = require('../../app/common/state/defaultBrowserState'
 const shieldState = require('../../app/common/state/shieldState')
 const siteSettingsState = require('../../app/common/state/siteSettingsState')
 const tabState = require('../../app/common/state/tabState')
+const menuState = require('../../app/common/state/menuState')
 
 // Util
 const _ = require('underscore')
@@ -67,7 +67,7 @@ const eventUtil = require('../lib/eventUtil')
 const siteSettings = require('../state/siteSettings')
 const debounce = require('../lib/debounce')
 const {getCurrentWindowId, isMaximized, isFocused, isFullScreen} = require('../../app/renderer/currentWindow')
-const platformUtil = require('../../app/common/lib/platformUtil')
+const {isDarwin, isWindows} = require('../../app/common/lib/platformUtil')
 
 class Main extends ImmutableComponent {
   constructor () {
@@ -93,14 +93,13 @@ class Main extends ImmutableComponent {
   }
   registerWindowLevelShortcuts () {
     // For window level shortcuts that don't work as local shortcuts
-    const isDarwin = platformUtil.isDarwin()
     document.addEventListener('keydown', (e) => {
       switch (e.which) {
         case keyCodes.ESC:
           this.exitFullScreen()
           break
         case keyCodes.F12:
-          if (!isDarwin) {
+          if (!isDarwin()) {
             ipc.emit(messages.SHORTCUT_ACTIVE_FRAME_TOGGLE_DEV_TOOLS)
           }
           break
@@ -662,13 +661,11 @@ class Main extends ImmutableComponent {
   }
 
   get customTitlebar () {
-    const customTitlebarEnabled = isWindows
-    const captionButtonsVisible = customTitlebarEnabled
-    const menubarVisible = customTitlebarEnabled && (!getSetting(settings.AUTO_HIDE_MENU) || this.props.windowState.getIn(['ui', 'menubar', 'isVisible']))
+    const menubarVisible = menuState.isMenuBarVisible(this.props.windowState)
     const selectedIndex = this.props.windowState.getIn(['ui', 'contextMenu', 'selectedIndex'])
     return {
-      enabled: customTitlebarEnabled,
-      captionButtonsVisible: captionButtonsVisible,
+      enabled: isWindows(),
+      captionButtonsVisible: isWindows(),
       menubarVisible: menubarVisible,
       menubarTemplate: menubarVisible ? this.props.appState.getIn(['menu', 'template']) : null,
       menubarSelectedIndex: this.props.windowState.getIn(['ui', 'menubar', 'selectedIndex']),
@@ -869,7 +866,7 @@ class Main extends ImmutableComponent {
             draggingOverData={this.props.appState.getIn(['dragData', 'dragOverData', 'draggingOverType']) === dragTypes.BOOKMARK && this.props.appState.getIn(['dragData', 'dragOverData'])}
             showFavicon={showFavicon}
             showOnlyFavicon={showOnlyFavicon}
-            shouldAllowWindowDrag={shouldAllowWindowDrag && !isWindows}
+            shouldAllowWindowDrag={shouldAllowWindowDrag && !isWindows()}
             activeFrameKey={(activeFrame && activeFrame.get('key')) || undefined}
             windowWidth={window.innerWidth}
             contextMenuDetail={contextMenuDetail}

test/unit/app/common/state/contextMenuStateTest.js

@@ -0,0 +1,35 @@
+/* global describe, it */
+const Immutable = require('immutable')
+const contextMenuState = require('../../../../../app/common/state/contextMenuState.js')
+const assert = require('chai').assert
+
+const defaultWindowState = Immutable.fromJS({
+  ui: {
+    contextMenu: {
+      selectedIndex: null
+    }
+  }
+})
+
+describe('contextMenuState', function () {
+  describe('selectedIndex', function () {
+    it('returns null if selectedIndex is not set', function () {
+      const result = contextMenuState.selectedIndex(defaultWindowState)
+      assert.equal(result, null)
+    })
+
+    it('returns null if selectedIndex is only number', function () {
+      const index = 0
+      const newState = defaultWindowState.setIn(['ui', 'contextMenu', 'selectedIndex'], index)
+      const result = contextMenuState.selectedIndex(newState)
+      assert.equal(result, null)
+    })
+
+    it('returns array of selected index', function () {
+      const index = [0]
+      const newState = defaultWindowState.setIn(['ui', 'contextMenu', 'selectedIndex'], index)
+      const result = contextMenuState.selectedIndex(newState)
+      assert.equal(result, index)
+    })
+  })
+})