From d45be64d0a424e3e7f39ce860445ed3da5dd226e Mon Sep 17 00:00:00 2001 From: NejcZdovc Date: Fri, 4 Aug 2017 15:35:42 -0500 Subject: [PATCH] Improves bookmark folder render for context menu Resolves #10054 Auditors: Test Plan: --- app/common/state/contextMenuState.js | 6 + .../common/contextMenu/contextMenu.js | 1 + .../common/contextMenu/contextMenuItem.js | 52 ++++-- app/renderer/reducers/contextMenuReducer.js | 25 ++- js/actions/windowActions.js | 5 +- .../common/contextMenu/contextMenuItemTest.js | 167 ++++++++++++++++++ .../common => js}/contextMenusTest.js | 14 +- 7 files changed, 236 insertions(+), 34 deletions(-) create mode 100644 test/unit/app/renderer/components/common/contextMenu/contextMenuItemTest.js rename test/unit/{app/renderer/components/common => js}/contextMenusTest.js (86%) diff --git a/app/common/state/contextMenuState.js b/app/common/state/contextMenuState.js index d278e679561..31840aa9830 100644 --- a/app/common/state/contextMenuState.js +++ b/app/common/state/contextMenuState.js @@ -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') @@ -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' && diff --git a/app/renderer/components/common/contextMenu/contextMenu.js b/app/renderer/components/common/contextMenu/contextMenu.js index 578752a56df..845c4660838 100644 --- a/app/renderer/components/common/contextMenu/contextMenu.js +++ b/app/renderer/components/common/contextMenu/contextMenu.js @@ -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 */ diff --git a/app/renderer/components/common/contextMenu/contextMenuItem.js b/app/renderer/components/common/contextMenu/contextMenuItem.js index 9e8c28c0626..12203b544af 100644 --- a/app/renderer/components/common/contextMenu/contextMenuItem.js +++ b/app/renderer/components/common/contextMenu/contextMenuItem.js @@ -29,7 +29,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 @@ -44,6 +44,27 @@ class ContextMenuItem extends ImmutableComponent { get hasAccelerator () { return this.accelerator !== null } + + /** + * Get Y position (top) fo the current event (target) + * @param e {event} - event for which we want to determinate top position + * @returns {number} - Y position + */ + getYAxis (e) { + let node = e.target + while (node && !elementHasDataset(node, 'contextMenuItem')) { + node = node.parentNode + } + let parentNode = node.parentNode + while (parentNode && !elementHasDataset(parentNode, '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) { @@ -88,28 +109,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 && !elementHasDataset(node, 'contextMenuItem')) { - node = node.parentNode - } - let parentNode = node.parentNode - while (parentNode && !elementHasDataset(parentNode, '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, 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') diff --git a/app/renderer/reducers/contextMenuReducer.js b/app/renderer/reducers/contextMenuReducer.js index 87fe93f1f63..fd0db84763a 100644 --- a/app/renderer/reducers/contextMenuReducer.js +++ b/app/renderer/reducers/contextMenuReducer.js @@ -464,7 +464,7 @@ const bookmarkItemsInit = (state, items) => { } } if (isFolder) { - templateItem.submenu = showBookmarkFolderInit(state, siteKey) + templateItem.folderKey = siteKey } template.push(templateItem) } @@ -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 } diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index 7b22093df33..23cecea4d75 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -1179,12 +1179,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 }) }, diff --git a/test/unit/app/renderer/components/common/contextMenu/contextMenuItemTest.js b/test/unit/app/renderer/components/common/contextMenu/contextMenuItemTest.js new file mode 100644 index 00000000000..c15710677da --- /dev/null +++ b/test/unit/app/renderer/components/common/contextMenu/contextMenuItemTest.js @@ -0,0 +1,167 @@ +/* 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') + }) + + after(function () { + mockery.disable() + }) + + describe('getYAxis', function () { + let nodeTop, parentTop + + it('target is contextMenuItem and parentNode is contextMenu', function () { + const event = { + target: { + dataset: { + contextMenuItem: true + }, + getBoundingClientRect: () => { + return { + top: nodeTop + } + }, + parentNode: { + dataset: { + contextMenu: true + }, + scrollTop: 1, + getBoundingClientRect: () => { + return { + top: parentTop + } + } + } + } + } + + const wrapper = mount( + {} + })} /> + ) + 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: { + dataset: { + contextMenuItem: true + }, + getBoundingClientRect: () => { + return { + top: nodeTop + } + }, + parentNode: { + dataset: { + class: true + }, + scrollTop: 1, + getBoundingClientRect: () => { + return { + top: 0 + } + }, + parentNode: { + dataset: { + contextMenu: true + }, + scrollTop: 50, + getBoundingClientRect: () => { + return { + top: parentTop + } + } + } + } + } + } + + const wrapper = mount( + {} + })} /> + ) + 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: { + dataset: { + class: true + }, + getBoundingClientRect: () => { + return { + top: 0 + } + }, + parentNode: { + dataset: { + contextMenuItem: true + }, + scrollTop: 1, + getBoundingClientRect: () => { + return { + top: nodeTop + } + }, + parentNode: { + dataset: { + contextMenu: true + }, + scrollTop: 50, + getBoundingClientRect: () => { + return { + top: parentTop + } + } + } + } + } + } + + const wrapper = mount( + {} + })} /> + ) + const instance = wrapper.instance() + nodeTop = 20 + parentTop = 10 + assert.equal(instance.getYAxis(event), 59) + }) + }) +}) diff --git a/test/unit/app/renderer/components/common/contextMenusTest.js b/test/unit/js/contextMenusTest.js similarity index 86% rename from test/unit/app/renderer/components/common/contextMenusTest.js rename to test/unit/js/contextMenusTest.js index 877ccc55fc0..de0b12253ff 100644 --- a/test/unit/app/renderer/components/common/contextMenusTest.js +++ b/test/unit/js/contextMenusTest.js @@ -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 } } @@ -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 () {