This repository has been archived by the owner on Dec 11, 2019. It is now read-only.
Prevent duplicate separators for all context menu items #5869
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Includes all content PLUS a fix for a bug found in the original PR #5846 (which was reverted with #5867) This PR fixes #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)
|
Gonna check back in here tomorrow- hoping Travis CI loves the tests this go-around 😄 |
bsclifton
commented
Nov 28, 2016
| const expectedResult = [{label: 'lol'}] | ||
| assert.deepEqual(result, expectedResult) | ||
| }) | ||
| it('always returns an array (even for one item)', function () { |
There was a problem hiding this comment.
This last test here is the one I added which failed (revealing the bug). The Array.reduce that I was doing would returning a non-array when only 1 entry exists (it now properly returns an array)
bsclifton
commented
Nov 28, 2016
| return Array.isArray(result) | ||
| ? result | ||
| : [result] | ||
| } |
There was a problem hiding this comment.
Here is the fix for the bug which led to the revert (which happened with #5867)
|
thanks! |
This was referenced Dec 21, 2016
|
I cleared the label and the milestone as this has been superseded with #6374 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
git rebase -ito squash commits (if needed).Includes all content PLUS a fix for a bug found in the original PR #5846
(which was reverted with #5867)
This PR fixes #5765
Auditors: @bbondy
Includes these changes
sanitizeTemplateItemsmethod to filter out bad entriesmenuUtilfromapp/browser=>app/commontest/unit/app, instead oftest/unit/lib)menuUtiland items incontextMenu.jsto reinforce distinction between template items and electron menu items (the compiled template)Automated test plan
npm run uitest -- --grep="bookmarksToolbar"(this was the one which had a failing test which I missed)npm run unittest -- --grep="sanitizeTemplateItems"Manual test plan