From 47131ec21e8287d061ac3291d8a3d8bdb68ccdbc Mon Sep 17 00:00:00 2001 From: Willy Bruns Date: Mon, 8 Aug 2016 13:20:11 -0700 Subject: [PATCH 1/3] Mousing over URL bar autocomplete suggestions in dropdown suggests but does not interfere with typed URL Fixes #3012: Autofill should not happen on URL bar unless item in autocomplete list is clicked --- js/actions/windowActions.js | 4 ++-- js/components/urlBar.js | 8 -------- js/components/urlBarSuggestions.js | 2 +- js/stores/windowStore.js | 15 ++++++++++++--- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index 8ea19ce87de..da0784c7111 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -489,8 +489,8 @@ const windowActions = { setUrlBarSuggestions: function (suggestionList, selectedIndex) { dispatch({ actionType: WindowConstants.WINDOW_SET_URL_BAR_SUGGESTIONS, - suggestionList, - selectedIndex + suggestionList: suggestionList, + selectedIndex: selectedIndex }) }, diff --git a/js/components/urlBar.js b/js/components/urlBar.js index 1cac2c4aef8..b4148a3abbb 100644 --- a/js/components/urlBar.js +++ b/js/components/urlBar.js @@ -300,14 +300,6 @@ class UrlBar extends ImmutableComponent { } get locationValue () { - // If there's a selected autocomplete entry, we just want to show its location - if (this.props.suggestionIndex) { - const suggestionLocation = this.props.urlbar.getIn(['suggestions', 'suggestionList', this.props.suggestionIndex - 1]).location - if (suggestionLocation) { - return suggestionLocation - } - } - let location = this.props.urlbar.get('location') const history = this.props.history if (isIntermediateAboutPage(location) && history.size > 0) { diff --git a/js/components/urlBarSuggestions.js b/js/components/urlBarSuggestions.js index fcf50d00f3e..344a160d161 100644 --- a/js/components/urlBarSuggestions.js +++ b/js/components/urlBarSuggestions.js @@ -121,7 +121,7 @@ class UrlBarSuggestions extends ImmutableComponent { } items = items.concat(suggestions.map((suggestion, i) => { const currentIndex = index + i - const selected = this.activeIndex === currentIndex + 1 || currentIndex === 0 && this.props.locationValueSuffix + const selected = this.activeIndex === currentIndex + 1 || (!this.activeIndex && currentIndex === 0 && this.props.locationValueSuffix) return
  • { * @param suggestionList - The suggestion list to use to figure out the suffix. */ const updateUrlSuffix = (suggestionList) => { - const suggestion = suggestionList && suggestionList.get(0) + let selectedIndex = windowState.getIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'suggestions', 'selectedIndex'])) + + if (!selectedIndex) { + selectedIndex = 0 + } else { + selectedIndex-- + } + + const suggestion = suggestionList && suggestionList.get(selectedIndex) let suffix = '' if (suggestion) { - const selectedIndex = windowState.getIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'suggestions', 'selectedIndex'])) const autocompleteEnabled = windowState.getIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'suggestions', 'autocompleteEnabled'])) - if (!selectedIndex && autocompleteEnabled) { + + if (autocompleteEnabled) { const location = windowState.getIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'location'])) const index = suggestion.location.toLowerCase().indexOf(location.toLowerCase()) if (index !== -1) { @@ -467,6 +475,7 @@ const doAction = (action) => { break case WindowConstants.WINDOW_SET_URL_BAR_SUGGESTIONS: windowState = windowState.setIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'suggestions', 'selectedIndex']), action.selectedIndex) + if (action.suggestionList !== undefined) { windowState = windowState.setIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'suggestions', 'suggestionList']), action.suggestionList) } From e82d734b48563682095718962afc27698f3cd8f6 Mon Sep 17 00:00:00 2001 From: Willy Bruns Date: Thu, 11 Aug 2016 10:42:05 -0700 Subject: [PATCH 2/3] Keep urlBar#onBlur from interfering with autocomplete suggestion handler (part of fix for #3012) --- js/components/urlBar.js | 18 ++++++++++++++---- js/components/urlBarSuggestions.js | 1 + js/lib/eventUtil.js | 23 ++++++++++++++++++++++- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/js/components/urlBar.js b/js/components/urlBar.js index b4148a3abbb..d3a4d1e3f1b 100644 --- a/js/components/urlBar.js +++ b/js/components/urlBar.js @@ -24,6 +24,9 @@ var searchProviders = require('../data/searchProviders') const searchIconSize = 16 const UrlUtil = require('../lib/urlutil') +const EventUtil = require('../lib/eventUtil') +const eventElHasAncestorWithClasses = EventUtil.eventElHasAncestorWithClasses + const { isUrl, isIntermediateAboutPage } = require('../lib/appUrlUtil') class UrlBar extends ImmutableComponent { @@ -203,8 +206,11 @@ class UrlBar extends ImmutableComponent { windowActions.setUrlBarSelected(false) // On blur, a user expects the text shown from the last autocomplete suffix // to be auto entered as the new location. - this.updateLocationToSuggestion() this.clearSearchEngine() + + if (!eventElHasAncestorWithClasses(e, ['urlBarSuggestions', 'urlbarForm'])) { + this.updateLocationToSuggestion() + } } updateLocationToSuggestion () { @@ -250,9 +256,13 @@ class UrlBar extends ImmutableComponent { } onActiveFrameStop () { - this.restore() - windowActions.setUrlBarSelected(true) - windowActions.setUrlBarActive(false) + if (this.isFocused()) { + windowActions.setUrlBarActive(false) + if (!this.shouldRenderUrlBarSuggestions) { + this.restore() + windowActions.setUrlBarSelected(true) + } + } } componentWillMount () { diff --git a/js/components/urlBarSuggestions.js b/js/components/urlBarSuggestions.js index 344a160d161..a92aa143ea0 100644 --- a/js/components/urlBarSuggestions.js +++ b/js/components/urlBarSuggestions.js @@ -205,6 +205,7 @@ class UrlBarSuggestions extends ImmutableComponent { } else { windowActions.loadUrl(this.activeFrame, location) windowActions.setUrlBarActive(false) + windowActions.setUrlBarPreview(null) this.blur() } } diff --git a/js/lib/eventUtil.js b/js/lib/eventUtil.js index d87a1b308d0..56a0d002d80 100644 --- a/js/lib/eventUtil.js +++ b/js/lib/eventUtil.js @@ -3,7 +3,28 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ const isDarwin = process.platform === 'darwin' -module.exports.isForSecondaryAction = (e) => +module.exports.isForSecondaryAction = (e) => { e.ctrlKey && !isDarwin || e.metaKey && isDarwin || e.button === 1 +} + +module.exports.eventElHasAncestorWithClasses = (e, classesToCheck) => { + let node = e.target + + while (node) { + let classMatch = classesToCheck.map( + function (className) { + return (node.classList ? node.classList.contains(className) : false) + } + ).includes(true) + + if (classMatch) { + return true + } + + node = node.parentNode + } + + return false +} From 5cadc80a3c85f68a0523d6c51643568edc2fb872 Mon Sep 17 00:00:00 2001 From: Willy Bruns Date: Wed, 17 Aug 2016 01:18:10 -0700 Subject: [PATCH 3/3] Adding test for #3012 fix - added test to urlBarSuggestionsTest.js to check that mousing over autocomplete suggestions does not interrupt typing of URL --- js/actions/windowActions.js | 4 +- test/components/urlBarSuggestionsTest.js | 51 +++++++++++++++++++++++- 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index da0784c7111..8ea19ce87de 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -489,8 +489,8 @@ const windowActions = { setUrlBarSuggestions: function (suggestionList, selectedIndex) { dispatch({ actionType: WindowConstants.WINDOW_SET_URL_BAR_SUGGESTIONS, - suggestionList: suggestionList, - selectedIndex: selectedIndex + suggestionList, + selectedIndex }) }, diff --git a/test/components/urlBarSuggestionsTest.js b/test/components/urlBarSuggestionsTest.js index 397b651f186..03c4abcee00 100644 --- a/test/components/urlBarSuggestionsTest.js +++ b/test/components/urlBarSuggestionsTest.js @@ -17,6 +17,8 @@ describe('urlbarSuggestions', function () { Brave.beforeAll(this) before(function * () { this.page1Url = Brave.server.url('page1.html') + this.page2Url = Brave.server.url('page2.html') + yield setup(this.app.client) yield this.app.client .tabByUrl(Brave.newTabUrl) @@ -28,6 +30,17 @@ describe('urlbarSuggestions', function () { .windowByUrl(Brave.browserWindowUrl) .waitForExist('.tab[data-frame-key="2"].active') .waitForElementFocus(urlInput) + + // add the page2.html to history (necessary for autocomplete mouseover test) + yield this.app.client + .tabByUrl(Brave.newTabUrl) + .url(this.page2Url) + .waitForUrl(this.page2Url) + .windowByUrl(Brave.browserWindowUrl) + .ipcSend(messages.SHORTCUT_NEW_FRAME) + .waitForUrl(Brave.newTabUrl) + .windowByUrl(Brave.browserWindowUrl) + .waitForElementFocus(urlInput) }) it('navigates to a suggestion when clicked', function * () { @@ -46,7 +59,7 @@ describe('urlbarSuggestions', function () { yield this.app.client.ipcSend(messages.SHORTCUT_NEW_FRAME) .waitForUrl(Brave.newTabUrl) .windowByUrl(Brave.browserWindowUrl) - .waitForExist('.tab[data-frame-key="3"].active') + .waitForExist('.tab[data-frame-key="4"].active') .ipcSend('shortcut-focus-url') .waitForElementFocus(urlInput) .setValue(urlInput, 'Page 1') @@ -67,7 +80,7 @@ describe('urlbarSuggestions', function () { yield this.app.client.ipcSend(messages.SHORTCUT_NEW_FRAME) .waitForUrl(Brave.newTabUrl) .windowByUrl(Brave.browserWindowUrl) - .waitForExist('.tab[data-frame-key="4"].active') + .waitForExist('.tab[data-frame-key="5"].active') .ipcSend('shortcut-focus-url') .waitForElementFocus(urlInput) .setValue(urlInput, 'http://') @@ -78,4 +91,38 @@ describe('urlbarSuggestions', function () { .setValue(urlInput, 'Page') .waitForExist(urlBarSuggestions + ' li.selected', 1000, true) }) + + it('on suggestion mouseover, appends autocomplete URLs without interrupting typing', function * () { + const page1Url = Brave.server.url('page1.html') + const page2Url = Brave.server.url('page2.html') + const pagePartialUrl = Brave.server.url('page') + // test that entering a partial URL which matches two options autocompletes initially to 1st option, + // but switches to other option when that is highlighted, while keeping cursor in same position, + // so that finally, if the rest of the 1st option is entered via keyboard, it overwrites the suggestion from the mouse + yield this.app.client.ipcSend(messages.SHORTCUT_NEW_FRAME) + .waitForUrl(Brave.newTabUrl) + .windowByUrl(Brave.browserWindowUrl) + .waitForExist('.tab[data-frame-key="6"].active') + .ipcSend('shortcut-focus-url') + .waitForElementFocus(urlInput) + .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) + }) + }) + .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) + }) + }) + .keys('1.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 + }) + }) + }) })