Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

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

Merged
merged 3 commits into from
Oct 10, 2016
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fixing bookmarks toolbar "hover after click" behavior on folders
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
bsclifton committed Oct 7, 2016

Verified

This commit was signed with the committer’s verified signature.
commit 1ede4e9bcf2255b624c108122e8631ff7ec1a605
10 changes: 6 additions & 4 deletions js/lib/faviconUtil.js → app/common/lib/faviconUtil.js
Original file line number Diff line number Diff line change
@@ -2,16 +2,18 @@
* 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 {isSourceAboutUrl} = require('./appUrlUtil')
const UrlUtil = require('./urlutil')
const { isSourceAboutUrl } = require('../../../js/lib/appUrlUtil')
const UrlUtil = require('../../../js/lib/urlutil')

module.exports = function getFavicon (frameProps, iconHref) {
module.exports.iconSize = 16

module.exports.getFavicon = (frameProps, iconHref) => {
return new Promise((resolve, reject) => {
if (!frameProps.get('location')) {
resolve(null)
}

const size = window.devicePixelRatio * 16
const size = window.devicePixelRatio * module.exports.iconSize
const resolution = '#-moz-resolution=' + size + ',' + size

// Default to favicon.ico if we can't find an icon.
Original file line number Diff line number Diff line change
@@ -4,26 +4,27 @@

const React = require('react')
const ReactDOM = require('react-dom')
const ImmutableComponent = require('./immutableComponent')
const contextMenus = require('../contextMenus')
const windowActions = require('../actions/windowActions')
const bookmarkActions = require('../actions/bookmarkActions')
const appActions = require('../actions/appActions')
const siteTags = require('../constants/siteTags')
const siteUtil = require('../state/siteUtil')
const dragTypes = require('../constants/dragTypes')
const Button = require('../components/button')
const cx = require('../lib/classSet')
const dnd = require('../dnd')
const dndData = require('../dndData')
const calculateTextWidth = require('../lib/textCalculator').calculateTextWidth
const windowStore = require('../stores/windowStore')
const iconSize = 16
const ImmutableComponent = require('../../../js/components/immutableComponent')
const contextMenus = require('../../../js/contextMenus')
const windowActions = require('../../../js/actions/windowActions')
const bookmarkActions = require('../../../js/actions/bookmarkActions')
const appActions = require('../../../js/actions/appActions')
const siteTags = require('../../../js/constants/siteTags')
const siteUtil = require('../../../js/state/siteUtil')
const dragTypes = require('../../../js/constants/dragTypes')
const Button = require('../../../js/components/button')
const cx = require('../../../js/lib/classSet')
const dnd = require('../../../js/dnd')
const dndData = require('../../../js/dndData')
const calculateTextWidth = require('../../../js/lib/textCalculator').calculateTextWidth
const windowStore = require('../../../js/stores/windowStore')
const iconSize = require('../../common/lib/faviconUtil').iconSize

class BookmarkToolbarButton extends ImmutableComponent {
constructor () {
super()
this.onClick = this.onClick.bind(this)
this.onMouseOver = this.onMouseOver.bind(this)
this.onDragStart = this.onDragStart.bind(this)
this.onDragEnd = this.onDragEnd.bind(this)
this.onDragEnter = this.onDragEnter.bind(this)
@@ -47,6 +48,21 @@ class BookmarkToolbarButton extends ImmutableComponent {
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. It's more inline with how flux is intended to work. We should have declarative actions instead of imperative
  2. 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

Copy link
Member Author

@bsclifton bsclifton Sep 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bridiver:

  1. 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
  2. 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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented feedback- ready for re-review @bridiver 😄

if (this.isFolder && this.props.selectedFolderId !== this.props.bookmark.get('folderId')) {
// Auto-expand the menu if user mouses over another folder
e.target = ReactDOM.findDOMNode(this)
this.props.showBookmarkFolderMenu(this.props.bookmark, e)
} else if (!this.isFolder && this.props.selectedFolderId !== -1) {
// Hide the currently expanded menu if user mouses over a non-folder
windowActions.setBookmarksToolbarSelectedFolderId(-1)
windowActions.setContextMenuDetail()
}
}
}

onDragStart (e) {
dnd.onDragStart(dragTypes.BOOKMARK, this.props.bookmark, e)
}
@@ -168,6 +184,7 @@ class BookmarkToolbarButton extends ImmutableComponent {
ref={(node) => { this.bookmarkNode = node }}
title={hoverTitle}
onClick={this.onClick}
onMouseOver={this.onMouseOver}
onDragStart={this.onDragStart}
onDragEnd={this.onDragEnd}
onDragEnter={this.onDragEnter}
@@ -271,6 +288,7 @@ class BookmarksToolbar extends ImmutableComponent {
return bookmarkActions.clickBookmarkItem(this.bookmarks, bookmark, this.activeFrame, e)
}
showBookmarkFolderMenu (bookmark, e) {
windowActions.setBookmarksToolbarSelectedFolderId(bookmark.get('folderId'))
contextMenus.onShowBookmarkFolderMenu(this.bookmarks, bookmark, this.activeFrame, e)
}
updateBookmarkData (props) {
@@ -394,7 +412,8 @@ class BookmarksToolbar extends ImmutableComponent {
showBookmarkFolderMenu={this.showBookmarkFolderMenu}
bookmark={bookmark}
showFavicon={this.props.showFavicon}
showOnlyFavicon={this.props.showOnlyFavicon} />)
showOnlyFavicon={this.props.showOnlyFavicon}
selectedFolderId={this.props.selectedFolderId} />)
}
{
this.overflowBookmarkItems.size !== 0
3 changes: 3 additions & 0 deletions docs/state.md
Original file line number Diff line number Diff line change
@@ -358,6 +358,9 @@ WindowStore
isVisible: boolean, // true if Menubar control is visible
selectedIndex: Array<number>, // indices of the selected menu item(s) (or null for none selected)
lastFocusedSelector: string // selector for the last selected element (browser ui, not frame content)
},
bookmarksToolbar: {
selectedFolderId: number // folderId from the siteDetail of the currently expanded folder
}
},
searchDetail: {
7 changes: 7 additions & 0 deletions js/actions/windowActions.js
Original file line number Diff line number Diff line change
@@ -1121,6 +1121,13 @@ const windowActions = {
tabId,
details
})
},

setBookmarksToolbarSelectedFolderId: function (folderId) {
dispatch({
actionType: WindowConstants.WINDOW_SET_BOOKMARKS_TOOLBAR_SELECTED_FOLDER_ID,
folderId
})
}
}

5 changes: 3 additions & 2 deletions js/components/main.js
Original file line number Diff line number Diff line change
@@ -35,7 +35,7 @@ const AutofillCreditCardPanel = require('./autofillCreditCardPanel')
const AddEditBookmark = require('./addEditBookmark')
const LoginRequired = require('./loginRequired')
const ReleaseNotes = require('./releaseNotes')
const BookmarksToolbar = require('./bookmarksToolbar')
const BookmarksToolbar = require('../../app/renderer/components/bookmarksToolbar')
const ContextMenu = require('./contextMenu')
const PopupWindow = require('./popupWindow')
const NoScriptInfo = require('./noScriptInfo')
@@ -1027,7 +1027,8 @@ class Main extends ImmutableComponent {
activeFrameKey={activeFrame && activeFrame.get('key') || undefined}
windowWidth={window.innerWidth}
contextMenuDetail={this.props.windowState.get('contextMenuDetail')}
sites={this.props.appState.get('sites')} />
sites={this.props.appState.get('sites')}
selectedFolderId={this.props.windowState.getIn(['ui', 'bookmarksToolbar', 'selectedFolderId'])} />
: null
}
<div className={cx({
3 changes: 2 additions & 1 deletion js/constants/windowConstants.js
Original file line number Diff line number Diff line change
@@ -77,7 +77,8 @@ const windowConstants = {
WINDOW_RESET_MENU_STATE: _,
WINDOW_SET_SUBMENU_SELECTED_INDEX: _,
WINDOW_SET_LAST_FOCUSED_SELECTOR: _,
WINDOW_GOT_RESPONSE_DETAILS: _
WINDOW_GOT_RESPONSE_DETAILS: _,
WINDOW_SET_BOOKMARKS_TOOLBAR_SELECTED_FOLDER_ID: _
}

module.exports = mapValuesByKeys(windowConstants)
4 changes: 4 additions & 0 deletions js/stores/windowStore.js
Original file line number Diff line number Diff line change
@@ -818,6 +818,7 @@ const doAction = (action) => {
doAction({actionType: WindowConstants.WINDOW_SET_CONTEXT_MENU_DETAIL})
}
doAction({actionType: WindowConstants.WINDOW_SET_SUBMENU_SELECTED_INDEX})
doAction({actionType: WindowConstants.WINDOW_SET_BOOKMARKS_TOOLBAR_SELECTED_FOLDER_ID})
break
case WindowConstants.WINDOW_SET_SUBMENU_SELECTED_INDEX:
windowState = windowState.setIn(['ui', 'menubar', 'selectedIndex'],
@@ -828,6 +829,9 @@ const doAction = (action) => {
case WindowConstants.WINDOW_SET_LAST_FOCUSED_SELECTOR:
windowState = windowState.setIn(['ui', 'menubar', 'lastFocusedSelector'], action.selector)
break
case WindowConstants.WINDOW_SET_BOOKMARKS_TOOLBAR_SELECTED_FOLDER_ID:
windowState = windowState.setIn(['ui', 'bookmarksToolbar', 'selectedFolderId'], action.folderId)
break

default:
}