Bug 1066358 - Improve how keyword autocomplete results are displayed. r=mak

--HG--
extra : transplant_source : %EC%9E%D7%F8%C7-%87%90%F67%8Ah8%1E%60%CCh%23%9F-

browser/base/content/test/general/browser.ini

@@ -101,6 +101,7 @@ skip-if = os == "linux" # Bug 924307
 [browser_aboutHome.js]
 skip-if = e10s # Bug ?????? - no about:home support yet
 [browser_aboutSyncProgress.js]
+[browser_action_keyword.js]
 [browser_addKeywordSearch.js]
 skip-if = e10s
 [browser_alltabslistener.js]

browser/base/content/test/general/browser_action_keyword.js

@@ -0,0 +1,92 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+let gOnSearchComplete = null;
+
+function* promise_first_result(inputText) {
+  gURLBar.focus();
+  gURLBar.value = inputText.slice(0, -1);
+  EventUtils.synthesizeKey(inputText.slice(-1) , {});
+  yield promiseSearchComplete();
+  // On Linux, the popup may or may not be open at this stage. So we need
+  // additional checks to ensure we wait long enough.
+  yield promisePopupShown(gURLBar.popup);
+
+  let firstResult = gURLBar.popup.richlistbox.firstChild;
+  return firstResult;
+}
+
+
+add_task(function*() {
+  // This test is only relevant if UnifiedComplete is enabled.
+  if (!Services.prefs.getBoolPref("browser.urlbar.unifiedcomplete"))
+    return;
+
+  let tab = gBrowser.selectedTab = gBrowser.addTab("about:mozilla");
+  let tabs = [tab];
+  registerCleanupFunction(() => {
+    for (let tab of tabs)
+      gBrowser.removeTab(tab);
+    PlacesUtils.bookmarks.removeItem(itemId);
+  });
+
+  yield promiseTabLoadEvent(tab);
+
+  let itemId =
+    PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
+                                         NetUtil.newURI("http://example.com/?q=%s"),
+                                         PlacesUtils.bookmarks.DEFAULT_INDEX,
+                                         "test");
+  PlacesUtils.bookmarks.setKeywordForBookmark(itemId, "keyword");
+
+  let result = yield promise_first_result("keyword something");
+  isnot(result, null, "Expect a keyword result");
+
+  is(result.getAttribute("type"), "action keyword", "Expect correct  `type` attribute");
+  is(result.getAttribute("actiontype"), "keyword", "Expect correct `actiontype` attribute");
+  is(result.getAttribute("title"), "test", "Expect correct title");
+
+  // We need to make a real URI out of this to ensure it's normalised for
+  // comparison.
+  let uri = NetUtil.newURI(result.getAttribute("url"));
+  is(uri.spec, makeActionURI("keyword", {url: "http://example.com/?q=something", input: "keyword something"}).spec, "Expect correct url");
+
+  is_element_visible(result._title, "Title element should be visible");
+  is(result._title.childNodes.length, 1, "Title element should have 1 child");
+  is(result._title.childNodes[0].nodeName, "#text", "That child should be a text node");
+  is(result._title.childNodes[0].data, "test", "Node should contain the name of the bookmark");
+
+  is_element_visible(result._extra, "Extra element should be visible");
+  is(result._extra.childNodes.length, 1, "Title element should have 1 child");
+  is(result._extra.childNodes[0].nodeName, "span", "That child should be a span node");
+  let span = result._extra.childNodes[0];
+  is(span.childNodes.length, 1, "span element should have 1 child");
+  is(span.childNodes[0].nodeName, "#text", "That child should be a text node");
+  is(span.childNodes[0].data, "something", "Node should contain the query for the keyword");
+
+  is_element_hidden(result._url, "URL element should be hidden");
+
+  // Click on the result
+  info("Normal click on result");
+  let tabPromise = promiseTabLoadEvent(tab);
+  EventUtils.synthesizeMouseAtCenter(result, {});
+  let loadEvent = yield tabPromise;
+  is(loadEvent.target.location.href, "http://example.com/?q=something", "Tab should have loaded from clicking on result");
+
+  // Middle-click on the result
+  info("Middle-click on result");
+  result = yield promise_first_result("keyword somethingmore");
+  isnot(result, null, "Expect a keyword result");
+  // We need to make a real URI out of this to ensure it's normalised for
+  // comparison.
+  uri = NetUtil.newURI(result.getAttribute("url"));
+  is(uri.spec, makeActionURI("keyword", {url: "http://example.com/?q=somethingmore", input: "keyword somethingmore"}).spec, "Expect correct url");
+
+  tabPromise = promiseWaitForEvent(gBrowser.tabContainer, "TabOpen");
+  EventUtils.synthesizeMouseAtCenter(result, {button: 1});
+  let tabOpenEvent = yield tabPromise;
+  let newTab = tabOpenEvent.target;
+  tabs.push(newTab);
+  loadEvent = yield promiseTabLoadEvent(newTab);
+  is(loadEvent.target.location.href, "http://example.com/?q=somethingmore", "Tab should have loaded from middle-clicking on result");
+});

browser/base/content/test/general/browser_autocomplete_a11y_label.js

@@ -1,53 +1,11 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
-function promisePopupShown(popup) {
-  if (popup.state = "open")
-    return Promise.resolve();
-
-  let deferred = Promise.defer();
-  popup.addEventListener("popupshown", function onPopupShown(event) {
-    popup.removeEventListener("popupshown", onPopupShown);
-    deferred.resolve();
-  });
-
-  return deferred.promise;
-}
-
-function promisePopupHidden(popup) {
-  if (popup.state = "closed")
-    return Promise.resolve();
-
-  let deferred = Promise.defer();
-  popup.addEventListener("popuphidden", function onPopupHidden(event) {
-    popup.removeEventListener("popuphidden", onPopupHidden);
-    deferred.resolve();
-  });
-
-  popup.closePopup();
-
-  return deferred.promise;
-}
-
-
 function* check_a11y_label(inputText, expectedLabel) {
-  let searchDeferred = Promise.defer();
-
-  let onSearchComplete = gURLBar.onSearchComplete;
-  registerCleanupFunction(() => {
-    gURLBar.onSearchComplete = onSearchComplete;
-  });
-  gURLBar.onSearchComplete = function () {
-    ok(gURLBar.popupOpen, "The autocomplete popup is correctly open");
-    onSearchComplete.apply(gURLBar);
-    gURLBar.onSearchComplete = onSearchComplete;
-    searchDeferred.resolve();
-  }
-
   gURLBar.focus();
   gURLBar.value = inputText.slice(0, -1);
   EventUtils.synthesizeKey(inputText.slice(-1) , {});
-  yield searchDeferred.promise;
+  yield promiseSearchComplete();
   // On Linux, the popup may or may not be open at this stage. So we need
   // additional checks to ensure we wait long enough.
   yield promisePopupShown(gURLBar.popup);

browser/base/content/test/general/head.js

@@ -670,3 +670,90 @@ function makeActionURI(action, params) {
   let url = "moz-action:" + action + "," + JSON.stringify(params);
   return NetUtil.newURI(url);
 }
+
+function is_hidden(element) {
+  var style = element.ownerDocument.defaultView.getComputedStyle(element, "");
+  if (style.display == "none")
+    return true;
+  if (style.visibility != "visible")
+    return true;
+  if (style.display == "-moz-popup")
+    return ["hiding","closed"].indexOf(element.state) != -1;
+
+  // Hiding a parent element will hide all its children
+  if (element.parentNode != element.ownerDocument)
+    return is_hidden(element.parentNode);
+
+  return false;
+}
+
+function is_visible(element) {
+  var style = element.ownerDocument.defaultView.getComputedStyle(element, "");
+  if (style.display == "none")
+    return false;
+  if (style.visibility != "visible")
+    return false;
+  if (style.display == "-moz-popup" && element.state != "open")
+    return false;
+
+  // Hiding a parent element will hide all its children
+  if (element.parentNode != element.ownerDocument)
+    return is_visible(element.parentNode);
+
+  return true;
+}
+
+function is_element_visible(element, msg) {
+  isnot(element, null, "Element should not be null, when checking visibility");
+  ok(is_visible(element), msg);
+}
+
+function is_element_hidden(element, msg) {
+  isnot(element, null, "Element should not be null, when checking visibility");
+  ok(is_hidden(element), msg);
+}
+
+function promisePopupEvent(popup, eventSuffix) {
+  let endState = {shown: "open", hidden: "closed"}[eventSuffix];
+
+  if (popup.state = endState)
+    return Promise.resolve();
+
+  let eventType = "popup" + eventSuffix;
+  let deferred = Promise.defer();
+  popup.addEventListener(eventType, function onPopupShown(event) {
+    popup.removeEventListener(eventType, onPopupShown);
+    deferred.resolve();
+  });
+
+  return deferred.promise;
+}
+
+function promisePopupShown(popup) {
+  return promisePopupEvent(popup, "shown");
+}
+
+function promisePopupHidden(popup) {
+  return promisePopupEvent(popup, "hidden");
+}
+
+let gURLBarOnSearchComplete = null;
+function promiseSearchComplete() {
+  info("Waiting for onSearchComplete");
+  let deferred = Promise.defer();
+  
+  if (!gURLBarOnSearchComplete) {
+    gURLBarOnSearchComplete = gURLBar.onSearchComplete;
+    registerCleanupFunction(() => {
+      gURLBar.onSearchComplete = gURLBarOnSearchComplete;
+    });
+  }
+
+  gURLBar.onSearchComplete = function () {
+    ok(gURLBar.popupOpen, "The autocomplete popup is correctly open");
+    gURLBarOnSearchComplete.apply(gURLBar);
+    deferred.resolve();
+  }
+  
+  return deferred.promise;
+}

browser/base/content/urlbarBindings.xml

@@ -155,6 +155,10 @@
                 returnValue = action.params.url;
                 break;
               }
+              case "keyword": {
+                returnValue = action.params.input;
+                break;
+              }
             }
           }
 
@@ -313,8 +317,10 @@
                 if (switchToTabHavingURI(url) &&
                     isTabEmpty(prevTab))
                   gBrowser.removeTab(prevTab);
+                return;
+              } else if (action.type == "keyword") {
+                url = action.params.url;
               }
-              return;
             }
             continueOperation.call(this);
           }
@@ -1005,10 +1011,18 @@
             // Check if this is meant to be an action
             let action = this.mInput._parseActionUrl(url);
             if (action) {
-              if (action.type == "switchtab")
-                url = action.param;
-              else
-                return;
+              // TODO (bug 1054816): Centralise the implementation of actions
+              // into a JS module.
+              switch (action.type) {
+                case "switchtab": //Fall through.
+                case "keyword": {
+                  url = action.params.url;
+                  break;
+                }
+                default: {
+                  return;
+                }
+              }
             }
 
             // respect the usual clicking subtleties

browser/locales/en-US/chrome/browser/places/places.properties

@@ -75,21 +75,15 @@ OrganizerQueryDownloads=Downloads
 OrganizerQueryAllBookmarks=All Bookmarks
 OrganizerQueryTags=Tags
 
-# LOCALIZATION NOTE (tagResultLabel) :
+# LOCALIZATION NOTE (tagResultLabel, bookmarkResultLabel, switchtabResultLabel,
+# keywordResultLabel)
 # Noun used to describe the location bar autocomplete result type
 # to users with screen readers
 # See createResultLabel() in urlbarBindings.xml
 tagResultLabel=Tag
-# LOCALIZATION NOTE (bookmarkResultLabel) :
-# Noun used to describe the location bar autocomplete result type
-# to users with screen readers
-# See createResultLabel() in urlbarBindings.xml
 bookmarkResultLabel=Bookmark
-# LOCALIZATION NOTE (switchtabResultLabel) :
-# Noun used to describe the location bar autocomplete result type
-# to users with screen readers
-# See createResultLabel() in urlbarBindings.xml
 switchtabResultLabel=Tab
+keywordResultLabel=Keyword
 
 # LOCALIZATION NOTE (lockPrompt.text)
 # %S will be replaced with the application name.

toolkit/components/places/UnifiedComplete.js

@@ -1055,15 +1055,24 @@ Search.prototype = {
     let title = bookmarkTitle || historyTitle;
 
     if (queryType == QUERYTYPE_KEYWORD) {
-      // If we do not have a title, then we must have a keyword, so let the UI
-      // know it is a keyword.  Otherwise, we found an exact page match, so just
-      // show the page like a regular result.  Because the page title is likely
-      // going to be more specific than the bookmark title (keyword title).
-      if (!historyTitle) {
+      if (this._enableActions) {
         match.style = "keyword";
-      }
-      else {
-        title = historyTitle;
+        url = makeActionURL("keyword", {
+          url: escapedURL,
+          input: this._originalSearchString,
+        });
+        action = "keyword";
+      } else {
+        // If we do not have a title, then we must have a keyword, so let the UI
+        // know it is a keyword.  Otherwise, we found an exact page match, so just
+        // show the page like a regular result.  Because the page title is likely
+        // going to be more specific than the bookmark title (keyword title).
+        if (!historyTitle) {
+          match.style = "keyword"
+        }
+        else {
+          title = historyTitle;
+        }
       }
     }
 

toolkit/components/places/tests/unifiedcomplete/test_keyword_search_actions.js

@@ -0,0 +1,85 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/**
+ * Test for bug 392143 that puts keyword results into the autocomplete. Makes
+ * sure that multiple parameter queries get spaces converted to +, + converted
+ * to %2B, non-ascii become escaped, and pages in history that match the
+ * keyword uses the page's title.
+ *
+ * Also test for bug 249468 by making sure multiple keyword bookmarks with the
+ * same keyword appear in the list.
+ */
+
+add_task(function* test_keyword_search() {
+  let uri1 = NetUtil.newURI("http://abc/?search=%s");
+  let uri2 = NetUtil.newURI("http://abc/?search=ThisPageIsInHistory");
+  yield promiseAddVisits([ { uri: uri1, title: "Generic page title" },
+                           { uri: uri2, title: "Generic page title" } ]);
+  addBookmark({ uri: uri1, title: "Keyword title", keyword: "key"});
+
+  do_log_info("Plain keyword query");
+  yield check_autocomplete({
+    search: "key term",
+    searchParam: "enable-actions",
+    matches: [ { uri: makeActionURI("keyword", {url: "http://abc/?search=term", input: "key term"}), title: "Keyword title" } ]
+  });
+
+  do_log_info("Multi-word keyword query");
+  yield check_autocomplete({
+    search: "key multi word",
+    searchParam: "enable-actions",
+    matches: [ { uri: makeActionURI("keyword", {url: "http://abc/?search=multi+word", input: "key multi word"}), title: "Keyword title" } ]
+  });
+
+  do_log_info("Keyword query with +");
+  yield check_autocomplete({
+    search: "key blocking+",
+    searchParam: "enable-actions",
+    matches: [ { uri: makeActionURI("keyword", {url: "http://abc/?search=blocking%2B", input: "key blocking+"}), title: "Keyword title" } ]
+  });
+
+  do_log_info("Unescaped term in query");
+  yield check_autocomplete({
+    search: "key ユニコード",
+    searchParam: "enable-actions",
+    matches: [ { uri: makeActionURI("keyword", {url: "http://abc/?search=ユニコード", input: "key ユニコード"}), title: "Keyword title" } ]
+  });
+
+  do_log_info("Keyword that happens to match a page");
+  yield check_autocomplete({
+    search: "key ThisPageIsInHistory",
+    searchParam: "enable-actions",
+    matches: [ { uri: makeActionURI("keyword", {url: "http://abc/?search=ThisPageIsInHistory", input: "key ThisPageIsInHistory"}), title: "Keyword title" } ]
+  });
+
+  do_log_info("Keyword without query (without space)");
+  yield check_autocomplete({
+    search: "key",
+    searchParam: "enable-actions",
+    matches: [ { uri: makeActionURI("keyword", {url: "http://abc/?search=", input: "key"}), title: "Keyword title" } ]
+  });
+
+  do_log_info("Keyword without query (with space)");
+  yield check_autocomplete({
+    search: "key ",
+    searchParam: "enable-actions",
+    matches: [ { uri: makeActionURI("keyword", {url: "http://abc/?search=", input: "key "}), title: "Keyword title" } ]
+  });
+
+  // This adds a second keyword so anything after this will match 2 keywords
+  let uri3 = NetUtil.newURI("http://xyz/?foo=%s");
+  yield promiseAddVisits([ { uri: uri3, title: "Generic page title" } ]);
+  addBookmark({ uri: uri3, title: "Keyword title", keyword: "key"});
+
+  do_log_info("Two keywords matched");
+  yield check_autocomplete({
+    search: "key twoKey",
+    searchParam: "enable-actions",
+    matches: [ { uri: makeActionURI("keyword", {url: "http://abc/?search=twoKey", input: "key twoKey"}), title: "Keyword title" },
+               { uri: makeActionURI("keyword", {url: "http://xyz/?foo=twoKey", input: "key twoKey"}), title: "Keyword title" } ]
+  });
+
+  yield cleanup();
+});

toolkit/components/places/tests/unifiedcomplete/xpcshell.ini

@@ -21,6 +21,7 @@ tail =
 [test_escape_self.js]
 [test_ignore_protocol.js]
 [test_keyword_search.js]
+[test_keyword_search_actions.js]
 [test_keywords.js]
 [test_match_beginning.js]
 [test_multi_word_search.js]

toolkit/themes/linux/global/autocomplete.css

@@ -117,10 +117,12 @@ treechildren.autocomplete-treebody::-moz-tree-cell-text(selected) {
   color: MenuText;
 }
 
+.autocomplete-richlistitem[actiontype="keyword"] .ac-url-box,
 .autocomplete-richlistitem[type~="autofill"] .ac-url-box {
   display: none;
 }
 
+.autocomplete-richlistitem[actiontype="keyword"] .ac-title-box,
 .autocomplete-richlistitem[type~="autofill"] .ac-title-box {
   margin-top: 12px;
   margin-bottom: 12px;

toolkit/themes/osx/global/autocomplete.css

@@ -102,10 +102,12 @@ treechildren.autocomplete-treebody::-moz-tree-cell-text(selected) {
   padding: 5px 2px;
 }
 
+.autocomplete-richlistitem[actiontype="keyword"] .ac-url-box,
 .autocomplete-richlistitem[type~="autofill"] .ac-url-box {
   display: none;
 }
 
+.autocomplete-richlistitem[actiontype="keyword"] .ac-title-box,
 .autocomplete-richlistitem[type~="autofill"] .ac-title-box {
   margin-top: 12px;
   margin-bottom: 12px;

toolkit/themes/windows/global/autocomplete.css

@@ -130,10 +130,12 @@ treechildren.autocomplete-treebody::-moz-tree-cell-text(selected) {
 }
 %endif
 
+.autocomplete-richlistitem[actiontype="keyword"] .ac-url-box,
 .autocomplete-richlistitem[type~="autofill"] .ac-url-box {
   display: none;
 }
 
+.autocomplete-richlistitem[actiontype="keyword"] .ac-title-box,
 .autocomplete-richlistitem[type~="autofill"] .ac-title-box {
   margin-top: 12px;
   margin-bottom: 12px;