From 3468b09ff77e8e0a216e151ff985a51f59e1f493 Mon Sep 17 00:00:00 2001 From: Radoslav Vitanov Date: Sat, 22 Oct 2016 06:40:54 +0300 Subject: [PATCH] UrlBar and suggestion improvements Fixes #5025 FIxes #4941 Fixes #5043 --- docs/state.md | 1 + js/actions/windowActions.js | 12 +++++++ js/components/urlBar.js | 37 ++++++++++----------- js/components/urlBarSuggestions.js | 42 ++++++++++++++---------- js/constants/windowConstants.js | 1 + js/stores/windowStore.js | 9 +++++ test/components/navigationBarTest.js | 7 ++-- test/components/urlBarSuggestionsTest.js | 14 ++++---- test/components/urlBarTest.js | 19 +++++------ 9 files changed, 84 insertions(+), 58 deletions(-) diff --git a/docs/state.md b/docs/state.md index 3bafe3e2d30..9b0311fc60d 100644 --- a/docs/state.md +++ b/docs/state.md @@ -316,6 +316,7 @@ WindowStore type: string // The type of suggestion (one of js/constants/suggestionTypes.js) }, urlSuffix: string, // autocomplete suffix + shouldRender: boolean, // if the suggestions should render autocompleteEnabled: boolean // used to enable or disable autocomplete }, focused: boolean, // whether the urlbar is focused diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index f2e02049991..6ef8b08fcf6 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -531,6 +531,18 @@ const windowActions = { }) }, + /* + * Sets if we should render URL bar suggestions. + * + * @param enabled If false URL bar suggestions will not be rendered. + */ + setRenderUrlBarSuggestions: function (enabled) { + dispatch({ + actionType: WindowConstants.WINDOW_SET_RENDER_URL_BAR_SUGGESTIONS, + enabled + }) + }, + /* * Sets the URL bar preview value. * TODO: name this something better. diff --git a/js/components/urlBar.js b/js/components/urlBar.js index a801a082ead..4583c632cd1 100644 --- a/js/components/urlBar.js +++ b/js/components/urlBar.js @@ -38,16 +38,14 @@ class UrlBar extends ImmutableComponent { this.onFocus = this.onFocus.bind(this) this.onBlur = this.onBlur.bind(this) this.onKeyDown = this.onKeyDown.bind(this) - this.onCut = this.onCut.bind(this) this.onKeyUp = this.onKeyUp.bind(this) this.onClick = this.onClick.bind(this) this.onContextMenu = this.onContextMenu.bind(this) this.activateSearchEngine = false this.searchSelectEntry = null this.keyPressed = false - this.disableAutocomplete = false this.showAutocompleteResult = debounce(() => { - if (!this.urlInput || this.keyPressed || this.disableAutocomplete) { + if (!this.urlInput || this.keyPressed) { return } const suffixLen = this.props.locationValueSuffix.length @@ -110,12 +108,10 @@ class UrlBar extends ImmutableComponent { if (!this.isActive) { windowActions.setUrlBarActive(true) } - if (e.keyCode > 47 && e.keyCode < 112) { - this.disableAutocomplete = false - } switch (e.keyCode) { case KeyCodes.ENTER: windowActions.setUrlBarActive(false) + windowActions.setRenderUrlBarSuggestions(false) this.restore() e.preventDefault() @@ -167,7 +163,6 @@ class UrlBar extends ImmutableComponent { } break case KeyCodes.UP: - this.disableAutocomplete = false if (this.shouldRenderUrlBarSuggestions) { // TODO: We shouldn't be calling into urlBarSuggestions from the parent component at all this.urlBarSuggestions.previousSuggestion() @@ -175,7 +170,6 @@ class UrlBar extends ImmutableComponent { } break case KeyCodes.DOWN: - this.disableAutocomplete = false if (this.shouldRenderUrlBarSuggestions) { // TODO: We shouldn't be calling into urlBarSuggestions from the parent component at all if (!this.urlBarSuggestions.suggestionList) { @@ -204,8 +198,16 @@ class UrlBar extends ImmutableComponent { case KeyCodes.BACKSPACE: this.hideAutoComplete() break + // Do not trigger rendering of suggestions if you are pressing alt or shift + case KeyCodes.ALT: + case KeyCodes.SHIFT: + case KeyCodes.CMD1: + case KeyCodes.CMD2: + case KeyCodes.CTRL: + break default: this.keyPressed = true + windowActions.setRenderUrlBarSuggestions(true) // Any other keydown is fair game for autocomplete to be enabled. if (!this.autocompleteEnabled) { windowActions.setUrlBarAutocompleteEnabled(true) @@ -262,10 +264,6 @@ class UrlBar extends ImmutableComponent { this.searchSelectEntry = null } - onCut () { - this.disableAutocomplete = true - } - onKeyUp (e) { switch (e.keyCode) { case KeyCodes.UP: @@ -281,6 +279,10 @@ class UrlBar extends ImmutableComponent { this.clearSearchEngine() this.detectSearchEngine(e.target.value) this.keyPressed = false + + if ((e.target.value !== undefined) && e.target.value.length === 0) { + windowActions.setRenderUrlBarSuggestions(false) + } } onFocus (e) { @@ -304,9 +306,7 @@ class UrlBar extends ImmutableComponent { componentWillMount () { ipc.on(messages.SHORTCUT_FOCUS_URL, (e) => { - // If the user hits Command+L while in the URL bar they want everything suggested as the new potential URL to laod. - this.disableAutocomplete = true - this.updateLocationToSuggestion() + windowActions.setRenderUrlBarSuggestions(false) windowActions.setUrlBarSelected(true) windowActions.setUrlBarActive(true) // The urlbar "selected" might already be set in the window state, so subsequent Command+L won't trigger component updates, so this needs another DOM refresh for selection. @@ -402,11 +402,9 @@ class UrlBar extends ImmutableComponent { windowActions.setSiteInfoVisible(true) } - // Currently even if there are no suggestions we render the URL bar suggestions because - // it needs to generate them. This needs to be refactored. See https://github.com/brave/browser-laptop/issues/3151 get shouldRenderUrlBarSuggestions () { - return (this.props.urlbar.get('location') || this.props.urlbar.get('urlPreview')) && - this.props.urlbar.get('active') + let shouldRender = this.props.urlbar.getIn(['suggestions', 'shouldRender']) + return shouldRender === true } onDragStart (e) { @@ -464,7 +462,6 @@ class UrlBar extends ImmutableComponent { onFocus={this.onFocus} onBlur={this.onBlur} onKeyDown={this.onKeyDown} - onCut={this.onCut} onKeyUp={this.onKeyUp} onClick={this.onClick} onContextMenu={this.onContextMenu} diff --git a/js/components/urlBarSuggestions.js b/js/components/urlBarSuggestions.js index 7d436659f21..44003a8f013 100644 --- a/js/components/urlBarSuggestions.js +++ b/js/components/urlBarSuggestions.js @@ -170,23 +170,26 @@ class UrlBarSuggestions extends ImmutableComponent { } componentDidMount () { - this.suggestionList = this.getNewSuggestionList() - this.searchXHR() + if (this.props.urlLocation.length > 0) { + this.suggestionList = this.getNewSuggestionList(this.props) + this.searchXHR(this.props, true) + } } componentWillUpdate (nextProps) { if (this.selectedElement) { this.selectedElement.scrollIntoView() } - // If both the URL is the same and the number of sites to consider is // the same then we don't need to regenerate the suggestions list. if (this.props.urlLocation === nextProps.urlLocation && - this.props.sites.size === nextProps.sites.size) { + this.props.sites.size === nextProps.sites.size && + // Make sure to update if there are online suggestions + this.props.searchResults.size === nextProps.searchResults.size) { return } this.suggestionList = this.getNewSuggestionList(nextProps) - this.searchXHR(nextProps) + this.searchXHR(nextProps, !(this.props.urlLocation === nextProps.urlLocation)) } getNewSuggestionList (props) { @@ -206,6 +209,8 @@ class UrlBarSuggestions extends ImmutableComponent { delete this.shiftKey const location = formatUrl(site) + // When clicked make sure to hide autocomplete + windowActions.setRenderUrlBarSuggestions(false) if (eventUtil.isForSecondaryAction(e)) { windowActions.newFrame({ location, @@ -386,7 +391,7 @@ class UrlBarSuggestions extends ImmutableComponent { windowActions.setUrlBarSuggestions(suggestions, newIndex) } - searchXHR (props) { + searchXHR (props, searchOnline) { props = props || this.props let autocompleteURL = props.searchSelectEntry ? props.searchSelectEntry.autocomplete : props.searchDetail.get('autocompleteURL') @@ -402,19 +407,20 @@ class UrlBarSuggestions extends ImmutableComponent { const replaceRE = new RegExp('^' + props.searchSelectEntry.shortcut + ' ', 'g') urlLocation = urlLocation.replace(replaceRE, '') } - const xhr = new window.XMLHttpRequest({mozSystem: true}) - xhr.open('GET', autocompleteURL - .replace('{searchTerms}', encodeURIComponent(urlLocation)), true) - xhr.responseType = 'json' - xhr.send() - xhr.onload = () => { - windowActions.setUrlBarSuggestionSearchResults(Immutable.fromJS(xhr.response[1])) - this.updateSuggestions(props.selectedIndex) - } + // Render all suggestions asap + this.updateSuggestions(props.selectedIndex) - xhr.onerror = () => { - windowActions.setUrlBarSuggestionSearchResults(Immutable.fromJS([])) - this.updateSuggestions(props.selectedIndex) + if (searchOnline) { + const xhr = new window.XMLHttpRequest({mozSystem: true}) + xhr.open('GET', autocompleteURL + .replace('{searchTerms}', encodeURIComponent(urlLocation)), true) + xhr.responseType = 'json' + xhr.send() + xhr.onload = () => { + // Once we have the online suggestions, append them to the others + windowActions.setUrlBarSuggestionSearchResults(Immutable.fromJS(xhr.response[1])) + this.updateSuggestions(props.selectedIndex) + } } } else { windowActions.setUrlBarSuggestionSearchResults(Immutable.fromJS([])) diff --git a/js/constants/windowConstants.js b/js/constants/windowConstants.js index f2af206e62f..0b46368a4dc 100644 --- a/js/constants/windowConstants.js +++ b/js/constants/windowConstants.js @@ -26,6 +26,7 @@ const windowConstants = { WINDOW_SET_NAVBAR_FOCUSED: _, WINDOW_SET_LINK_HOVER_PREVIEW: _, WINDOW_SET_URL_BAR_SUGGESTIONS: _, + WINDOW_SET_RENDER_URL_BAR_SUGGESTIONS: _, WINDOW_SET_URL_BAR_AUTCOMPLETE_ENABLED: _, WINDOW_SET_URL_BAR_PREVIEW: _, WINDOW_SET_URL_BAR_SUGGESTION_SEARCH_RESULTS: _, diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index 891be3d88c7..6d43565a13b 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -300,6 +300,7 @@ const doAction = (action) => { break case WindowConstants.WINDOW_SET_NAVIGATED: action.location = action.location.trim() + windowState = windowState.setIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'suggestions', 'shouldRender']), false) // For about: URLs, make sure we store the URL as about:something // and not what we map to. action.location = getSourceAboutUrl(action.location) || action.location @@ -558,6 +559,14 @@ const doAction = (action) => { }) } break + case WindowConstants.WINDOW_SET_RENDER_URL_BAR_SUGGESTIONS: + windowState = windowState.setIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'suggestions', 'shouldRender']), action.enabled) + if (!action.enabled) { + // Make sure to remove the suffix from the url bar + windowState = windowState.setIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'suggestions', 'selectedIndex']), null) + updateUrlSuffix(undefined) + } + break case WindowConstants.WINDOW_SET_URL_BAR_AUTCOMPLETE_ENABLED: windowState = windowState.setIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'suggestions', 'autocompleteEnabled']), action.enabled) break diff --git a/test/components/navigationBarTest.js b/test/components/navigationBarTest.js index 00aa988bf6d..60569569b58 100644 --- a/test/components/navigationBarTest.js +++ b/test/components/navigationBarTest.js @@ -738,6 +738,9 @@ describe('navigationBar', function () { // now type something yield this.app.client .setValue(urlInput, 'b') + .waitUntil(function () { + return this.getValue(urlInput).then((val) => val === 'b') + }) .waitForExist(urlBarSuggestions + ' li') }) @@ -1010,9 +1013,9 @@ describe('navigationBar', function () { .addSite({ location: 'https://brave.com', title: 'Brave' }) // now type something - yield this.app.client.keys('b') + yield this.app.client.keys('br') yield this.app.client.waitUntil(function () { - return this.getValue(urlInput).then((val) => val === 'b') + return this.getValue(urlInput).then((val) => val === 'br') }) yield blur(this.app.client) yield this.app.client diff --git a/test/components/urlBarSuggestionsTest.js b/test/components/urlBarSuggestionsTest.js index d340e6e27cb..bd04f595b17 100644 --- a/test/components/urlBarSuggestionsTest.js +++ b/test/components/urlBarSuggestionsTest.js @@ -81,15 +81,15 @@ describe('urlbarSuggestions', function () { .keys('\uE015') .waitForExist(urlBarSuggestions + ' li.suggestionItem[data-index="2"].selected') .keys('\uE007') - .tabByIndex(1).getUrl().should.become(this.page2Url) + .tabByIndex(1).getUrl().should.become(this.page1Url) }) it('selects a location auto complete result but not for titles', function * () { - const page1Url = Brave.server.url('page1.html') + const page2Url = Brave.server.url('page2.html') yield this.app.client .setValue(urlInput, 'http://') .waitUntil(function () { - return this.getValue(urlInput).then((val) => val === page1Url) + return this.getValue(urlInput).then((val) => val === page2Url) }) .waitForExist(urlBarSuggestions + ' li.selected') .setValue(urlInput, 'Page') @@ -107,20 +107,20 @@ describe('urlbarSuggestions', function () { .keys(pagePartialUrl) .waitUntil(function () { return this.getValue(urlInput).then(function (val) { - return val === page1Url // after entering partial URL matching two options, 1st is tentatively filled in (_without_ moving cursor to end) + return val === page2Url // after entering partial URL matching two options, 1st is tentatively filled in (_without_ moving cursor to end) }) }) .waitForExist(urlBarSuggestions + ' li.suggestionItem') .moveToObject(urlBarSuggestions + ' li.suggestionItem:not(.selected)') .waitUntil(function () { return this.getValue(urlInput).then(function (val) { - return val === page2Url // mousing over 2nd option tentatively completes URL with 2nd option (_without_ moving cursor to end) + return val === page1Url // mousing over 2nd option tentatively completes URL with 2nd option (_without_ moving cursor to end) }) }) - .keys('1.html') + .keys('2.html') .waitUntil(function () { return this.getValue(urlInput).then(function (val) { - return val === page1Url // without moving mouse, typing rest of 1st option URL overwrites the autocomplete from mouseover + return val === page2Url // without moving mouse, typing rest of 1st option URL overwrites the autocomplete from mouseover }) }) }) diff --git a/test/components/urlBarTest.js b/test/components/urlBarTest.js index 138bf98c746..9c39209bb78 100644 --- a/test/components/urlBarTest.js +++ b/test/components/urlBarTest.js @@ -1,8 +1,7 @@ /* global describe, it, beforeEach */ const Brave = require('../lib/brave') -const {activeWebview, urlInput} = require('../lib/selectors') -const assert = require('assert') +const {urlInput, urlBarSuggestions} = require('../lib/selectors') describe('urlBar', function () { function * setup (client) { @@ -89,18 +88,16 @@ describe('urlBar', function () { }) }) - it('it does not change input after focus until keydown', function * () { + it('does not show suggestions on focus', function * () { yield this.app.client - .keys('https://brave.com/ ') - .keys('\uE007') - .waitForElementFocus(activeWebview) + .keys('brave') + .waitUntil(function () { + return this.isExisting(urlBarSuggestions).then((exists) => exists === true) + }) .ipcSend('shortcut-focus-url') - .getValue(urlInput).then((val) => assert(val === 'https://brave.com/')) - .keys('\uE015') + .waitForElementFocus(urlInput) .waitUntil(function () { - return this.getValue(urlInput).then((val) => { - return val === 'https://brave.com/test' - }) + return this.isExisting(urlBarSuggestions).then((exists) => exists === false) }) }) })