From 72a71a582600d4f93b26e4087a8d74df3f5c295d Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Mon, 28 Nov 2016 00:38:43 -0700 Subject: [PATCH] Prevent duplicate separators for all context menu items Includes all content PLUS a fix for a bug found in the original PR https://github.com/brave/browser-laptop/pull/5846 (which was reverted with https://github.com/brave/browser-laptop/pull/5867) This PR fixes https://github.com/brave/browser-laptop/issues/5765 Auditors: @bbondy Includes these changes: - Added new `sanitizeTemplateItems` method to filter out bad entries - Moved `menuUtil` from `app/browser` => `app/common` - Moved some tests to match the new directory structure (ex: `test/unit/app`, instead of `test/unit/lib`) - Rename template functions in `menuUtil` and items in `contextMenu.js` to reinforce distinction between **template** items and **electron menu** items (the compiled template) - **The new sanitize method is called in each method which builds a context menu** Automated test plan: - `npm run uitest -- --grep="bookmarksToolbar"` - `npm run unittest -- --grep="sanitizeTemplateItems"` Manual test plan: - Launch Brave on Windows or Linux - Right click on each of the about pages and ensure there are no double separators (might be easy to go to about:about to launch each) - Try other context menus - Right clicking on a tab - right clicking on page - right clicking a bookmark / bookmark folder in the bookmarks toolbar - right clicking inside the downloads bar (shown after downloading an item) --- app/browser/menu.js | 6 +- app/common/lib/historyUtil.js | 2 +- app/{browser => common}/lib/menuUtil.js | 52 ++++++++- js/contextMenus.js | 110 +++++++++--------- .../{ => app/common}/lib/formatUtilTest.js | 4 +- .../{ => app/common}/lib/historyUtilTest.js | 5 +- .../unit/{ => app/common}/lib/httpUtilTest.js | 4 +- .../{ => app/common}/lib/ledgerUtilTest.js | 4 +- .../unit/{ => app/common}/lib/menuUtilTest.js | 69 ++++++++--- .../{ => app/common}/lib/platformUtilTest.js | 8 +- .../common/state/aboutHistoryStateTest.js | 2 +- .../common/state/aboutNewTabStateTest.js | 4 +- .../common/state/basicAuthStateTest.js | 4 +- .../common/state/extensionStateTest.js | 2 +- .../{ => app}/common/state/tabStateTest.js | 2 +- 15 files changed, 179 insertions(+), 99 deletions(-) rename app/{browser => common}/lib/menuUtil.js (76%) rename test/unit/{ => app/common}/lib/formatUtilTest.js (96%) rename test/unit/{ => app/common}/lib/historyUtilTest.js (97%) rename test/unit/{ => app/common}/lib/httpUtilTest.js (95%) rename test/unit/{ => app/common}/lib/ledgerUtilTest.js (95%) rename test/unit/{ => app/common}/lib/menuUtilTest.js (71%) rename test/unit/{ => app/common}/lib/platformUtilTest.js (90%) rename test/unit/{ => app}/common/state/aboutHistoryStateTest.js (92%) rename test/unit/{ => app}/common/state/aboutNewTabStateTest.js (97%) rename test/unit/{ => app}/common/state/basicAuthStateTest.js (97%) rename test/unit/{ => app}/common/state/extensionStateTest.js (99%) rename test/unit/{ => app}/common/state/tabStateTest.js (99%) diff --git a/app/browser/menu.js b/app/browser/menu.js index 293d97c226c..2a4866af3c0 100644 --- a/app/browser/menu.js +++ b/app/browser/menu.js @@ -20,7 +20,7 @@ const siteTags = require('../../js/constants/siteTags') const dialog = electron.dialog const BrowserWindow = electron.BrowserWindow const {fileUrl} = require('../../js/lib/appUrlUtil') -const menuUtil = require('./lib/menuUtil') +const menuUtil = require('../common/lib/menuUtil') const getSetting = require('../../js/settings').getSetting const locale = require('../locale') const {isSiteBookmarked} = require('../../js/state/siteUtil') @@ -329,7 +329,7 @@ const createHistorySubmenu = () => { } } ] - submenu = submenu.concat(menuUtil.createRecentlyClosedMenuItems(Immutable.fromJS(Object.keys(closedFrames).map(key => closedFrames[key])))) + submenu = submenu.concat(menuUtil.createRecentlyClosedTemplateItems(Immutable.fromJS(Object.keys(closedFrames).map(key => closedFrames[key])))) submenu.push( // TODO: recently visited @@ -374,7 +374,7 @@ const createBookmarksSubmenu = () => { CommonMenu.importBrowserDataMenuItem() ] - const bookmarks = menuUtil.createBookmarkMenuItems(appStore.getState().get('sites')) + const bookmarks = menuUtil.createBookmarkTemplateItems(appStore.getState().get('sites')) if (bookmarks.length > 0) { submenu.push(CommonMenu.separatorMenuItem) submenu = submenu.concat(bookmarks) diff --git a/app/common/lib/historyUtil.js b/app/common/lib/historyUtil.js index 108590b073f..e42e76c8821 100644 --- a/app/common/lib/historyUtil.js +++ b/app/common/lib/historyUtil.js @@ -34,7 +34,7 @@ module.exports.groupEntriesByDay = (history, locale) => { const reduced = history.reduce((previousValue, currentValue, currentIndex, array) => { const result = currentIndex === 1 ? [] : previousValue if (currentIndex === 1) { - const firstDate = getDayString(currentValue, locale) + const firstDate = getDayString(previousValue, locale) result.push({date: firstDate, entries: [previousValue]}) } const date = getDayString(currentValue, locale) diff --git a/app/browser/lib/menuUtil.js b/app/common/lib/menuUtil.js similarity index 76% rename from app/browser/lib/menuUtil.js rename to app/common/lib/menuUtil.js index cc2f7068d06..09d5418a610 100644 --- a/app/browser/lib/menuUtil.js +++ b/app/common/lib/menuUtil.js @@ -69,7 +69,7 @@ module.exports.setTemplateItemChecked = (template, label, checked) => { return null } -const createBookmarkMenuItems = (bookmarks, parentFolderId) => { +const createBookmarkTemplateItems = (bookmarks, parentFolderId) => { const filteredBookmarks = parentFolderId ? bookmarks.filter((bookmark) => bookmark.get('parentFolderId') === parentFolderId) : bookmarks.filter((bookmark) => !bookmark.get('parentFolderId')) @@ -99,7 +99,7 @@ const createBookmarkMenuItems = (bookmarks, parentFolderId) => { const submenuItems = bookmarks.filter((bookmark) => bookmark.get('parentFolderId') === folderId) payload.push({ label: site.get('customTitle') || site.get('title'), - submenu: submenuItems.count() > 0 ? createBookmarkMenuItems(bookmarks, folderId) : null + submenu: submenuItems.count() > 0 ? createBookmarkTemplateItems(bookmarks, folderId) : null }) } }) @@ -107,15 +107,18 @@ const createBookmarkMenuItems = (bookmarks, parentFolderId) => { } /** - * Add bookmarks / folders to "Bookmarks" menu + * Used to create bookmarks and bookmark folder entries for the "Bookmarks" menu * * @param sites The application state's Immutable sites list */ -module.exports.createBookmarkMenuItems = (sites) => { - return createBookmarkMenuItems(siteUtil.getBookmarks(sites)) +module.exports.createBookmarkTemplateItems = (sites) => { + return createBookmarkTemplateItems(siteUtil.getBookmarks(sites)) } -module.exports.createRecentlyClosedMenuItems = (lastClosedFrames) => { +/** + * Create "recently closed" history entries for the "History" menu + */ +module.exports.createRecentlyClosedTemplateItems = (lastClosedFrames) => { const payload = [] if (lastClosedFrames && lastClosedFrames.size > 0) { payload.push( @@ -141,3 +144,40 @@ module.exports.createRecentlyClosedMenuItems = (lastClosedFrames) => { } return payload } + +const isItemValid = (currentItem, previousItem) => { + if (previousItem && previousItem === CommonMenu.separatorMenuItem) { + if (currentItem === CommonMenu.separatorMenuItem) { + return false + } + } + return currentItem && (typeof currentItem.label === 'string' || typeof currentItem.type === 'string') +} + +/** + * Remove invalid entries from a menu template: + * - null or falsey entries + * - extra menu separators + * - entries which don't have a label or type + */ +module.exports.sanitizeTemplateItems = (template) => { + const result = template.reduce((previousValue, currentValue, currentIndex, array) => { + const result = currentIndex === 1 ? [] : previousValue + if (currentIndex === 1) { + if (isItemValid(previousValue)) { + result.push(previousValue) + } + } + const previousItem = result.length > 0 + ? result[result.length - 1] + : undefined + if (isItemValid(currentValue, previousItem)) { + result.push(currentValue) + } + return result + }) + + return Array.isArray(result) + ? result + : [result] +} diff --git a/js/contextMenus.js b/js/contextMenus.js index badbc862909..7dd5d5fe106 100644 --- a/js/contextMenus.js +++ b/js/contextMenus.js @@ -19,6 +19,7 @@ const siteTags = require('./constants/siteTags') const dragTypes = require('./constants/dragTypes') const siteUtil = require('./state/siteUtil') const downloadUtil = require('./state/downloadUtil') +const menuUtil = require('../app/common/lib/menuUtil') const CommonMenu = require('../app/common/commonMenu') const dnd = require('./dnd') const dndData = require('./dndData') @@ -124,79 +125,79 @@ function findBarTemplateInit () { } function tabsToolbarTemplateInit (activeFrame, closestDestinationDetail, isParent) { - const menu = [ + const template = [ CommonMenu.bookmarksManagerMenuItem(), CommonMenu.bookmarksToolbarMenuItem(), CommonMenu.separatorMenuItem ] if (!isDarwin) { - menu.push(CommonMenu.autoHideMenuBarMenuItem(), + template.push(CommonMenu.autoHideMenuBarMenuItem(), CommonMenu.separatorMenuItem) } - menu.push(addBookmarkMenuItem('addBookmark', siteUtil.getDetailFromFrame(activeFrame, siteTags.BOOKMARK), closestDestinationDetail, isParent), + template.push(addBookmarkMenuItem('addBookmark', siteUtil.getDetailFromFrame(activeFrame, siteTags.BOOKMARK), closestDestinationDetail, isParent), addFolderMenuItem(closestDestinationDetail, isParent)) - return menu + return menuUtil.sanitizeTemplateItems(template) } function downloadsToolbarTemplateInit (downloadId, downloadItem) { - const menu = [] + const template = [] if (downloadItem) { const downloads = appStoreRenderer.state.get('downloads') if (downloadUtil.shouldAllowPause(downloadItem)) { - menu.push({ + template.push({ label: locale.translation('downloadItemPause'), click: downloadActions.pauseDownload.bind(null, downloadId) }) } if (downloadUtil.shouldAllowResume(downloadItem)) { - menu.push({ + template.push({ label: locale.translation('downloadItemResume'), click: downloadActions.resumeDownload.bind(null, downloadId) }) } if (downloadUtil.shouldAllowCancel(downloadItem)) { - menu.push({ + template.push({ label: locale.translation('downloadItemCancel'), click: downloadActions.cancelDownload.bind(null, downloadId) }) } if (downloadUtil.shouldAllowRedownload(downloadItem)) { - menu.push({ + template.push({ label: locale.translation('downloadItemRedownload'), click: downloadActions.redownloadURL.bind(null, downloadItem, downloadId) }) } if (downloadUtil.shouldAllowCopyLink(downloadItem)) { - menu.push({ + template.push({ label: locale.translation('downloadItemCopyLink'), click: downloadActions.copyLinkToClipboard.bind(null, downloadItem) }) } if (downloadUtil.shouldAllowOpenDownloadLocation(downloadItem)) { - menu.push({ + template.push({ label: locale.translation('downloadItemPath'), click: downloadActions.locateShellPath.bind(null, downloadItem) }) } if (downloadUtil.shouldAllowDelete(downloadItem)) { - menu.push({ + template.push({ label: locale.translation('downloadItemDelete'), click: downloadActions.deleteDownload.bind(null, downloads, downloadItem, downloadId) }) } if (downloadUtil.shouldAllowRemoveFromList(downloadItem)) { - menu.push({ + template.push({ label: locale.translation('downloadItemClear'), click: downloadActions.clearDownload.bind(null, downloads, downloadId) }) @@ -204,26 +205,26 @@ function downloadsToolbarTemplateInit (downloadId, downloadItem) { } if (windowStore.getState().getIn(['ui', 'downloadsToolbar', 'isVisible'])) { - if (menu.length) { - menu.push(CommonMenu.separatorMenuItem) + if (template.length) { + template.push(CommonMenu.separatorMenuItem) } - menu.push({ + template.push({ label: locale.translation('downloadToolbarHide'), click: () => { windowActions.setDownloadsToolbarVisible(false) } }) } - if (menu.length) { - menu.push(CommonMenu.separatorMenuItem) + if (template.length) { + template.push(CommonMenu.separatorMenuItem) } - menu.push({ + template.push({ label: locale.translation('downloadItemClearCompleted'), click: () => { appActions.clearCompletedDownloads() } }) - return menu + return menuUtil.sanitizeTemplateItems(template) } function siteDetailTemplateInit (siteDetail, activeFrame) { @@ -331,7 +332,7 @@ function siteDetailTemplateInit (siteDetail, activeFrame) { addFolderMenuItem(siteDetail, true)) } - return template + return menuUtil.sanitizeTemplateItems(template) } function showBookmarkFolderInit (allBookmarkItems, parentBookmarkFolder, activeFrame) { @@ -359,7 +360,7 @@ function showBookmarkFolderInit (allBookmarkItems, parentBookmarkFolder, activeF function bookmarkItemsInit (allBookmarkItems, items, activeFrame) { const btbMode = getSetting(settings.BOOKMARKS_TOOLBAR_MODE) const showFavicon = (btbMode === bookmarksToolbarMode.TEXT_AND_FAVICONS || btbMode === bookmarksToolbarMode.FAVICONS_ONLY) - return items.map((site) => { + const template = items.map((site) => { const isFolder = siteUtil.isFolder(site) let faIcon if (showFavicon && !site.get('favicon')) { @@ -400,6 +401,7 @@ function bookmarkItemsInit (allBookmarkItems, items, activeFrame) { } return templateItem }).toJS() + return menuUtil.sanitizeTemplateItems(template) } function moreBookmarksTemplateInit (allBookmarkItems, bookmarks, activeFrame) { @@ -411,14 +413,14 @@ function moreBookmarksTemplateInit (allBookmarkItems, bookmarks, activeFrame) { windowActions.setContextMenuDetail() } }) - return template + return menuUtil.sanitizeTemplateItems(template) } function usernameTemplateInit (usernames, origin, action) { - let items = [] + let template = [] for (let username in usernames) { let password = usernames[username] - items.push({ + template.push({ label: username, click: (item, focusedWindow) => { windowActions.setActiveFrameShortcut(null, messages.FILL_PASSWORD, { @@ -431,11 +433,11 @@ function usernameTemplateInit (usernames, origin, action) { } }) } - return items + return menuUtil.sanitizeTemplateItems(template) } function autofillTemplateInit (suggestions, frame) { - const items = [] + const template = [] for (let i = 0; i < suggestions.length; ++i) { let value const frontendId = suggestions[i].frontend_id @@ -449,9 +451,9 @@ function autofillTemplateInit (suggestions, frame) { value = 'Autofill Settings' } if (frontendId === -3) { // POPUP_ITEM_ID_SEPARATOR - items.push(CommonMenu.separatorMenuItem) + template.push(CommonMenu.separatorMenuItem) } else { - items.push({ + template.push({ label: value, click: (item, focusedWindow) => { ipc.send('autofill-selection-clicked', frame.get('tabId'), value, frontendId, i) @@ -460,15 +462,15 @@ function autofillTemplateInit (suggestions, frame) { }) } } - return items + return menuUtil.sanitizeTemplateItems(template) } function tabTemplateInit (frameProps) { const frameKey = frameProps.get('key') - const items = [CommonMenu.newTabMenuItem(frameProps.get('key'))] + const template = [CommonMenu.newTabMenuItem(frameProps.get('key'))] const location = frameProps.get('location') if (location !== 'about:newtab') { - items.push( + template.push( CommonMenu.separatorMenuItem, { label: locale.translation('reloadTab'), @@ -492,7 +494,7 @@ function tabTemplateInit (frameProps) { if (!frameProps.get('isPrivate')) { const isPinned = frameProps.get('pinnedLocation') if (!(location === 'about:blank' || location === 'about:newtab' || isIntermediateAboutPage(location))) { - items.push({ + template.push({ label: locale.translation(isPinned ? 'unpinTab' : 'pinTab'), click: (item) => { // Handle converting the current tab window into a pinned site @@ -502,7 +504,7 @@ function tabTemplateInit (frameProps) { } } - // items.push({ + // template.push({ // label: locale.translation('moveTabToNewWindow'), // enabled: false, // click: (item, focusedWindow) => { @@ -510,7 +512,7 @@ function tabTemplateInit (frameProps) { // } // }) - items.push(CommonMenu.separatorMenuItem, + template.push(CommonMenu.separatorMenuItem, { label: locale.translation('muteOtherTabs'), click: (item, focusedWindow) => { @@ -521,7 +523,7 @@ function tabTemplateInit (frameProps) { if (frameProps.get('audioPlaybackActive')) { const isMuted = frameProps.get('audioMuted') - items.push({ + template.push({ label: locale.translation(isMuted ? 'unmuteTab' : 'muteTab'), click: (item) => { windowActions.setAudioMuted(frameProps, !isMuted) @@ -529,10 +531,10 @@ function tabTemplateInit (frameProps) { }) } - items.push(CommonMenu.separatorMenuItem) + template.push(CommonMenu.separatorMenuItem) if (!frameProps.get('pinnedLocation')) { - items.push({ + template.push({ label: locale.translation('closeTab'), click: (item, focusedWindow) => { if (focusedWindow) { @@ -543,7 +545,7 @@ function tabTemplateInit (frameProps) { }) } - items.push({ + template.push({ label: locale.translation('closeOtherTabs'), click: (item, focusedWindow) => { if (focusedWindow) { @@ -566,22 +568,22 @@ function tabTemplateInit (frameProps) { } }, CommonMenu.separatorMenuItem) - items.push(Object.assign({}, + template.push(Object.assign({}, CommonMenu.reopenLastClosedTabItem(), { enabled: windowStore.getState().get('closedFrames').size > 0 } )) - return items + return menuUtil.sanitizeTemplateItems(template) } function getMisspelledSuggestions (selection, isMisspelled, suggestions) { const hasSelection = selection.length > 0 - const items = [] + const template = [] if (hasSelection) { if (suggestions.length > 0) { // Map the first 3 suggestions to menu items that allows click // to replace the text. - items.push(...suggestions.slice(0, 3).map((suggestion) => { + template.push(...suggestions.slice(0, 3).map((suggestion) => { return { label: suggestion, click: () => { @@ -591,7 +593,7 @@ function getMisspelledSuggestions (selection, isMisspelled, suggestions) { }), CommonMenu.separatorMenuItem) } if (isMisspelled) { - items.push({ + template.push({ label: locale.translation('learnSpelling'), click: () => { appActions.addWord(selection, true) @@ -608,16 +610,16 @@ function getMisspelledSuggestions (selection, isMisspelled, suggestions) { }, CommonMenu.separatorMenuItem) } } - return items + return menuUtil.sanitizeTemplateItems(template) } function getEditableItems (selection, editFlags) { const hasSelection = selection.length > 0 const hasClipboard = clipboard.readText().length > 0 - const items = [] + const template = [] if (!editFlags || editFlags.canCut) { - items.push({ + template.push({ label: locale.translation('cut'), enabled: hasSelection, accelerator: 'CmdOrCtrl+X', @@ -625,7 +627,7 @@ function getEditableItems (selection, editFlags) { }) } if (!editFlags || editFlags.canCopy) { - items.push({ + template.push({ label: locale.translation('copy'), enabled: hasSelection, accelerator: 'CmdOrCtrl+C', @@ -633,14 +635,14 @@ function getEditableItems (selection, editFlags) { }) } if (!editFlags || editFlags.canPaste) { - items.push({ + template.push({ label: locale.translation('paste'), accelerator: 'CmdOrCtrl+V', enabled: hasClipboard, role: 'paste' }) } - return items + return menuUtil.sanitizeTemplateItems(template) } function hamburgerTemplateInit (location, e) { @@ -683,7 +685,7 @@ function hamburgerTemplateInit (location, e) { }, { label: locale.translation('bravery'), submenu: [ -// CommonMenu.braveryGlobalMenuItem(), + // CommonMenu.braveryGlobalMenuItem(), CommonMenu.braverySiteMenuItem(), CommonMenu.braveryPaymentsMenuItem() ] @@ -707,7 +709,7 @@ function hamburgerTemplateInit (location, e) { }, CommonMenu.quitMenuItem() ] - return template + return menuUtil.sanitizeTemplateItems(template) } const openInNewTabMenuItem = (location, isPrivate, partitionNumber, parentFrameKey) => { @@ -890,7 +892,7 @@ function mainTemplateInit (nodeProps, frame) { } }) } - return template + return menuUtil.sanitizeTemplateItems(template) } const isLink = nodeProps.linkURL && nodeProps.linkURL !== '' @@ -1211,7 +1213,7 @@ function mainTemplateInit (nodeProps, frame) { ) } - return template + return menuUtil.sanitizeTemplateItems(template) } function onHamburgerMenu (location, e) { diff --git a/test/unit/lib/formatUtilTest.js b/test/unit/app/common/lib/formatUtilTest.js similarity index 96% rename from test/unit/lib/formatUtilTest.js rename to test/unit/app/common/lib/formatUtilTest.js index 7d9d404e5b6..ad995e78000 100644 --- a/test/unit/lib/formatUtilTest.js +++ b/test/unit/app/common/lib/formatUtilTest.js @@ -1,8 +1,8 @@ /* global describe, before, after, it */ -const formatUtil = require('../../../app/common/lib/formatUtil') +const formatUtil = require('../../../../../app/common/lib/formatUtil') const assert = require('assert') -require('../braveUnit') +require('../../../braveUnit') describe('formatUtil', function () { describe('formatAccelerator', function () { diff --git a/test/unit/lib/historyUtilTest.js b/test/unit/app/common/lib/historyUtilTest.js similarity index 97% rename from test/unit/lib/historyUtilTest.js rename to test/unit/app/common/lib/historyUtilTest.js index bcd6f484faf..dc39ec646f2 100644 --- a/test/unit/lib/historyUtilTest.js +++ b/test/unit/app/common/lib/historyUtilTest.js @@ -1,8 +1,9 @@ /* global describe, it */ const assert = require('assert') const Immutable = require('immutable') -const historyUtil = require('../../../app/common/lib/historyUtil') -require('../braveUnit') +const historyUtil = require('../../../../../app/common/lib/historyUtil') + +require('../../../braveUnit') describe('historyUtil', function () { const historyDayOne = Immutable.fromJS({ diff --git a/test/unit/lib/httpUtilTest.js b/test/unit/app/common/lib/httpUtilTest.js similarity index 95% rename from test/unit/lib/httpUtilTest.js rename to test/unit/app/common/lib/httpUtilTest.js index 7b1fe40299d..824d200b530 100644 --- a/test/unit/lib/httpUtilTest.js +++ b/test/unit/app/common/lib/httpUtilTest.js @@ -1,8 +1,8 @@ /* global describe, it */ -const httpUtil = require('../../../app/common/lib/httpUtil') +const httpUtil = require('../../../../../app/common/lib/httpUtil') const assert = require('assert') -require('../braveUnit') +require('../../../braveUnit') describe('httpUtil test', function () { describe('responseHasContent', function () { diff --git a/test/unit/lib/ledgerUtilTest.js b/test/unit/app/common/lib/ledgerUtilTest.js similarity index 95% rename from test/unit/lib/ledgerUtilTest.js rename to test/unit/app/common/lib/ledgerUtilTest.js index 96fdc376c89..df643fb74ba 100644 --- a/test/unit/lib/ledgerUtilTest.js +++ b/test/unit/app/common/lib/ledgerUtilTest.js @@ -1,8 +1,8 @@ /* global describe, it */ -const ledgerUtil = require('../../../app/common/lib/ledgerUtil') +const ledgerUtil = require('../../../../../app/common/lib/ledgerUtil') const assert = require('assert') -require('../braveUnit') +require('../../../braveUnit') describe('ledgerUtil test', function () { describe('shouldTrackView', function () { diff --git a/test/unit/lib/menuUtilTest.js b/test/unit/app/common/lib/menuUtilTest.js similarity index 71% rename from test/unit/lib/menuUtilTest.js rename to test/unit/app/common/lib/menuUtilTest.js index e7f3f82f6b4..3acf59d62d9 100644 --- a/test/unit/lib/menuUtilTest.js +++ b/test/unit/app/common/lib/menuUtilTest.js @@ -1,12 +1,14 @@ /* global describe, before, after, it */ -const siteTags = require('../../../js/constants/siteTags') +const siteTags = require('../../../../../js/constants/siteTags') const mockery = require('mockery') const assert = require('assert') const Immutable = require('immutable') -require('../braveUnit') -describe('menuUtil', function () { +require('../../../braveUnit') + +describe('menuUtil tests', function () { let menuUtil + let separator before(function () { mockery.enable({ @@ -14,8 +16,10 @@ describe('menuUtil', function () { warnOnUnregistered: false, useCleanCache: true }) - mockery.registerMock('electron', require('./fakeElectron')) - menuUtil = require('../../../app/browser/lib/menuUtil') + // This is required; commonMenu is included by menuUtil (and references electron) + mockery.registerMock('electron', require('../../../lib/fakeElectron')) + menuUtil = require('../../../../../app/common/lib/menuUtil') + separator = require('../../../../../app/common/commonMenu').separatorMenuItem }) after(function () { @@ -116,13 +120,13 @@ describe('menuUtil', function () { }) }) - describe('createBookmarkMenuItems', function () { + describe('createBookmarkTemplateItems', function () { it('returns an array of items w/ the bookmark tag', function () { const appStateSites = Immutable.fromJS([ { tags: [siteTags.BOOKMARK], title: 'my website', location: 'https://brave.com' } ]) - const menuItems = menuUtil.createBookmarkMenuItems(appStateSites) + const menuItems = menuUtil.createBookmarkTemplateItems(appStateSites) assert.equal(Array.isArray(menuItems), true) assert.equal(menuItems.length, 1) @@ -133,7 +137,7 @@ describe('menuUtil', function () { { tags: [siteTags.BOOKMARK], customTitle: 'use this', title: 'not this', location: 'https://brave.com' } ]) - const menuItems = menuUtil.createBookmarkMenuItems(appStateSites) + const menuItems = menuUtil.createBookmarkTemplateItems(appStateSites) assert.equal(menuItems[0].label, 'use this') }) @@ -144,7 +148,7 @@ describe('menuUtil', function () { ] }) - const menuItems = menuUtil.createBookmarkMenuItems(appStateSites) + const menuItems = menuUtil.createBookmarkTemplateItems(appStateSites) assert.deepEqual(menuItems, []) }) @@ -155,7 +159,7 @@ describe('menuUtil', function () { ] }) - const menuItems = menuUtil.createBookmarkMenuItems(appStateSites) + const menuItems = menuUtil.createBookmarkTemplateItems(appStateSites) assert.deepEqual(menuItems, []) }) @@ -164,7 +168,7 @@ describe('menuUtil', function () { { tags: [siteTags.PINNED], title: 'pinned site', location: 'https://pinned-website.com' }, { tags: [siteTags.BOOKMARK], title: 'my website', location: 'https://brave.com' } ]) - const menuItems = menuUtil.createBookmarkMenuItems(appStateSites) + const menuItems = menuUtil.createBookmarkTemplateItems(appStateSites) assert.equal(menuItems.length, 1) assert.equal(menuItems[0].label, 'my website') @@ -174,7 +178,7 @@ describe('menuUtil', function () { { tags: [siteTags.BOOKMARK_FOLDER], title: 'my folder', folderId: 123 }, { tags: [siteTags.BOOKMARK], title: 'my website', location: 'https://brave.com', parentFolderId: 123 } ]) - const menuItems = menuUtil.createBookmarkMenuItems(appStateSites) + const menuItems = menuUtil.createBookmarkTemplateItems(appStateSites) assert.equal(menuItems.length, 1) assert.equal(menuItems[0].label, 'my folder') @@ -187,20 +191,20 @@ describe('menuUtil', function () { { tags: [siteTags.BOOKMARK], title: 'my website', location: 'https://brave.com', parentFolderId: 123 } ]) - const menuItems = menuUtil.createBookmarkMenuItems(appStateSites) + const menuItems = menuUtil.createBookmarkTemplateItems(appStateSites) assert.equal(menuItems.length, 1) assert.equal(menuItems[0].label, 'use this') }) }) - describe('createRecentlyClosedMenuItems', function () { + describe('createRecentlyClosedTemplateItems', function () { it('returns an array of closedFrames preceded by a separator and "Recently Closed" items', function () { const windowStateClosedFrames = Immutable.fromJS([{ title: 'sample', location: 'https://brave.com' }]) - const menuItems = menuUtil.createRecentlyClosedMenuItems(windowStateClosedFrames) + const menuItems = menuUtil.createRecentlyClosedTemplateItems(windowStateClosedFrames) assert.equal(Array.isArray(menuItems), true) assert.equal(menuItems.length, 3) @@ -224,11 +228,44 @@ describe('menuUtil', function () { { title: 'site10', location: 'https://brave10.com' }, { title: 'site11', location: 'https://brave11.com' } ]) - const menuItems = menuUtil.createRecentlyClosedMenuItems(windowStateClosedFrames) + const menuItems = menuUtil.createRecentlyClosedTemplateItems(windowStateClosedFrames) assert.equal(menuItems.length, 12) assert.equal(menuItems[2].label, windowStateClosedFrames.get(1).get('title')) assert.equal(menuItems[11].label, windowStateClosedFrames.get(10).get('title')) }) }) + + describe('sanitizeTemplateItems', function () { + it('removes entries which are falsey', function () { + const template = [null, undefined, false, {label: 'lol'}] + const result = menuUtil.sanitizeTemplateItems(template) + const expectedResult = [{label: 'lol'}] + assert.deepEqual(result, expectedResult) + }) + it('removes duplicate menu separators', function () { + const template = [separator, separator, {label: 'lol'}] + const result = menuUtil.sanitizeTemplateItems(template) + const expectedResult = [separator, {label: 'lol'}] + assert.deepEqual(result, expectedResult) + }) + it('removes items which are missing label or type', function () { + const template = [{}, {test: 'test'}, {label: 'lol'}] + const result = menuUtil.sanitizeTemplateItems(template) + const expectedResult = [{label: 'lol'}] + assert.deepEqual(result, expectedResult) + }) + it('removes items which have non-string values for label or type', function () { + const template = [{label: true}, {type: function () { console.log('test') }}, {label: 'lol'}] + const result = menuUtil.sanitizeTemplateItems(template) + const expectedResult = [{label: 'lol'}] + assert.deepEqual(result, expectedResult) + }) + it('always returns an array (even for one item)', function () { + const template = [{label: 'lol'}] + const result = menuUtil.sanitizeTemplateItems(template) + const expectedResult = [{label: 'lol'}] + assert.deepEqual(result, expectedResult) + }) + }) }) diff --git a/test/unit/lib/platformUtilTest.js b/test/unit/app/common/lib/platformUtilTest.js similarity index 90% rename from test/unit/lib/platformUtilTest.js rename to test/unit/app/common/lib/platformUtilTest.js index f241a6d0e22..b6bffc2ff55 100644 --- a/test/unit/lib/platformUtilTest.js +++ b/test/unit/app/common/lib/platformUtilTest.js @@ -1,9 +1,9 @@ /* global describe, it */ const mockery = require('mockery') const assert = require('assert') -let platformUtil = require('../../../app/common/lib/platformUtil') +let platformUtil = require('../../../../../app/common/lib/platformUtil') -require('../braveUnit') +require('../../../braveUnit') describe('platformUtil', function () { const mockWin7 = { @@ -26,11 +26,11 @@ describe('platformUtil', function () { const loadMocks = (osMock) => { mockery.enable({ warnOnReplace: false, warnOnUnregistered: false, useCleanCache: true }) mockery.registerMock('os', osMock) - platformUtil = require('../../../app/common/lib/platformUtil') + platformUtil = require('../../../../../app/common/lib/platformUtil') } const unloadMocks = () => { mockery.disable() - platformUtil = require('../../../app/common/lib/platformUtil') + platformUtil = require('../../../../../app/common/lib/platformUtil') } describe('getPlatformStyles', function () { diff --git a/test/unit/common/state/aboutHistoryStateTest.js b/test/unit/app/common/state/aboutHistoryStateTest.js similarity index 92% rename from test/unit/common/state/aboutHistoryStateTest.js rename to test/unit/app/common/state/aboutHistoryStateTest.js index eb1f13f13fe..ac40ae6c82f 100644 --- a/test/unit/common/state/aboutHistoryStateTest.js +++ b/test/unit/app/common/state/aboutHistoryStateTest.js @@ -1,5 +1,5 @@ /* global describe, it */ -const aboutHistoryState = require('../../../../app/common/state/aboutHistoryState') +const aboutHistoryState = require('../../../../../app/common/state/aboutHistoryState') const Immutable = require('immutable') const assert = require('assert') diff --git a/test/unit/common/state/aboutNewTabStateTest.js b/test/unit/app/common/state/aboutNewTabStateTest.js similarity index 97% rename from test/unit/common/state/aboutNewTabStateTest.js rename to test/unit/app/common/state/aboutNewTabStateTest.js index c2d2a489b66..9349f47907a 100644 --- a/test/unit/common/state/aboutNewTabStateTest.js +++ b/test/unit/app/common/state/aboutNewTabStateTest.js @@ -1,8 +1,8 @@ /* global describe, it */ -const aboutNewTabState = require('../../../../app/common/state/aboutNewTabState') +const aboutNewTabState = require('../../../../../app/common/state/aboutNewTabState') const Immutable = require('immutable') const assert = require('assert') -const siteTags = require('../../../../js/constants/siteTags') +const siteTags = require('../../../../../js/constants/siteTags') const defaultAppState = Immutable.fromJS({ about: { diff --git a/test/unit/common/state/basicAuthStateTest.js b/test/unit/app/common/state/basicAuthStateTest.js similarity index 97% rename from test/unit/common/state/basicAuthStateTest.js rename to test/unit/app/common/state/basicAuthStateTest.js index 21b07203594..df354f2cf71 100644 --- a/test/unit/common/state/basicAuthStateTest.js +++ b/test/unit/app/common/state/basicAuthStateTest.js @@ -1,6 +1,6 @@ /* global describe, it, before */ -const basicAuthState = require('../../../../app/common/state/basicAuthState') -const tabState = require('../../../../app/common/state/tabState') +const basicAuthState = require('../../../../../app/common/state/basicAuthState') +const tabState = require('../../../../../app/common/state/tabState') const Immutable = require('immutable') const assert = require('assert') diff --git a/test/unit/common/state/extensionStateTest.js b/test/unit/app/common/state/extensionStateTest.js similarity index 99% rename from test/unit/common/state/extensionStateTest.js rename to test/unit/app/common/state/extensionStateTest.js index a15ee503e3a..4a263faa54c 100644 --- a/test/unit/common/state/extensionStateTest.js +++ b/test/unit/app/common/state/extensionStateTest.js @@ -1,5 +1,5 @@ /* global describe, it, before */ -const extensionState = require('../../../../app/common/state/extensionState') +const extensionState = require('../../../../../app/common/state/extensionState') const Immutable = require('immutable') const assert = require('assert') diff --git a/test/unit/common/state/tabStateTest.js b/test/unit/app/common/state/tabStateTest.js similarity index 99% rename from test/unit/common/state/tabStateTest.js rename to test/unit/app/common/state/tabStateTest.js index cc6b0653326..2daf1b33182 100644 --- a/test/unit/common/state/tabStateTest.js +++ b/test/unit/app/common/state/tabStateTest.js @@ -1,5 +1,5 @@ /* global describe, it, before */ -const tabState = require('../../../../app/common/state/tabState') +const tabState = require('../../../../../app/common/state/tabState') const Immutable = require('immutable') const assert = require('assert')