UrlBar and suggestion improvements

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

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.

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,15 +163,13 @@ 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()
           e.preventDefault()
         }
         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}

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([]))

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: _,

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

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

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

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