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

Commit

Permalink
Fix middle click support
Browse files Browse the repository at this point in the history
Fix #7144

Fix #7185

Auditors: @bsclifton

auxclick is a new event in C55 and up, click is no longer fired for non primary buttons.  Unfortunately React doens't have support yet so I had to do it via componentDidUpdate and access to the DOM node.
  • Loading branch information
bbondy committed Feb 12, 2017
1 parent 6cf4f1b commit da801c7
Show file tree
Hide file tree
Showing 7 changed files with 167 additions and 33 deletions.
6 changes: 6 additions & 0 deletions app/renderer/components/bookmarksToolbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,15 @@ class BookmarkToolbarButton extends ImmutableComponent {
this.onDragOver = this.onDragOver.bind(this)
this.onContextMenu = this.onContextMenu.bind(this)
}
componentDidMount () {
this.bookmarkNode.addEventListener('auxclick', this.onAuxClick.bind(this))
}
get activeFrame () {
return windowStore.getFrame(this.props.activeFrameKey)
}
onAuxClick (e) {
this.onClick(e)
}
onClick (e) {
if (!this.props.clickBookmarkItem(this.props.bookmark, e) &&
this.props.bookmark.get('tags').includes(siteTags.BOOKMARK_FOLDER)) {
Expand Down
44 changes: 44 additions & 0 deletions app/renderer/components/urlBarSuggestionItem.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/* 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/. */

const React = require('react')
const ImmutableComponent = require('../../../js/components/immutableComponent')
const suggestionTypes = require('../../../js/constants/suggestionTypes')
const cx = require('../../../js/lib/classSet')

class UrlBarSuggestionItem extends ImmutableComponent {
componentDidMount () {
this.node.addEventListener('auxclick', this.props.onClick)
}
componentWillUpdate (nextProps) {
if (!this.props.selected && nextProps.selected) {
this.node.scrollIntoView()
}
}
render () {
return <li data-index={this.props.currentIndex}
onMouseOver={this.props.onMouseOver.bind(this)}
onClick={this.props.onClick}
key={`${this.props.suggestion.location}|${this.props.currentIndex + this.props.i}`}
ref={(node) => { this.node = node }}
className={cx({
selected: this.props.selected,
suggestionItem: true,
[this.props.suggestion.type]: true
})}>
{
this.props.suggestion.type !== suggestionTypes.TOP_SITE && this.props.suggestion.title
? <div className='suggestionTitle'>{this.props.suggestion.title}</div>
: null
}
{
this.props.suggestion.type !== suggestionTypes.SEARCH && this.props.suggestion.type !== suggestionTypes.ABOUT_PAGES
? <div className='suggestionLocation'>{this.props.suggestion.location}</div>
: null
}
</li>
}
}

module.exports = UrlBarSuggestionItem
39 changes: 10 additions & 29 deletions app/renderer/components/urlBarSuggestions.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
* You can obtain one at http://mozilla.org/MPL/2.0/. */

const React = require('react')

const windowActions = require('../../../js/actions/windowActions')
const ImmutableComponent = require('../../../js/components/immutableComponent')

const UrlBarSuggestionItem = require('./urlBarSuggestionItem')
const windowActions = require('../../../js/actions/windowActions')
const suggestionTypes = require('../../../js/constants/suggestionTypes')
const cx = require('../../../js/lib/classSet')
const locale = require('../../../js/l10n')
Expand All @@ -16,6 +16,7 @@ class UrlBarSuggestions extends ImmutableComponent {
constructor () {
super()
this.onSuggestionClicked = this.onSuggestionClicked.bind(this)
this.onMouseOver = this.onMouseOver.bind(this)
}

get activeIndex () {
Expand Down Expand Up @@ -63,27 +64,13 @@ class UrlBarSuggestions extends ImmutableComponent {
items = items.concat(suggestions.map((suggestion, i) => {
const currentIndex = index + i
const selected = this.activeIndex === currentIndex || (!this.activeIndex && currentIndex === 0 && this.props.hasLocationValueSuffix)
return <li data-index={currentIndex}
onMouseOver={this.onMouseOver.bind(this)}
onClick={this.onSuggestionClicked}
key={`${suggestion.location}|${index + i}`}
ref={(node) => { selected && (this.selectedElement = node) }}
className={cx({
selected,
suggestionItem: true,
[suggestion.type]: true
})}>
{
suggestion.type !== suggestionTypes.TOP_SITE && suggestion.title
? <div className='suggestionTitle'>{suggestion.title}</div>
: null
}
{
suggestion.type !== suggestionTypes.SEARCH && suggestion.type !== suggestionTypes.ABOUT_PAGES
? <div className='suggestionLocation'>{suggestion.location}</div>
: null
}
</li>
return <UrlBarSuggestionItem
suggestion={suggestion}
selected={selected}
currentIndex={currentIndex}
i={i}
onMouseOver={this.onMouseOver}
onClick={this.onSuggestionClicked} />
}))
index += suggestions.size
}
Expand All @@ -109,12 +96,6 @@ class UrlBarSuggestions extends ImmutableComponent {
componentDidMount () {
}

componentWillUpdate (nextProps) {
if (this.selectedElement) {
this.selectedElement.scrollIntoView()
}
}

updateSuggestions (newIndex) {
const suggestions = this.suggestionList || this.props.suggestionList
if (!suggestions) {
Expand Down
9 changes: 9 additions & 0 deletions js/components/contextMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ const separatorMenuItem = require('../../app/common/commonMenu').separatorMenuIt
const keyCodes = require('../../app/common/constants/keyCodes')

class ContextMenuItem extends ImmutableComponent {
componentDidMount () {
if (this.node) {
this.node.addEventListener('auxclick', this.onAuxClick.bind(this))
}
}
get submenu () {
return this.props.contextMenuItem.get('submenu')
}
Expand Down Expand Up @@ -65,6 +70,9 @@ class ContextMenuItem extends ImmutableComponent {
}
windowActions.setContextMenuDetail()
}
onAuxClick (e) {
this.onClick(this.props.contextMenuItem.get('click'), true, e)
}

onMouseEnter (e) {
let openedSubmenuDetails = this.props.contextMenuDetail.get('openedSubmenuDetails')
Expand Down Expand Up @@ -156,6 +164,7 @@ class ContextMenuItem extends ImmutableComponent {
}

return <div {...props}
ref={(node) => { this.node = node }}
draggable={this.props.contextMenuItem.get('draggable')}
onDragStart={this.onDragStart.bind(this)}
onDragEnd={this.onDragEnd.bind(this)}
Expand Down
9 changes: 9 additions & 0 deletions js/components/longPressButton.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ class LongPressButton extends ImmutableComponent {
this.onMouseLeave = this.onMouseLeave.bind(this)
}

componentDidMount (e) {
this.buttonNode.addEventListener('auxclick', this.onAuxClick.bind(this))
}

onMouseDown (e) {
if (e.target.attributes.getNamedItem('disabled')) {
return
Expand Down Expand Up @@ -67,11 +71,16 @@ class LongPressButton extends ImmutableComponent {
}
}

onAuxClick (e) {
this.onClick(e)
}

render () {
return <button data-l10n-id={this.props.l10nId}
className={this.props.className}
disabled={this.props.disabled}
onClick={this.onClick}
ref={(node) => { this.buttonNode = node }}
onMouseDown={this.onMouseDown}
onMouseUp={this.onMouseUp}
onMouseLeave={this.onMouseLeave} />
Expand Down
10 changes: 6 additions & 4 deletions js/components/tab.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,15 +154,16 @@ class Tab extends ImmutableComponent {
windowActions.setTabHoverState(this.frame, true)
}

onClickTab (e) {
// Middle click should close tab
onAuxClick (e) {
if (e.button === 1) {
this.onTabClosedWithMouse(e)
} else {
this.setActiveFrame(e)
}
}

onClickTab (e) {
this.setActiveFrame(e)
}

get themeColor () {
return this.props.paintTabs &&
(this.props.tab.get('themeColor') || this.props.tab.get('computedThemeColor'))
Expand Down Expand Up @@ -203,6 +204,7 @@ class Tab extends ImmutableComponent {

componentDidMount () {
this.onUpdateTabSize()
this.tabNode.addEventListener('auxclick', this.onAuxClick.bind(this))
window.addEventListener('resize', throttle(this.onUpdateTabSize, tabUpdateFrameRate))
}

Expand Down
83 changes: 83 additions & 0 deletions test/unit/app/renderer/components/urlBarSuggestionItemTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/* 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 mockery = require('mockery')
const {mount} = require('enzyme')
const assert = require('assert')
const sinon = require('sinon')
const fakeElectron = require('../../../lib/fakeElectron')
const suggestionTypes = require('../../../../../js/constants/suggestionTypes')
let UrlBarSuggestionItem
require('../../../braveUnit')

describe('UrlBarSuggestionItem component', function () {
before(function () {
mockery.enable({
warnOnReplace: false,
warnOnUnregistered: false,
useCleanCache: true
})
mockery.registerMock('electron', fakeElectron)
UrlBarSuggestionItem = require('../../../../../app/renderer/components/urlBarSuggestionItem')
})
after(function () {
mockery.disable()
})

Object.values(suggestionTypes).forEach((suggestionType) => {
describe(`${suggestionType} suggestion item`, function () {
before(function () {
this.onMouseOver = sinon.spy()
this.onSuggestionClicked = sinon.spy()
this.suggestion = {
title: 'hello',
type: suggestionType,
location: 'http://www.brave.com'
}
this.result = mount(<UrlBarSuggestionItem
suggestion={this.suggestion}
selected
currentIndex={1}
i={0}
onMouseOver={this.onMouseOver}
onClick={this.onSuggestionClicked}
/>)
})

it('renders a list item', function () {
assert.equal(this.result.find('li').length, 1)
})

it('renders the suggestion title', function () {
if (suggestionType !== suggestionTypes.TOP_SITE) {
assert.equal(this.result.find('.suggestionTitle').length, 1)
assert.equal(this.result.find('.suggestionTitle').text(), this.suggestion.title)
} else {
assert.equal(this.result.find('.suggestionTitle').length, 0)
}
})

it('renders a suggestion URL', function () {
if (suggestionType !== suggestionTypes.SEARCH && suggestionType !== suggestionTypes.ABOUT_PAGES) {
assert.equal(this.result.find('.suggestionLocation').length, 1)
assert.equal(this.result.find('.suggestionLocation').text(), this.suggestion.location)
} else {
assert.equal(this.result.find('.suggestionLocation').length, 0)
}
})

it('detects mouse moves', function () {
this.result.simulate('click')
assert.ok(this.onSuggestionClicked.calledOnce)
assert.ok(this.onMouseOver.notCalled)
})

it('detects mouse moves', function () {
this.result.simulate('mouseover')
assert.ok(this.onMouseOver.calledOnce)
})
})
})
})

5 comments on commit da801c7

@bsclifton
Copy link
Member

Choose a reason for hiding this comment

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

Couple of things:

  • Wow- this change in C55 sucks ☹️ This could
  • One way we can find other (possibly missed) items would be to grep for eventUtil. This is used for cmd + click but also it checks middle click. Some places that may/may not have been missed:
    • /app/common/lib/menuUtil.js
    • /js/contextMenus.js
    • /js/actions/bookmarkActions.js
    • /js/components/navigationBar.js
  • Do we need to register remove event listeners too? This is something I did in components where I needed to capture specific input. Ex:
componentWillUnmount () {
    this.bookmarkNode.removeEventListener('auxclick', this.onAuxClick)
  }

Those issues aside, I think routing AuxClick through Click is a great approach 😄

@bbondy
Copy link
Member Author

@bbondy bbondy commented on da801c7 Feb 12, 2017

Choose a reason for hiding this comment

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

I grepped for isForSecondaryAction and also for e\.button and got those instances.

@bbondy
Copy link
Member Author

@bbondy bbondy commented on da801c7 Feb 12, 2017

Choose a reason for hiding this comment

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

I think the nodes will be removed from DOM and discarded along with the event listener for that when it isn't needed

@bsclifton
Copy link
Member

@bsclifton bsclifton commented on da801c7 Feb 12, 2017

Choose a reason for hiding this comment

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

@bbondy for example, with the js/contextMenus.js, we only have handlers for click. Are these covered because the only potential caller is js/ContextMenu.js (which you updated to route auxclick => click)?

If those situations are all covered, I think we're good to go 😄 The first one I saw which may have been missed was with regard to menu... not sure if you can middle click menu on macOS

@bbondy
Copy link
Member Author

@bbondy bbondy commented on da801c7 Feb 13, 2017

Choose a reason for hiding this comment

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

Yes we only ever supported that in our own custom context menus and those are handled and routed to onClick

Please sign in to comment.