Skip to content

Commit

Permalink
Improves bookmark folder render for context menu
Browse files Browse the repository at this point in the history
Resolves brave#10054

Auditors:

Test Plan:
  • Loading branch information
NejcZdovc committed Aug 9, 2017
1 parent 796b967 commit 85be36d
Show file tree
Hide file tree
Showing 7 changed files with 217 additions and 34 deletions.
6 changes: 6 additions & 0 deletions app/common/state/contextMenuState.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* 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 Immutable = require('immutable')
const assert = require('assert')
const { makeImmutable, isMap } = require('./immutableUtil')

Expand Down Expand Up @@ -33,6 +34,11 @@ const api = {
return windowState
},

getContextMenu: (windowState) => {
windowState = validateState(windowState)
return windowState.get('contextMenuDetail', Immutable.Map())
},

selectedIndex: (windowState) => {
const selectedIndex = windowState.getIn(['ui', 'contextMenu', 'selectedIndex'])
return (typeof selectedIndex === 'object' &&
Expand Down
1 change: 1 addition & 0 deletions app/renderer/components/common/contextMenu/contextMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const cx = require('../../../../../js/lib/classSet')
const frameStateUtil = require('../../../../../js/state/frameStateUtil')
const {separatorMenuItem} = require('../../../../common/commonMenu')
const {wrappingClamp} = require('../../../../common/lib/formatUtil')

/**
* Represents a context menu including all submenus
*/
Expand Down
48 changes: 29 additions & 19 deletions app/renderer/components/common/contextMenu/contextMenuItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class ContextMenuItem extends ImmutableComponent {
return this.props.contextMenuItem.get('items')
}
get hasSubmenu () {
return this.submenu && this.submenu.size > 0
return (this.submenu && this.submenu.size > 0) || this.props.contextMenuItem.has('folderKey')
}
get isMulti () {
return this.items && this.items.size > 0
Expand All @@ -43,6 +43,23 @@ class ContextMenuItem extends ImmutableComponent {
get hasAccelerator () {
return this.accelerator !== null
}

getYAxis (e) {
let node = e.target
// TODO we shouldn't be relaying on classList
while (node && node.classList && !node.classList.contains('contextMenuItem')) {
node = node.parentNode
}
let parentNode = node.parentNode
while (parentNode && parentNode.classList && !parentNode.classList.contains('contextMenu')) {
parentNode = parentNode.parentNode
}
const parentBoundingRect = parentNode.getBoundingClientRect()
const boundingRect = node.getBoundingClientRect()

return boundingRect.top - parentBoundingRect.top - 1 + parentNode.scrollTop
}

onClick (clickAction, shouldHide, e) {
e.stopPropagation()
if (clickAction) {
Expand Down Expand Up @@ -87,28 +104,21 @@ class ContextMenuItem extends ImmutableComponent {
}

onMouseEnter (e) {
let openedSubmenuDetails = this.props.contextMenuDetail.get('openedSubmenuDetails')
openedSubmenuDetails = openedSubmenuDetails
? openedSubmenuDetails.splice(this.props.submenuIndex, this.props.contextMenuDetail.get('openedSubmenuDetails').size)
: new Immutable.List()

if (this.hasSubmenu) {
let node = e.target
while (node && node.classList && !node.classList.contains('contextMenuItem')) {
node = node.parentNode
}
let parentNode = node.parentNode
while (parentNode && parentNode.classList && !parentNode.classList.contains('contextMenu')) {
parentNode = parentNode.parentNode
}
const parentBoundingRect = parentNode.getBoundingClientRect()
const boundingRect = node.getBoundingClientRect()
if (this.props.contextMenuItem.has('folderKey')) {
const yAxis = this.getYAxis(e)
windowActions.onShowBookmarkFolderMenu(this.props.contextMenuItem.get('folderKey'), yAxis, null, this.props.submenuIndex)
} else if (this.hasSubmenu) {
let openedSubmenuDetails = this.props.contextMenuDetail.get('openedSubmenuDetails')
openedSubmenuDetails = openedSubmenuDetails
? openedSubmenuDetails.splice(this.props.submenuIndex, this.props.contextMenuDetail.get('openedSubmenuDetails').size)
: new Immutable.List()
const yAxis = this.getYAxis(e)
openedSubmenuDetails = openedSubmenuDetails.push(Immutable.fromJS({
y: boundingRect.top - parentBoundingRect.top - 1 + parentNode.scrollTop,
y: yAxis,
template: this.submenu
}))
windowActions.setContextMenuDetail(this.props.contextMenuDetail.set('openedSubmenuDetails', openedSubmenuDetails))
}
windowActions.setContextMenuDetail(this.props.contextMenuDetail.set('openedSubmenuDetails', openedSubmenuDetails))
}
getLabelForItem (item) {
const label = item.get('label')
Expand Down
25 changes: 19 additions & 6 deletions app/renderer/reducers/contextMenuReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ const bookmarkItemsInit = (state, items) => {
}
}
if (isFolder) {
templateItem.submenu = showBookmarkFolderInit(state, siteKey)
templateItem.folderKey = siteKey
}
template.push(templateItem)
}
Expand Down Expand Up @@ -509,11 +509,24 @@ const onShowBookmarkFolderMenu = (state, action) => {
state = validateState(state)

const menuTemplate = showBookmarkFolderInit(state, action.get('bookmarkKey'))
state = contextMenuState.setContextMenu(state, makeImmutable({
left: action.get('left'),
top: action.get('top'),
template: menuTemplate
}))
if (action.get('submenuIndex') != null) {
let contextMenu = contextMenuState.getContextMenu(state)
let openedSubmenuDetails = contextMenu.get('openedSubmenuDetails', Immutable.List())

openedSubmenuDetails = openedSubmenuDetails
.splice(action.get('submenuIndex'), openedSubmenuDetails.size)
.push(makeImmutable({
y: action.get('left'),
template: menuTemplate
}))
state = contextMenuState.setContextMenu(state, contextMenu.set('openedSubmenuDetails', openedSubmenuDetails))
} else {
state = contextMenuState.setContextMenu(state, makeImmutable({
left: action.get('left'),
top: action.get('top'),
template: menuTemplate
}))
}

return state
}
Expand Down
5 changes: 3 additions & 2 deletions js/actions/windowActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -1230,12 +1230,13 @@ const windowActions = {
})
},

onShowBookmarkFolderMenu: function (bookmarkKey, left, top) {
onShowBookmarkFolderMenu: function (bookmarkKey, left, top, submenuIndex) {
dispatch({
actionType: windowConstants.WINDOW_ON_SHOW_BOOKMARK_FOLDER_MENU,
bookmarkKey,
left,
top
top,
submenuIndex
})
},

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
/* 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/. */
/* global describe, before, after, it */

const Immutable = require('immutable')
const mockery = require('mockery')
const assert = require('assert')
require('../../../../../braveUnit')
const {mount} = require('enzyme')

describe('ContextMenuItem unit test', function () {
let ContextMenuItem

before(function () {
mockery.enable({
warnOnReplace: false,
warnOnUnregistered: false,
useCleanCache: true
})
ContextMenuItem = require('../../../../../../../app/renderer/components/common/contextMenu/contextMenuItem')
Array.prototype.contains = function (item) { return this.includes(item) } // eslint-disable-line
})

after(function () {
mockery.disable()
})

describe('getYAxis', function () {
let nodeTop, parentTop

it('target is contextMenuItem and parentNode is contextMenu', function () {
const event = {
target: {
classList: ['contextMenuItem'],
getBoundingClientRect: () => {
return {
top: nodeTop
}
},
parentNode: {
classList: ['contextMenu'],
scrollTop: 1,
getBoundingClientRect: () => {
return {
top: parentTop
}
}
}
}
}

const wrapper = mount(
<ContextMenuItem contextMenuItem={Immutable.fromJS({
label: 'New Tab',
accelerator: 'CmdOrCtrl+T',
click: () => {}
})} />
)
const instance = wrapper.instance()
nodeTop = 1
parentTop = 0
assert.equal(instance.getYAxis(event), 1)
})

it('target is contextMenuItem and parentNode is second contextMenu', function () {
const event = {
target: {
classList: ['contextMenuItem'],
getBoundingClientRect: () => {
return {
top: nodeTop
}
},
parentNode: {
classList: ['class'],
scrollTop: 1,
getBoundingClientRect: () => {
return {
top: 0
}
},
parentNode: {
classList: ['contextMenu'],
scrollTop: 50,
getBoundingClientRect: () => {
return {
top: parentTop
}
}
}
}
}
}

const wrapper = mount(
<ContextMenuItem contextMenuItem={Immutable.fromJS({
label: 'New Tab',
accelerator: 'CmdOrCtrl+T',
click: () => {}
})} />
)
const instance = wrapper.instance()
nodeTop = 1
parentTop = 10
assert.equal(instance.getYAxis(event), 40)
})

it('target is second contextMenuItem and parentNode is second contextMenu', function () {
const event = {
target: {
classList: ['class'],
getBoundingClientRect: () => {
return {
top: 0
}
},
parentNode: {
classList: ['contextMenuItem'],
scrollTop: 1,
getBoundingClientRect: () => {
return {
top: nodeTop
}
},
parentNode: {
classList: ['contextMenu'],
scrollTop: 50,
getBoundingClientRect: () => {
return {
top: parentTop
}
}
}
}
}
}

const wrapper = mount(
<ContextMenuItem contextMenuItem={Immutable.fromJS({
label: 'New Tab',
accelerator: 'CmdOrCtrl+T',
click: () => {}
})} />
)
const instance = wrapper.instance()
nodeTop = 20
parentTop = 10
assert.equal(instance.getYAxis(event), 59)
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@
const mockery = require('mockery')
const assert = require('assert')
const sinon = require('sinon')
const fakeElectron = require('../../../../lib/fakeElectron')
let fakeElectronMenu
let contextMenus
require('../../../../braveUnit')
const fakeElectron = require('../lib/fakeElectron')
require('../braveUnit')

describe('ContextMenus unit tests', function () {
let fakeElectronMenu, contextMenus

describe('Context menu module unit tests', function () {
const fakeLocale = {
translation: (token) => { return token }
}
Expand All @@ -24,8 +24,8 @@ describe('Context menu module unit tests', function () {
})
mockery.registerMock('electron', fakeElectron)
mockery.registerMock('../js/l10n', fakeLocale)
contextMenus = require('../../../../../../js/contextMenus')
fakeElectronMenu = require('../../../../lib/fakeElectronMenu')
contextMenus = require('../../../js/contextMenus')
fakeElectronMenu = require('../lib/fakeElectronMenu')
})

after(function () {
Expand Down

0 comments on commit 85be36d

Please sign in to comment.