From 7fc8cd2a2c2e455e6d3fb016fbe4e193b40c3916 Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Sun, 9 Jul 2017 16:07:08 -0400 Subject: [PATCH] Command+L when in url bar with URL bar suggestion should select full URL There was also a lot of other fixes which moves things out of the component and into the store. Note that there are no state functions in use yet for the window renderer so I didn't create them yet as part of this task and just used immutableJS directly. I also fixed some intermittents with this fix. Fix #9914 Auditors: @bsclifton Test Plan: 1. Fully test url bar and suggestions thoroughly 2. Type in some chars that lead to a autocomplete selection, press command+L and make sure it selects all text. --- app/renderer/components/navigation/urlBar.js | 23 ++++++---------- app/renderer/reducers/urlBarReducer.js | 29 ++++++++++++++++---- docs/windowActions.md | 8 ++---- js/actions/windowActions.js | 9 ++---- js/constants/windowConstants.js | 2 +- test/navbar-components/navigationBarTest.js | 21 ++++++++++---- test/navbar-components/urlBarTest.js | 2 +- 7 files changed, 55 insertions(+), 39 deletions(-) diff --git a/app/renderer/components/navigation/urlBar.js b/app/renderer/components/navigation/urlBar.js index 4ba5f2a2d32..1a6a0f8e31b 100644 --- a/app/renderer/components/navigation/urlBar.js +++ b/app/renderer/components/navigation/urlBar.js @@ -29,7 +29,6 @@ const siteSettingsState = require('../../../common/state/siteSettingsState') // Utils const cx = require('../../../../js/lib/classSet') -const debounce = require('../../../../js/lib/debounce') const {getSetting} = require('../../../../js/settings') const contextMenus = require('../../../../js/contextMenus') const {eventElHasAncestorWithClasses, isForSecondaryAction} = require('../../../../js/lib/eventUtil') @@ -55,12 +54,6 @@ class UrlBar extends React.Component { this.onKeyPress = this.onKeyPress.bind(this) this.onClick = this.onClick.bind(this) this.onContextMenu = this.onContextMenu.bind(this) - this.showAutocompleteResult = debounce(() => { - if (!this.urlInput) { - return - } - this.updateAutocomplete(this.lastVal) - }, 10) } maybeUrlBarTextChanged (value) { @@ -111,7 +104,7 @@ class UrlBar extends React.Component { let location = this.urlInput ? this.getValue() : this.props.urlbarLocation if (location === null || location.length === 0) { - windowActions.setUrlBarSelected(true) + windowActions.urlBarSelected(true) } else { // Filter javascript URLs to prevent self-XSS location = location.replace(/^(\s*javascript:)+/i, '') @@ -298,12 +291,13 @@ class UrlBar extends React.Component { return } if (this.props.isSelected) { - windowActions.setUrlBarSelected(false) + windowActions.urlBarSelected(false) } this.maybeUrlBarTextChanged(this.lastVal) } select () { + windowActions.urlBarSelected(true) setImmediate(() => { if (this.urlInput) { this.urlInput.select() @@ -356,17 +350,18 @@ class UrlBar extends React.Component { if (!(prevProps.frameLocation === 'about:blank' && this.props.frameLocation === 'about:newtab' && this.props.urlbarLocation !== 'about:blank')) { this.setValue(this.props.displayURL) } - } else if (this.props.isActive && - this.props.urlbarLocationSuffix !== this.lastSuffix) { - this.showAutocompleteResult() + } else if (this.props.autocompleteEnabled && + this.props.normalizedSuggestion !== prevProps.normalizedSuggestion) { + this.updateAutocomplete(this.lastVal) + // This case handles when entering urlmode from tilemode } else if ((this.props.titleMode !== prevProps.titleMode) || - (!this.props.isActive && !this.props.isFocused)) { + (!this.props.isActive && !this.props.isFocused)) { this.setValue(this.props.urlbarLocation) } if (this.props.isSelected && !prevProps.isSelected) { this.select() - windowActions.setUrlBarSelected(false) + windowActions.urlBarSelected(false) } if (this.props.noScriptIsVisible && !this.props.showNoScriptInfo) { diff --git a/app/renderer/reducers/urlBarReducer.js b/app/renderer/reducers/urlBarReducer.js index b3a15ee6b7f..a893eaebd5e 100644 --- a/app/renderer/reducers/urlBarReducer.js +++ b/app/renderer/reducers/urlBarReducer.js @@ -97,6 +97,22 @@ const updateUrlSuffix = (state, suggestionList, framePath) => { return state } +/** + * Accepts whatever suffix is present as part of the user input + */ +const acceptUrlSuffix = (state, framePath) => { + const lastSuffix = state.getIn(framePath.concat(['navbar', 'urlbar', 'suggestions', 'urlSuffix'])) + if (lastSuffix) { + const input = state.getIn(framePath.concat(['navbar', 'urlbar', 'location'])) + state = setNavBarUserInput(state, input + lastSuffix, framePath) + } + state = state.mergeIn(framePath.concat(['navbar', 'urlbar', 'suggestions']), { + selectedIndex: null, + suggestionList: null + }) + return state +} + const updateNavBarInput = (state, loc, framePath) => { if (framePath === undefined) { framePath = activeFrameStatePath(state) @@ -131,7 +147,9 @@ const setActive = (state, isActive) => { } const setUrlBarSelected = (state, selected) => { - const urlBarPath = activeFrameStatePath(state).concat(['navbar', 'urlbar']) + const framePath = activeFrameStatePath(state) + const urlBarPath = framePath.concat(['navbar', 'urlbar']) + state = acceptUrlSuffix(state, framePath) state = state.mergeIn(urlBarPath, { selected: selected }) @@ -139,7 +157,8 @@ const setUrlBarSelected = (state, selected) => { if (selected) { state = state.setIn(activeFrameStatePath(state).concat(['navbar', 'urlbar', 'focused']), true) } - + state = state.setIn(activeFrameStatePath(state).concat(['navbar', 'urlbar', 'suggestions', 'autocompleteEnabled']), false) + state = setRenderUrlBarSuggestions(state, false, framePath) return state } @@ -152,9 +171,8 @@ const urlBarReducer = (state, action) => { const displayURL = navigationState.getIn(['visibleEntry', 'virtualURL']) const frame = getFrameByTabId(state, tabId) if (frame) { - state = updateNavBarInput(state, displayURL, frameStatePath(state, frame.get('key'))) + state = setNavBarUserInput(state, displayURL, frameStatePath(state, frame.get('key'))) state = state.setIn(frameStatePath(state, frame.get('key')).concat(['navbar', 'urlbar', 'suggestions', 'shouldRender']), false) - state = updateSearchEngineInfoFromInput(state, frame) } return state } @@ -185,6 +203,7 @@ const urlBarReducer = (state, action) => { state = state.setIn(activeFrameStatePath(state).concat(['navbar', 'urlbar', 'suggestions', 'selectedIndex']), action.selectedIndex) } state = setUrlSuggestions(state, action.suggestionList) + state = updateUrlSuffix(state, action.suggestionList) break case windowConstants.WINDOW_URL_BAR_ON_FOCUS: state = navigationBarState.setFocused(state, tabId, true) @@ -201,7 +220,7 @@ const urlBarReducer = (state, action) => { state = navigationBarState.setFocused(state, tabId, false) state = setActive(state, false) break - case windowConstants.WINDOW_SET_URL_BAR_SELECTED: + case windowConstants.WINDOW_URL_BAR_SELECTED: state = setUrlBarSelected(state, action.selected) break case windowConstants.WINDOW_SET_FINDBAR_SHOWN: diff --git a/docs/windowActions.md b/docs/windowActions.md index 5355ab83404..435d47ddb77 100644 --- a/docs/windowActions.md +++ b/docs/windowActions.md @@ -336,13 +336,9 @@ This is sometimes only temporarily disabled, e.g. a user is pressing backspace. -### setUrlBarSelected(isSelected) +### urlBarSelected() -Marks the URL bar text as selected or not - -**Parameters** - -**isSelected**: `boolean`, Whether or not the URL bar text input should be selected +Indicates the URLbar has been selected diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index 7802c6fbbd8..ac58b866543 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -411,14 +411,11 @@ const windowActions = { }, /** - * Marks the URL bar text as selected or not - * - * @param {boolean} isSelected - Whether or not the URL bar text input should be selected + * Indicates the URLbar has been selected */ - setUrlBarSelected: function (selected) { + urlBarSelected: function (selected) { dispatch({ - actionType: windowConstants.WINDOW_SET_URL_BAR_SELECTED, - selected + actionType: windowConstants.WINDOW_URL_BAR_SELECTED }) }, diff --git a/js/constants/windowConstants.js b/js/constants/windowConstants.js index 85c77e7c427..ca2d7a32455 100644 --- a/js/constants/windowConstants.js +++ b/js/constants/windowConstants.js @@ -40,7 +40,7 @@ const windowConstants = { WINDOW_URL_BAR_ON_BLUR: _, WINDOW_URL_BAR_ON_FOCUS: _, WINDOW_TAB_ON_FOCUS: _, - WINDOW_SET_URL_BAR_SELECTED: _, + WINDOW_URL_BAR_SELECTED: _, WINDOW_SET_FIND_DETAIL: _, WINDOW_SET_BOOKMARK_DETAIL: _, // If set, also indicates that add/edit is shown WINDOW_SET_CONTEXT_MENU_DETAIL: _, // If set, also indicates that the context menu is shown diff --git a/test/navbar-components/navigationBarTest.js b/test/navbar-components/navigationBarTest.js index 2df9d6c497b..ac204329ec6 100644 --- a/test/navbar-components/navigationBarTest.js +++ b/test/navbar-components/navigationBarTest.js @@ -98,7 +98,7 @@ describe('navigationBar tests', function () { .newTab({ url: page1Url }) .waitUntil(function () { return this.getWindowState().then((val) => { - return val.value.frames.length === 2 + return val.value.frames.length === 3 }) }) .waitForElementFocus(activeWebview) @@ -214,6 +214,7 @@ describe('navigationBar tests', function () { it('remains cleared when onChange is fired but not onKeyUp', function * () { yield this.app.client .windowByUrl(Brave.browserWindowUrl) + .activateURLMode() .waitForVisible(urlInput) .setValue(urlInput, '') .moveToObject(activeWebview) @@ -610,7 +611,7 @@ describe('navigationBar tests', function () { }) .windowByUrl(Brave.browserWindowUrl) .waitForExist(urlbarIcon + '.fa-lock') - .click(urlbarIcon) + .click(urlbarIcon + '.fa-lock') .waitForVisible('[data-test-id="secureConnection"]') .waitForVisible('[data-test-id="runInsecureContentWarning"]') .waitForVisible(dismissAllowRunInsecureContentButton) @@ -913,10 +914,9 @@ describe('navigationBar tests', function () { yield setup(this.app.client) yield this.app.client .waitForExist(urlInput) - yield this.app.client .keys(this.page1) - yield this.app.client .keys(Brave.keys.ENTER) + .activateURLMode() }) it('shows search icon when input is empty', function * () { @@ -1002,6 +1002,7 @@ describe('navigationBar tests', function () { .keys(Brave.keys.ENTER) .tabByUrl('about:about') .windowByUrl(Brave.browserWindowUrl) + .activateURLMode() .waitForInputText(urlInput, 'about:about') .waitForExist('.urlbarIcon.fa-list') }) @@ -1223,9 +1224,9 @@ describe('navigationBar tests', function () { }) describe('shortcut-focus-url', function () { - Brave.beforeAll(this) + Brave.beforeEach(this) - before(function * () { + beforeEach(function * () { yield setup(this.app.client) yield blur(this.app.client) yield this.app.client.ipcSend('shortcut-focus-url') @@ -1238,6 +1239,14 @@ describe('navigationBar tests', function () { it('has focus', function * () { yield this.app.client.waitForElementFocus(urlInput) }) + + it('selects full url when autocompleting with partial selection', function * () { + yield this.app.client + .keys('about:bra') + yield selectsText(this.app.client, 've') + yield this.app.client.ipcSend('shortcut-focus-url') + yield selectsText(this.app.client, 'about:brave') + }) }) describe('auto open bookmarks toolbar for the first bookmark', function () { diff --git a/test/navbar-components/urlBarTest.js b/test/navbar-components/urlBarTest.js index e4d470639f2..1fb3321a24f 100644 --- a/test/navbar-components/urlBarTest.js +++ b/test/navbar-components/urlBarTest.js @@ -767,7 +767,7 @@ describe('urlBar tests', function () { it('changes only the selection', function * () { yield this.app.client - .setInputText(urlInput, 'git') + .keys('git') .waitForSelectedText('hub.com') // Select next suggestion .keys(Brave.keys.DOWN)