Refactors MenuBar and MenuBarItem into redux component

Resolves brave#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/menuBarState.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 tabState = require('../../app/common/state/tabState')
 const siteSettingsState = require('../../app/common/state/siteSettingsState')
+const menuBarState = require('../../app/common/state/menuBarState')
 
 // 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
@@ -653,13 +652,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 = menuBarState.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']),
@@ -858,7 +855,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)
+ })
+ })
+})