Fixing bookmarks toolbar "hover after click" behavior on folders #4361

bsclifton · 2016-09-28T18:10:14Z

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

When context menu is expanded for a bookmark folder, mousing over other folders will auto-expand them (closing your previous folder's context menu).
Matching behavior in Chrome, mousing over non-folders will hide the menu.

Fixes #1725

Also includes:

Added constant to faviconUtils for default icon size (referenced from bookmarksToolbar component)
Moved bookmarksToolbar component and faviconUtils to new directory structure

Auditors: @bridiver

Test Plan

Launch Brave and ensure you have bookmarks toolbar showing with at least 2 folders and 1 non-folder
Click one of the folders in bookmarks toolbar and confirm it opens
Mouse over the other folder and confirm it auto-expands the menu
Mouse over a non-folder (ex: a bookmark) and confirm it hides the menu
Mouse back over a folder and confirm it auto-expands it again
Click anywhere to dismiss the context menu
Drag a bookmark over a bookmark folder and confirm it auto-expands. For bonus points, you can try dragging to a folder inside a folder.

bridiver · 2016-09-28T18:25:40Z

app/renderer/components/bookmarksToolbar.js

@@ -47,6 +48,21 @@ class BookmarkToolbarButton extends ImmutableComponent {
    }
  }

+  onMouseOver (e) {
+    // Behavior when a bookmarks toolbar folder has its list expanded
+    if (this.props.selectedFolderId) {


I think that ideally this should trigger something like an onMouseOverBookmarkFolder action with whatever params are needed to allow the windowstore (browser process eventually) to update the state rather than having the logic here to open context menus and update state. The are two reasons for this:

It's more inline with how flux is intended to work. We should have declarative actions instead of imperative

We want to take node out of the main window renderers so we want to move electron api calls out of the react components (showBookmarkFolderMenu indirectly calls electron.remote methods) in preparation for moving them to the browser process

@bridiver:

I can update the code to do this. It would be a larger change and would put "business logic" into the WindowStore. For example, if the selection changes from null or -1 to a valid number, the WindowStore would know it needs to create a ContextMenu renderer control, which I guess it would handle by doing another windowAction? Maybe we can talk this out more

This particular method (showBookmarkFolderMenu) doesn't use electron.remote. However, there are methods in that file (contextMenus.js) which do (specifically for creating / showing popup menu templates)

I was looking at this.props.showBookmarkFolderMenu which at first looked like it called the electron api directly, but it calls setContextMenuDetails which eventually triggers a call the remote api. The windowStore really should directly care about any of this, the logic itself should be in a "reducer" similar extensionState. In the flux/redux model that is exactly where the logic is supposed to go. The UI should just fire off events and react to state changes. We have a bit of code to untangle because actions shouldn't call other actions, but some of the actions are "setters" and aren't written in a way that makes it easy to reuse the login in them.

Implemented feedback- ready for re-review @bridiver 😄

darkdh · 2016-09-29T03:23:04Z

test/components/bookmarksToolbarTest.js

+  //       .keys('ArrowDown')
+  //       .keys('ArrowDown')
+  //       .keys('ArrowDown')
+  //       .keys('Enter')


You can try .click('.contextMenuItemText=Add Folder')

@darkdh thanks for checking this out- I did end up looking at it more and unfortunately, the menu that pops up is a system popup (not one of our custom rendered context menus, like autofill 😞). I'll remove the commented out tests for now

Follow up- I did find a way to test adding bookmark folders (see commits)

@bbondy

When context menu is expanded for a bookmark folder, mousing over other folders will auto-expand them (closing your previous folder's context menu). Matching behavior in Chrome, mousing over non-folders will hide the menu. Fixes #1725 Also includes: - Added constant to faviconUtils for default icon size (referenced from bookmarksToolbar component) - Moved bookmarksToolbar component and faviconUtils to new directory structure Auditors: @bbondy @darkdh Test Plan: 1. Launch Brave and ensure you have bookmarks toolbar showing with at least 2 folders and 1 non-folder 2. Click one of the folders in bookmarks toolbar and confirm it opens 3. Mouse over the other folder and confirm it auto-expands the menu 4. Mouse over a non-folder (ex: a bookmark) and confirm it hides the menu 5. Mouse back over a folder and confirm it auto-expands it again 6. Click anywhere to dismiss the context menu

@bridiver

…ew functionality - Some cleanup, per comments, cc: @bridiver

@bridiver

Auditors: @bridiver

bsclifton · 2016-10-07T20:17:02Z

Chatting w/ ya on Slack now 😄

The sites have to be passed (for now) since they come from the appState. FolderId & bookmark are needed (for now- the template methods would need to be updated)
I ran into weird issues w/ passing windowState and assigning. I think it has to do with the emit. Will definitely have to revisit this... @bridiver, I'll check this out in a follow-up

bsclifton · 2016-10-07T21:24:48Z

If @bridiver gives the big thumbs up, let's merge this 😄 Assigning to 0.12.5...

bridiver · 2016-10-10T17:30:40Z

++

bsclifton added this to the 0.12.4dev milestone Sep 28, 2016

bridiver reviewed Sep 28, 2016

View reviewed changes

luixxiul added the feature/bookmarks label Sep 28, 2016

darkdh reviewed Sep 29, 2016

View reviewed changes

bbondy removed this from the 0.12.4dev milestone Oct 4, 2016

bsclifton added 3 commits October 7, 2016 12:51

- Added new bookmarksToolbars test file with tests for existing and n…

39c7c80

…ew functionality - Some cleanup, per comments, cc: @bridiver

Cleanup part duex, per conversation w/ @bridiver :)

6f0336a

Auditors: @bridiver

bsclifton added this to the 0.12.5dev milestone Oct 7, 2016

bsclifton assigned bridiver Oct 8, 2016

bsclifton added the QA/test-plan-specified label Oct 8, 2016

bridiver merged commit ff68e16 into brave:master Oct 10, 2016

bsclifton mentioned this pull request Oct 11, 2016

Fix autofill regression caused by refactor #4669

Closed

alexwykoff added QA/checked-macOS QA/checked-Win64 labels Oct 13, 2016

This was referenced Oct 14, 2016

Manual tests for Ubuntu 0.12.5 RC1 #4772

Closed

Manual tests for OS X 0.12.5 RC1 #4773

Closed

Manual tests for Windows ia-32 0.12.5 RC 1 #4774

Closed

Manual tests for Windows x64 0.12.5 RC 1 #4775

Closed

luixxiul added the QA/checked-Win32 label Oct 15, 2016

This was referenced Oct 16, 2016

Manual tests for Windows x64 0.12.5 RC 3 #4838

Closed

Manual tests for OS X 0.12.5 RC3 #4839

Closed

Manual tests for Windows ia-32 0.12.5 RC3 #4840

Closed

Manual tests for Ubuntu 0.12.5 RC3 #4841

Closed

bsclifton deleted the bookmarks-toolbar-click branch October 17, 2016 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing bookmarks toolbar "hover after click" behavior on folders #4361

Fixing bookmarks toolbar "hover after click" behavior on folders #4361

bsclifton commented Sep 28, 2016 •

edited

Loading

bridiver Sep 28, 2016

bsclifton Sep 28, 2016 •

edited

Loading

bridiver Sep 28, 2016

bsclifton Oct 4, 2016

darkdh Sep 29, 2016

bsclifton Sep 29, 2016

bsclifton Oct 4, 2016 •

edited

Loading

bsclifton commented Oct 7, 2016 •

edited

Loading

bsclifton commented Oct 7, 2016

bridiver commented Oct 10, 2016

Fixing bookmarks toolbar "hover after click" behavior on folders #4361

Fixing bookmarks toolbar "hover after click" behavior on folders #4361

Conversation

bsclifton commented Sep 28, 2016 • edited Loading

Test Plan

bridiver Sep 28, 2016

Choose a reason for hiding this comment

bsclifton Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

bridiver Sep 28, 2016

Choose a reason for hiding this comment

bsclifton Oct 4, 2016

Choose a reason for hiding this comment

darkdh Sep 29, 2016

Choose a reason for hiding this comment

bsclifton Sep 29, 2016

Choose a reason for hiding this comment

bsclifton Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

bsclifton commented Oct 7, 2016 • edited Loading

bsclifton commented Oct 7, 2016

bridiver commented Oct 10, 2016

bsclifton commented Sep 28, 2016 •

edited

Loading

bsclifton Sep 28, 2016 •

edited

Loading

bsclifton Oct 4, 2016 •

edited

Loading

bsclifton commented Oct 7, 2016 •

edited

Loading