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

Commit

Permalink
Command+L when in url bar with URL bar suggestion should select full URL
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bbondy committed Jul 10, 2017
1 parent 7efb0d0 commit 36d569f
Show file tree
Hide file tree
Showing 7 changed files with 56 additions and 41 deletions.
23 changes: 9 additions & 14 deletions app/renderer/components/navigation/urlBar.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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) {
Expand Down Expand Up @@ -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, '')
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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) {
Expand Down
29 changes: 24 additions & 5 deletions app/renderer/reducers/urlBarReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -131,15 +147,18 @@ 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
})
// selection implies focus
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
}

Expand All @@ -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
}
Expand Down Expand Up @@ -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)
Expand All @@ -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:
Expand Down
8 changes: 2 additions & 6 deletions docs/windowActions.md
Original file line number Diff line number Diff line change
Expand Up @@ -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



Expand Down
9 changes: 3 additions & 6 deletions js/actions/windowActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
},

Expand Down
2 changes: 1 addition & 1 deletion js/constants/windowConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 14 additions & 8 deletions test/navbar-components/navigationBarTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 * () {
Expand Down Expand Up @@ -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')
})
Expand Down Expand Up @@ -1223,20 +1224,25 @@ 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')
yield this.app.client.waitForElementFocus(urlInput)
})

it('has an empty url with placeholder', function * () {
yield defaultUrlInputValue(this.app.client)
})

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')
})
})

Expand Down
4 changes: 3 additions & 1 deletion test/navbar-components/urlBarTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const Brave = require('../lib/brave')
const {urlInput, urlBarSuggestions, urlbarIcon, reloadButton} = require('../lib/selectors')
const searchProviders = require('../../js/data/searchProviders')
const config = require('../../js/constants/config')
const settings = require('../../js/constants/settings')

describe('urlBar tests', function () {
function * setup (client) {
Expand Down Expand Up @@ -708,6 +709,7 @@ describe('urlBar tests', function () {
this.page1Url = Brave.server.url('page1.html')
yield setup(this.app.client)
yield this.app.client
.changeSetting(settings.DISABLE_TITLE_MODE, false)
.waitForExist(urlInput)
.waitForElementFocus(urlInput)
.tabByIndex(0)
Expand Down Expand Up @@ -767,7 +769,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)
Expand Down

0 comments on commit 36d569f

Please sign in to comment.