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

Commit

Permalink
Fix various URLbar problems
Browse files Browse the repository at this point in the history 
Various urlbar fixes

- When cutting a URL it will not revert the user input to the original
  URL.  Fix #5063
- Fix URL sometimes getting appended. Some automated tests were
  failing with exactly that problem.  Fix #5036
- Fix various navigationBar tests. Fix #5459
- This might Fix #5444. Please re-open if not.
- Clicking in the navigation bar was only selecting part of the URL
  sometimes and it should select all. Fix #5460
- One failing automated test was disabled, it seems to be failing for
  something related to windowByIndex though and doesn't seem to be a
  problem.  It was already failing before too.  It was documented here:
  #5389

Auditors: @diracdeltas
  • Loading branch information
@bbondy
bbondy committed Nov 7, 2016
1 parent 9a8c957 commit 3e6d2de
Show file tree
Hide file tree
Showing 6 changed files with 164 additions and 87 deletions.
103 changes: 71 additions & 32 deletions js/components/urlBar.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class UrlBar extends ImmutableComponent {
this.onBlur = this.onBlur.bind(this)
this.onKeyDown = this.onKeyDown.bind(this)
this.onKeyUp = this.onKeyUp.bind(this)
this.onChange = this.onChange.bind(this)
this.onClick = this.onClick.bind(this)
this.onContextMenu = this.onContextMenu.bind(this)
this.onSiteInfo = this.onSiteInfo.bind(this)
Expand Down Expand Up @@ -73,27 +74,19 @@ class UrlBar extends ImmutableComponent {
return this.props.urlbar.get('focused')
}

// update the DOM with state that is not stored in the component
updateDOM () {
this.updateDOMInputFocus(this.isFocused())
this.updateDOMInputSelected(this.isSelected())
}

updateDOMInputFocus (focused) {
if (this.urlInput && focused) {
this.urlInput.focus()
}
}

updateDOMInputSelected (selected) {
if (this.urlInput && selected) {
this.urlInput.select()
this.focus()
}
}

// restores the url bar to the current location
restore () {
windowActions.setNavBarUserInput(this.props.location)
const location = UrlUtil.getDisplayLocation(this.props.location, getSetting(settings.PDFJS_ENABLED))
if (this.urlInput) {
this.urlInput.value = location
}
windowActions.setNavBarUserInput(location)
}

// Temporarily disable the autocomplete when a user is pressing backspace.
Expand Down Expand Up @@ -133,6 +126,8 @@ class UrlBar extends ImmutableComponent {
e.preventDefault()
ipc.emit(messages.SHORTCUT_ACTIVE_FRAME_STOP)
this.clearSearchEngine()
this.restore()
this.select()
break
case KeyCodes.DELETE:
if (e.shiftKey) {
Expand All @@ -146,6 +141,9 @@ class UrlBar extends ImmutableComponent {
}
break
case KeyCodes.BACKSPACE:
this.clearSearchEngine()
break
case KeyCodes.TAB:
this.hideAutoComplete()
break
// Do not trigger rendering of suggestions if you are pressing alt or shift
Expand All @@ -166,16 +164,17 @@ class UrlBar extends ImmutableComponent {
}

onClick (e) {
// if the url bar is already selected then clicking in it should make it active
if (this.isSelected()) {
windowActions.setUrlBarSelected(false)
windowActions.setUrlBarSelected(true)
windowActions.setUrlBarActive(true)
}
}

onBlur (e) {
windowActions.setNavBarFocused(false)
windowActions.setUrlBarSelected(false)
if (!this.isActive) {
windowActions.setNavBarUserInput(e.target.value)
}
// On blur, a user expects the text shown from the last autocomplete suffix
// to be auto entered as the new location.
this.clearSearchEngine()
Expand All @@ -192,8 +191,8 @@ class UrlBar extends ImmutableComponent {
}

detectSearchEngine (input) {
let location = input || this.props.urlbar.get('location')
if (location !== null && location.length !== 0) {
let location = typeof input === 'string' ? input : this.props.urlbar.get('location')
if (typeof location === 'string' && location.length !== 0) {
const isLocationUrl = isUrl(location)
if (!isLocationUrl &&
!(this.searchSelectEntry && location.startsWith(this.searchSelectEntry.shortcut + ' '))) {
Expand All @@ -214,6 +213,23 @@ class UrlBar extends ImmutableComponent {
this.searchSelectEntry = null
}

onChange (e) {
const oldActivateSearchEngine = this.activateSearchEngine
const oldSearchSelectEntry = this.searchSelectEntry

this.clearSearchEngine()
this.detectSearchEngine(e.target.value)

// TODO: activeSearchEngine and searchSelectEntry should be stored in
// state so we don't have to do this hack.
// It comes into play because no keyUp state changes happen when
// you use the context menu to select all and then cut.
if (oldActivateSearchEngine !== this.activateSearchEngine ||
oldSearchSelectEntry !== this.searchSelectEntry) {
this.forceUpdate()
}
}

onKeyUp (e) {
switch (e.keyCode) {
case KeyCodes.UP:
Expand Down Expand Up @@ -241,7 +257,7 @@ class UrlBar extends ImmutableComponent {
const isLocationUrl = isUrl(location)
if (!isLocationUrl && e.ctrlKey) {
windowActions.loadUrl(this.activeFrame, `www.${location}.com`)
} else if (this.shouldRenderUrlBarSuggestions && (this.urlBarSuggestions.activeIndex > 0 || this.props.locationValueSuffix)) {
} else if (this.shouldRenderUrlBarSuggestions && (this.urlBarSuggestions.activeIndex > 0 || this.props.locationValueSuffix && this.autocompleteEnabled)) {
// Hack to make alt enter open a new tab for url bar suggestions when hitting enter on them.
const isDarwin = process.platform === 'darwin'
if (e.altKey) {
Expand Down Expand Up @@ -284,6 +300,18 @@ class UrlBar extends ImmutableComponent {
}
}

select () {
if (this.urlInput) {
this.urlInput.select()
}
}

focus () {
if (this.urlInput) {
this.urlInput.focus()
}
}

onFocus (e) {
windowActions.setUrlBarSelected(true)
this.detectSearchEngine()
Expand All @@ -309,32 +337,47 @@ class UrlBar extends ImmutableComponent {
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.
this.updateDOM()
this.updateDOMInputFocus()
})
// escape key handling
ipc.on(messages.SHORTCUT_ACTIVE_FRAME_STOP, this.onActiveFrameStop)
}

componentDidMount () {
this.updateDOM()
if (this.urlInput) {
this.urlInput.value = UrlUtil.getDisplayLocation(this.props.location, getSetting(settings.PDFJS_ENABLED))
this.focus()
}
}

componentDidUpdate (prevProps) {
// this.urlInput is not initialized in titleMode
if (this.urlInput) {
// Select the part of the URL which was an autocomplete suffix.
if (this.props.locationValueSuffix.length > 0 && this.isActive &&
if (this.props.location !== prevProps.location) {
this.urlInput.value = UrlUtil.getDisplayLocation(this.props.location, getSetting(settings.PDFJS_ENABLED))
} else if (this.props.locationValueSuffix.length > 0 && this.isActive &&
(this.props.locationValueSuffix !== prevProps.locationValueSuffix ||
this.props.urlbar.get('location') !== prevProps.urlbar.get('location'))) {
this.showAutocompleteResult()
} else if (this.props.activeFrameKey !== prevProps.activeFrameKey ||
this.props.titleMode !== prevProps.titleMode) {
this.props.titleMode !== prevProps.titleMode ||
!this.isActive && !this.isFocused) {
this.urlInput.value = this.locationValue
} else if (this.isActive) {
const selectedIndex = this.activeFrame.getIn(['navbar', 'urlbar', 'suggestions', 'selectedIndex'])
if (typeof selectedIndex === 'number') {
const suggestionLocation = this.activeFrame.getIn(['navbar', 'urlbar', 'suggestions', 'suggestionList', selectedIndex - 1]).location
this.urlInput.value = suggestionLocation
}
}
}
if (this.isSelected() !== prevProps.urlbar.get('selected') ||
this.isFocused() !== prevProps.urlbar.get('focused')) {
this.updateDOM()
if (this.isFocused() !== prevProps.urlbar.get('focused')) {
this.updateDOMInputFocus()
}
if (this.isSelected() !== prevProps.urlbar.get('selected')) {
this.select()
windowActions.setUrlBarSelected(false)
}
}

Expand Down Expand Up @@ -412,9 +455,6 @@ class UrlBar extends ImmutableComponent {
const showIconSecure = !this.activateSearchEngine && this.isHTTPPage && this.props.isSecure && !this.props.urlbar.get('active')
const showIconInsecure = !this.activateSearchEngine && this.isHTTPPage && !this.props.isSecure && !this.props.urlbar.get('active') && !this.props.titleMode
const showIconSearch = !this.activateSearchEngine && this.props.urlbar.get('active') && this.props.loading === false
const value = this.isActive || (!this.locationValue && this.isFocused())
? undefined
: this.locationValue
return <form
className='urlbarForm'
action='#'
Expand Down Expand Up @@ -458,9 +498,9 @@ class UrlBar extends ImmutableComponent {
onBlur={this.onBlur}
onKeyDown={this.onKeyDown}
onKeyUp={this.onKeyUp}
onChange={this.onChange}
onClick={this.onClick}
onContextMenu={this.onContextMenu}
value={value}
data-l10n-id='urlbar'
className={cx({
insecure: !this.props.isSecure && this.props.loading === false && !this.isHTTPPage,
Expand Down Expand Up @@ -498,7 +538,6 @@ class UrlBar extends ImmutableComponent {
urlLocation={this.props.urlbar.get('location')}
urlPreview={this.props.urlbar.get('urlPreview')}
searchSelectEntry={this.searchSelectEntry}
previewActiveIndex={this.props.previewActiveIndex || 0}
menubarVisible={this.props.menubarVisible} />
: null
}
Expand Down
2 changes: 1 addition & 1 deletion js/stores/windowStore.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ const doAction = (action) => {
windowState = windowState.setIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'focused']), action.focused)
// selection should be cleared on blur
if (!action.focused) {
windowState = windowState.setIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'selected']), action.false)
windowState = windowState.setIn(activeFrameStatePath().concat(['navbar', 'urlbar', 'selected']), false)
}
break
case WindowConstants.WINDOW_NEW_FRAME:
Expand Down
Loading

1 comment on commit 3e6d2de

@diracdeltas
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for taking this!

Please sign in to comment.