Don't require 2 esc key presses when no suggestions

Auditors: @bridiver Fix brave#3088
ayumi · Aug 12, 2016 · c60d2f4 · c60d2f4
1 parent ceaca95
commit c60d2f4
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 6 deletions.
diff --git a/js/components/urlBar.js b/js/components/urlBar.js
@@ -252,7 +252,11 @@ class UrlBar extends ImmutableComponent {
   onActiveFrameStop () {
     if (this.isFocused()) {
       windowActions.setUrlBarActive(false)
-      if (!this.shouldRenderUrlBarSuggestions) {
+      if (!this.shouldRenderUrlBarSuggestions ||
+          // TODO: Once we take out suggestion generation from within URLBarSuggestions we can remove this check
+          // and put it in shouldRenderUrlBarSuggestions where it belongs.  See https://github.com/brave/browser-laptop/issues/3151
+          !this.props.urlbar.getIn(['suggestions', 'suggestionList']) ||
+          this.props.urlbar.getIn(['suggestions', 'suggestionList']).size === 0) {
         this.restore()
         windowActions.setUrlBarSelected(true)
       }
@@ -352,6 +356,8 @@ 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')

diff --git a/test/components/navigationBarTest.js b/test/components/navigationBarTest.js
@@ -2,7 +2,7 @@
 
 const Brave = require('../lib/brave')
 const config = require('../../js/constants/config')
-const {urlInput, activeWebview, activeTabFavicon, activeTab, navigatorLoadTime, navigator, titleBar, urlbarIcon, bookmarksToolbar, navigatorNotBookmarked, navigatorBookmarked} = require('../lib/selectors')
+const {urlBarSuggestions, urlInput, activeWebview, activeTabFavicon, activeTab, navigatorLoadTime, navigator, titleBar, urlbarIcon, bookmarksToolbar, navigatorNotBookmarked, navigatorBookmarked} = require('../lib/selectors')
 const urlParse = require('url').parse
 const assert = require('assert')
 const settings = require('../../js/constants/settings')
@@ -445,7 +445,7 @@ describe('navigationBar', function () {
       })
     })
 
-    describe('type escape once', function () {
+    describe('type escape once with suggestions', function () {
       before(function * () {
         this.page = Brave.server.url('page1.html')
         return yield this.app.client
@@ -454,18 +454,45 @@ describe('navigationBar', function () {
           .windowByUrl(Brave.browserWindowUrl)
           .ipcSend('shortcut-focus-url')
           .waitForElementFocus(urlInput)
-          .setValue(urlInput, 'blah')
+          .setValue(urlInput, 'google')
+          .waitForExist(urlBarSuggestions + ' li')
+
           // hit escape
           .keys('\uE00C')
           .waitForElementFocus(urlInput)
       })
 
       it('does not select the urlbar text', function * () {
-        yield selectsText(this.app.client, '')
+        yield selectsText(this.app.client, '.com')
       })
 
       it('does not revert the urlbar text', function * () {
-        yield this.app.client.getValue(urlInput).should.eventually.be.equal('blah')
+        yield this.app.client.getValue(urlInput).should.eventually.be.equal('google.com')
+      })
+    })
+
+    describe('type escape once with no suggestions', function () {
+      before(function * () {
+        this.page = Brave.server.url('page1.html')
+        return yield this.app.client
+          .tabByIndex(0)
+          .loadUrl(this.page)
+          .windowByUrl(Brave.browserWindowUrl)
+          .ipcSend('shortcut-focus-url')
+          .waitForElementFocus(urlInput)
+          .setValue(urlInput, 'random-uuid-d63ecb78-eec8-4c08-973b-fb39cb5a6f1a')
+
+          // hit escape
+          .keys('\uE00C')
+          .waitForElementFocus(urlInput)
+      })
+
+      it('does select the urlbar text', function * () {
+        yield selectsText(this.app.client, this.page)
+      })
+
+      it('does revert the urlbar text', function * () {
+        yield this.app.client.getValue(urlInput).should.eventually.be.equal(this.page)
       })
     })