From 97015ef728562c7f09b285907cdb707824f1ea59 Mon Sep 17 00:00:00 2001 From: NejcZdovc Date: Thu, 25 May 2017 13:57:51 +0200 Subject: [PATCH] Converts urlBarSuggestions into redux Resolves #9052 Auditors: @bsclifton @bridiver @bbondy Test Plan: - test if suggestion list is displayed - test if hover and click is working on suggestion list --- app/common/lib/suggestion.js | 49 ++++++- app/renderer/components/navigation/urlBar.js | 6 +- .../navigation/urlBarSuggestionItem.js | 39 ++++- .../navigation/urlBarSuggestions.js | 135 ++++++++---------- app/renderer/reducers/urlBarReducer.js | 7 + docs/appActions.md | 12 ++ js/actions/appActions.js | 17 +++ js/constants/appConstants.js | 3 +- .../navigation/urlBarSuggestionItemTest.js | 23 +-- 9 files changed, 202 insertions(+), 89 deletions(-) diff --git a/app/common/lib/suggestion.js b/app/common/lib/suggestion.js index be42cd00ebf..0b1b24719cd 100644 --- a/app/common/lib/suggestion.js +++ b/app/common/lib/suggestion.js @@ -627,6 +627,52 @@ const generateNewSearchXHRResults = debounce((state, windowId, tabId, input) => } }, 10) +const filterSuggestionListByType = (suggestionList) => { + const bookmarkSuggestions = [] + const historySuggestions = [] + const aboutPagesSuggestions = [] + const tabSuggestions = [] + const searchSuggestions = [] + const topSiteSuggestions = [] + + suggestionList.forEach(item => { + switch (item.get('type')) { + case suggestionTypes.BOOKMARK: + bookmarkSuggestions.push(item) + break + + case suggestionTypes.HISTORY: + historySuggestions.push(item) + break + + case suggestionTypes.ABOUT_PAGES: + aboutPagesSuggestions.push(item) + break + + case suggestionTypes.TAB: + tabSuggestions.push(item) + break + + case suggestionTypes.SEARCH: + searchSuggestions.push(item) + break + + case suggestionTypes.TOP_SITE: + topSiteSuggestions.push(item) + break + } + }) + + return { + bookmarkSuggestions, + historySuggestions, + aboutPagesSuggestions, + tabSuggestions, + searchSuggestions, + topSiteSuggestions + } +} + module.exports = { sortingPriority, sortByAccessCountWithAgeDecay, @@ -647,5 +693,6 @@ module.exports = { getSearchSuggestions, getAlexaSuggestions, generateNewSuggestionsList, - generateNewSearchXHRResults + generateNewSearchXHRResults, + filterSuggestionListByType } diff --git a/app/renderer/components/navigation/urlBar.js b/app/renderer/components/navigation/urlBar.js index 4647b61ab55..b622322e3f4 100644 --- a/app/renderer/components/navigation/urlBar.js +++ b/app/renderer/components/navigation/urlBar.js @@ -612,10 +612,8 @@ class UrlBar extends React.Component { { this.shouldRenderUrlBarSuggestions ? + menubarVisible={this.props.menubarVisible} + /> : null } diff --git a/app/renderer/components/navigation/urlBarSuggestionItem.js b/app/renderer/components/navigation/urlBarSuggestionItem.js index 73400bf7f73..c0fb3c153dd 100644 --- a/app/renderer/components/navigation/urlBarSuggestionItem.js +++ b/app/renderer/components/navigation/urlBarSuggestionItem.js @@ -3,24 +3,57 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ const React = require('react') + +// Components const ImmutableComponent = require('../immutableComponent') + +// Constants const suggestionTypes = require('../../../../js/constants/suggestionTypes') + +// Actions +const appActions = require('../../../../js/actions/appActions') +const windowActions = require('../../../../js/actions/windowActions') + +// utils const cx = require('../../../../js/lib/classSet') +const {isForSecondaryAction} = require('../../../../js/lib/eventUtil') +const {getCurrentWindowId} = require('../../currentWindow') class UrlBarSuggestionItem extends ImmutableComponent { + constructor () { + super() + this.onMouseOver = this.onMouseOver.bind(this) + } + + onMouseOver (e) { + let newIndex = parseInt(e.target.getAttribute('data-index'), 10) + + if (newIndex < 0) { + newIndex = null + } + + appActions.urlBarSelectedIndexChanged(getCurrentWindowId(), newIndex) + } + + onClick (e) { + windowActions.activeSuggestionClicked(isForSecondaryAction(e), e.shiftKey) + } + componentDidMount () { - this.node.addEventListener('auxclick', this.props.onClick) + this.node.addEventListener('auxclick', this.onClick) } + componentWillUpdate (nextProps) { if (!this.props.selected && nextProps.selected) { this.node.scrollIntoView() } } + render () { return
  • { this.node = node }} className={cx({ diff --git a/app/renderer/components/navigation/urlBarSuggestions.js b/app/renderer/components/navigation/urlBarSuggestions.js index 3295dba0d5a..f52cd81ba00 100644 --- a/app/renderer/components/navigation/urlBarSuggestions.js +++ b/app/renderer/components/navigation/urlBarSuggestions.js @@ -3,112 +3,103 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ const React = require('react') -const ImmutableComponent = require('../immutableComponent') +const Immutable = require('immutable') +// Components +const ReduxComponent = require('../reduxComponent') const UrlBarSuggestionItem = require('./urlBarSuggestionItem') -const windowActions = require('../../../../js/actions/windowActions') + +// Actions const appActions = require('../../../../js/actions/appActions') -const suggestionTypes = require('../../../../js/constants/suggestionTypes') + +// Utils const cx = require('../../../../js/lib/classSet') const locale = require('../../../../js/l10n') -const {isForSecondaryAction} = require('../../../../js/lib/eventUtil') +const suggestions = require('../../../common/lib/suggestion') +const frameStateUtil = require('../../../../js/state/frameStateUtil') const {getCurrentWindowId} = require('../../currentWindow') -class UrlBarSuggestions extends ImmutableComponent { - constructor () { - super() - this.onSuggestionClicked = this.onSuggestionClicked.bind(this) - this.onMouseOver = this.onMouseOver.bind(this) - } - - get activeIndex () { - if (this.props.suggestionList === null) { - return -1 - } - return this.props.selectedIndex - } - +class UrlBarSuggestions extends React.Component { blur () { appActions.urlBarSuggestionsChanged(getCurrentWindowId(), null, null) } - onSuggestionClicked (e) { - windowActions.activeSuggestionClicked(isForSecondaryAction(e), e.shiftKey) - } - - render () { - const suggestions = this.props.suggestionList - const bookmarkSuggestions = suggestions.filter((s) => s.get('type') === suggestionTypes.BOOKMARK) - const historySuggestions = suggestions.filter((s) => s.get('type') === suggestionTypes.HISTORY) - const aboutPagesSuggestions = suggestions.filter((s) => s.get('type') === suggestionTypes.ABOUT_PAGES) - const tabSuggestions = suggestions.filter((s) => s.get('type') === suggestionTypes.TAB) - const searchSuggestions = suggestions.filter((s) => s.get('type') === suggestionTypes.SEARCH) - const topSiteSuggestions = suggestions.filter((s) => s.get('type') === suggestionTypes.TOP_SITE) - + generateAllItems () { let items = [] let index = 0 + const addToItems = (suggestions, sectionKey, title, icon) => { - if (suggestions.size > 0) { + if (suggestions.length > 0) { items.push(
  • { icon - ? - : null + ? + : null } {title}
    • ) } + items = items.concat(suggestions.map((suggestion, i) => { const currentIndex = index + i - const selected = this.activeIndex === currentIndex || (!this.activeIndex && currentIndex === 0 && this.props.hasSuggestionMatch) + const selected = this.props.activeIndex === currentIndex || (this.props.activeIndex == null && currentIndex === 0 && this.props.hasSuggestionMatch) return + i={i} /> })) - index += suggestions.size + index += suggestions.length } - addToItems(historySuggestions, 'historyTitle', locale.translation('historySuggestionTitle'), 'fa-clock-o') - addToItems(bookmarkSuggestions, 'bookmarksTitle', locale.translation('bookmarksSuggestionTitle'), 'fa-star-o') - addToItems(aboutPagesSuggestions, 'aboutPagesTitle', locale.translation('aboutPagesSuggestionTitle'), null) - addToItems(tabSuggestions, 'tabsTitle', locale.translation('tabsSuggestionTitle'), 'fa-external-link') - addToItems(searchSuggestions, 'searchTitle', locale.translation('searchSuggestionTitle'), 'fa-search') - addToItems(topSiteSuggestions, 'topSiteTitle', locale.translation('topSiteSuggestionTitle'), 'fa-link') - const documentHeight = Number.parseInt(window.getComputedStyle(document.querySelector(':root')).getPropertyValue('--navbar-height'), 10) - const menuHeight = this.props.menubarVisible ? 30 : 0 - return
      - {items} -
    - } - onMouseOver (e) { - this.updateSuggestions(parseInt(e.target.dataset.index, 10)) + const list = suggestions.filterSuggestionListByType(this.props.suggestionList) + + addToItems(list.historySuggestions, 'historyTitle', locale.translation('historySuggestionTitle'), 'fa-clock-o') + addToItems(list.bookmarkSuggestions, 'bookmarksTitle', locale.translation('bookmarksSuggestionTitle'), 'fa-star-o') + addToItems(list.aboutPagesSuggestions, 'aboutPagesTitle', locale.translation('aboutPagesSuggestionTitle'), null) + addToItems(list.tabSuggestions, 'tabsTitle', locale.translation('tabsSuggestionTitle'), 'fa-external-link') + addToItems(list.searchSuggestions, 'searchTitle', locale.translation('searchSuggestionTitle'), 'fa-search') + addToItems(list.topSiteSuggestions, 'topSiteTitle', locale.translation('topSiteSuggestionTitle'), 'fa-link') + + return items } - componentDidMount () { + mergeProps (state, dispatchProps, ownProps) { + const currentWindow = state.get('currentWindow') + const activeFrame = frameStateUtil.getActiveFrame(currentWindow) || Immutable.Map() + const urlBar = activeFrame.getIn(['navbar', 'urlbar'], Immutable.Map()) + const documentHeight = Number.parseInt( + window.getComputedStyle(document.querySelector(':root')).getPropertyValue('--navbar-height'), 10 + ) + const menuHeight = ownProps.menubarVisible ? 30 : 0 + + const props = {} + // used in renderer + props.maxHeight = document.documentElement.offsetHeight - documentHeight - 2 - menuHeight + + // used in functions + props.menubarVisible = ownProps.menubarVisible + props.suggestionList = urlBar.getIn(['suggestions', 'suggestionList']) // TODO (nejc) improve, use primitives + props.hasSuggestionMatch = urlBar.getIn(['suggestions', 'hasSuggestionMatch']) + props.activeIndex = props.suggestionList === null + ? -1 + : urlBar.getIn(['suggestions', 'selectedIndex']) + + return props } - updateSuggestions (newIndex) { - const suggestions = this.suggestionList || this.props.suggestionList - if (!suggestions) { - return - } - // Update the urlbar preview content - if (newIndex > suggestions.size) { - newIndex = null - } - appActions.urlBarSuggestionsChanged(getCurrentWindowId(), suggestions, newIndex) + render () { + return
      + {this.generateAllItems()} +
    } } -module.exports = UrlBarSuggestions +module.exports = ReduxComponent.connect(UrlBarSuggestions) diff --git a/app/renderer/reducers/urlBarReducer.js b/app/renderer/reducers/urlBarReducer.js index 98d1940b5a0..556aa62465e 100644 --- a/app/renderer/reducers/urlBarReducer.js +++ b/app/renderer/reducers/urlBarReducer.js @@ -156,6 +156,13 @@ const urlBarReducer = (state, action) => { case appConstants.APP_URL_BAR_TEXT_CHANGED: state = setNavBarUserInput(state, action.input) break + case appConstants.APP_URL_BAR_SELECTED_INDEX_CHANGED: + { + const suggestionList = state.getIn(activeFrameStatePath(state).concat(['navbar', 'urlbar', 'suggestions', 'suggestionList'])) + state = state.setIn(activeFrameStatePath(state).concat(['navbar', 'urlbar', 'suggestions', 'selectedIndex']), action.selectedIndex) + state = updateUrlSuffix(state, suggestionList) + break + } case appConstants.APP_URL_BAR_SUGGESTIONS_CHANGED: if (action.selectedIndex !== undefined) { state = state.setIn(activeFrameStatePath(state).concat(['navbar', 'urlbar', 'suggestions', 'selectedIndex']), action.selectedIndex) diff --git a/docs/appActions.md b/docs/appActions.md index 895f2e7af4b..58656da0ad0 100644 --- a/docs/appActions.md +++ b/docs/appActions.md @@ -1098,6 +1098,18 @@ Indicates URL bar suggestions and selected index. +### urlBarSelectedIndexChanged(windowId, selectedIndex) + +Indicates URL bar selected index + +**Parameters** + +**windowId**: `number`, the window ID + +**selectedIndex**: `number`, The index for the selected item (users can select items with down arrow on their keyboard) + + + ### defaultSearchEngineLoaded(searchDetail) Dispatches a message to set the search engine details. diff --git a/js/actions/appActions.js b/js/actions/appActions.js index 9715034b900..4cfd9693e74 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -1393,6 +1393,23 @@ const appActions = { }) }, + /** + * Indicates URL bar selected index + * + * @param {number} windowId - the window ID + * @param {number} selectedIndex - The index for the selected item (users can select items with down arrow on their keyboard) + */ + urlBarSelectedIndexChanged: function (windowId, selectedIndex) { + dispatch({ + actionType: appConstants.APP_URL_BAR_SELECTED_INDEX_CHANGED, + selectedIndex, + windowId, + queryInfo: { + windowId + } + }) + }, + /** * Dispatches a message to set the search engine details. * @param {Object} searchDetail - the search details diff --git a/js/constants/appConstants.js b/js/constants/appConstants.js index 2c46754f24c..ec1efbfb048 100644 --- a/js/constants/appConstants.js +++ b/js/constants/appConstants.js @@ -131,7 +131,8 @@ const appConstants = { APP_REMOVE_PASSWORD: _, /** @param {Object} passwordDetail */ APP_REMOVE_PASSWORD_SITE: _, /** @param {Object} passwordDetail */ APP_CLEAR_PASSWORDS: _, - APP_UPDATE_LOG_OPENED: _ + APP_UPDATE_LOG_OPENED: _, + APP_URL_BAR_SELECTED_INDEX_CHANGED: _ } module.exports = mapValuesByKeys(appConstants) diff --git a/test/unit/app/renderer/components/navigation/urlBarSuggestionItemTest.js b/test/unit/app/renderer/components/navigation/urlBarSuggestionItemTest.js index d7ecf4616ce..f8fb265a2ee 100644 --- a/test/unit/app/renderer/components/navigation/urlBarSuggestionItemTest.js +++ b/test/unit/app/renderer/components/navigation/urlBarSuggestionItemTest.js @@ -1,7 +1,7 @@ /* 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 */ +/* global describe, before, after, it, afterEach */ const mockery = require('mockery') const {mount} = require('enzyme') @@ -10,10 +10,11 @@ const sinon = require('sinon') const Immutable = require('immutable') const fakeElectron = require('../../../../lib/fakeElectron') const suggestionTypes = require('../../../../../../js/constants/suggestionTypes') -let UrlBarSuggestionItem require('../../../../braveUnit') describe('UrlBarSuggestionItem component', function () { + let appActions, windowActions, UrlBarSuggestionItem + before(function () { mockery.enable({ warnOnReplace: false, @@ -22,7 +23,17 @@ describe('UrlBarSuggestionItem component', function () { }) mockery.registerMock('electron', fakeElectron) UrlBarSuggestionItem = require('../../../../../../app/renderer/components/navigation/urlBarSuggestionItem') + appActions = require('../../../../../../js/actions/appActions') + windowActions = require('../../../../../../js/actions/windowActions') + this.onMouseOver = sinon.spy(appActions, 'urlBarSelectedIndexChanged') + this.onSuggestionClicked = sinon.spy(windowActions, 'activeSuggestionClicked') + }) + + afterEach(function () { + this.onMouseOver.reset() + this.onSuggestionClicked.reset() }) + after(function () { mockery.disable() }) @@ -30,8 +41,6 @@ describe('UrlBarSuggestionItem component', function () { Object.values(suggestionTypes).forEach((suggestionType) => { describe(`${suggestionType} suggestion item`, function () { before(function () { - this.onMouseOver = sinon.spy() - this.onSuggestionClicked = sinon.spy() this.suggestion = Immutable.fromJS({ title: 'hello', type: suggestionType, @@ -42,8 +51,6 @@ describe('UrlBarSuggestionItem component', function () { selected currentIndex={1} i={0} - onMouseOver={this.onMouseOver} - onClick={this.onSuggestionClicked} />) }) @@ -69,13 +76,13 @@ describe('UrlBarSuggestionItem component', function () { } }) - it('detects mouse moves', function () { + it('detects mouse click', function () { this.result.simulate('click') assert.ok(this.onSuggestionClicked.calledOnce) assert.ok(this.onMouseOver.notCalled) }) - it('detects mouse moves', function () { + it('detects mouse over', function () { this.result.simulate('mouseover') assert.ok(this.onMouseOver.calledOnce) })