1066358 - Improve how keyword autocomplete results are displayed

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

As part of bug 951624, we want keyword result items to have a more rich display. This will require converting keyword results into actions, so we can more data available for the UI to use.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e65fe33c13ee

Comment 3

[Valerio]

Is this for bookmarks keyword and search engine keyword?

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We obviously need better language to describe these things :\

This bug only covers bookmarks keywords.

Bug 1067888 covers what is traditionally known as "keyword search" - ie, typing anything and searching via the current search engine.

Bug 1067896 covers using using an alias of a search engine, which acts the same as using bookmarks keywords.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8491378 [details] [diff] [review]
Patch v1

Review of attachment 8491378 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_action_keyword.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function promisePopupShown(popup) {

customizeableui/test/head.js suggests a promisePopupEvent.
http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/head.js#469

fwiw, I'd suggest to put this (or the more generic version for any event) in test/general/head.js

@@ +13,5 @@
> +
> +  return deferred.promise;
> +}
> +
> +function promisePopupHidden(popup) {

ditto.
I don't see a reason to have 2 helpers when they could be coalesced
internally you can check popup.state depending on the event.

@@ +36,5 @@
> +  gURLBar.onSearchComplete = function () {
> +    ok(gURLBar.popupOpen, "The autocomplete popup is correctly open");
> +    onSearchComplete.apply(gURLBar);
> +    gURLBar.onSearchComplete = onSearchComplete;
> +    searchDeferred.resolve();

please also restore onSearchComplete in a registerCleanupFunction, so in case of error we won't pollute all of the other tests.

@@ +69,5 @@
> +  yield promiseTabLoadEvent(tab);
> +
> +  let itemId =
> +    PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
> +                                         Services.io.newURI("http://example.com/?q=%s", null, null),

NetUtil.newURI

@@ +83,5 @@
> +  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 = Services.io.newURI(result.getAttribute("url"), null, null);

NetUtil.newURI

@@ +114,5 @@
> +  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 = Services.io.newURI(result.getAttribute("url"), null, null);

NetUtil.newURI

::: browser/base/content/test/general/head.js
@@ +686,5 @@
> +
> +  return false;
> +}
> +
> +function is_visible(element) {

why can't we just have is_visible and !is_visible instead of 2 separate helpers?

@@ +702,5 @@
> +
> +  return true;
> +}
> +
> +function is_element_visible(element, msg) {

ditto

@@ +704,5 @@
> +}
> +
> +function is_element_visible(element, msg) {
> +  isnot(element, null, "Element should not be null, when checking visibility");
> +  ok(is_visible(element), msg);

...but honestly I'd move the null check inside is_visible, and leave to the caller the task to do the simple ok(is_visible, "some messge") thing... we are not winning much by wrapping just ok and moreover by doing so we hide an actual test.

@@ +712,5 @@
> +  isnot(element, null, "Element should not be null, when checking visibility");
> +  ok(is_hidden(element), msg);
> +}
> +
> +function makeActionURI(action, params) {

looks like you duped this :)

::: browser/base/content/urlbarBindings.xml
@@ +1020,5 @@
> +                  break;
> +                }
> +                default: {
> +                  return;
> +                }

nit: I don't think there's a valid reason to brace the case/default here

::: browser/locales/en-US/chrome/browser/places/places.properties
@@ +92,5 @@
>  switchtabResultLabel=Tab
> +# LOCALIZATION NOTE (keywordResultLabel) :
> +# Noun used to describe the location bar autocomplete result type
> +# to users with screen readers
> +# See createResultLabel() in urlbarBindings.xml

maybe you could merge the 3 localization notes and just make a plural one... they read the same regardless.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #5)
> why can't we just have is_visible and !is_visible instead of 2 separate
> helpers?

These functions are copied verbatim from elsewhere in the tree - we have a couple of identical versions already, and I don't want to complicate things by introducing inconsistencies. Ideally they'd go into a standard testing JSM that can be shared through the tree, but we don't have such a thing yet.

> ::: browser/base/content/urlbarBindings.xml
> @@ +1020,5 @@
> > +                  break;
> > +                }
> > +                default: {
> > +                  return;
> > +                }
> 
> nit: I don't think there's a valid reason to brace the case/default here

I prefer it due to consistency and predictability - in the same sense as we try to avoid the following pattern:

  if (condition) {
    ohai();
    blah();
  } else
    do_something();

Comment 7

[Marco Bonardo [:mak]]

(In reply to Blair McBride [:Unfocused] from comment #6)
> (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #5)
> > why can't we just have is_visible and !is_visible instead of 2 separate
> > helpers?
> 
> These functions are copied verbatim from elsewhere in the tree - we have a
> couple of identical versions already, and I don't want to complicate things
> by introducing inconsistencies. Ideally they'd go into a standard testing
> JSM that can be shared through the tree, but we don't have such a thing yet.

If some code does it wrong I don't think it's a good idea to copy it, nor share it... I'd rather fix it.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Well, seems I blindly copied the wrong version anyway. Version I mean to copy doesn't have the issues you mention.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

This is the point where I realise I already pushed the patch to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/4b8f6b34b63e

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/4b8f6b34b63e

Comment 11

[Francesco Lodolo [:flod]]

Is there any screenshot of this feature in action? Because 'keyword' can get pretty long
http://transvision.mozfr.org/string/?entity=browser/chrome/browser/places/places.dtd:col.keyword.label&repo=aurora

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot

(In reply to Francesco Lodolo [:flod] from comment #11)
> Is there any screenshot of this feature in action? Because 'keyword' can get
> pretty long

The word 'keyword' is only exposed to the screenreader - it's not shown on screen. There isn't any localisation impact for on-screen changes here. See the screenshot - what's shown is the name of the bookmark, and the inputted search string (this hasn't changed). Previously we also showed the URL, which is now hidden.

Comment 13

[Francesco Lodolo [:flod]]

Thanks, a lot clearer now.

Comment 14

[Andrei Vaida [:avaida]]

Attachment: ubuntu&mac issue

Comment 15

[Andrei Vaida [:avaida]]

Testing performed on Nightly 35.0a1 (2014-09-21) using Windows 7 64-bit, Windows 8.1 64-bit (High DPI, Microsoft Surface Pro 2), Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.5.

Potential issues:
1. [All platforms] The awesomebar does not use ellipsis in case of long keyword search strings.
 * screenshot: http://i.imgur.com/vZiAArI.png 
2. [All platforms] The awesomebar does not use ellipsis in case of narrow browser window width.
 * screenshot: http://i.imgur.com/qQVXBRh.png 
 * note: most likely the same as issue no. 1
3. [All platforms] The user is able to search using keyword, despite adding multiple spaces before the keyword itself.
 * i.e.: keyword is 'cnn' (w/o apostrophe) → type in the awesomebar: space keyword search_string (e.g. ' cnn example')
4. [All platforms] The user is unable to keyword search if the keyword assigned to the bookmark contains spaces.
 * i.e.: for the cnn search, use the following keyword: ' cnn' or 'cn n' or 'cnn ' (w/o apostrophe)
5. [Ubuntu, Mac] The keyword search text from the awesomebar keeps moving vertically while typing the search string.
 * screencast: see attachment 8493001 [details]
6. [All platforms] Keyword search does not work if the keyword associated to that bookmark is an actual URL
 * i.e.: for the http://dictionary.reference.com/ bookmark, use http://dictionary.com/ keyword

Blair, what do you think?

Comment 16

[u428464]

(In reply to Andrei Vaida, QA [:avaida] from comment #14)
> Created attachment 8493001 [details]
> ubuntu&mac issue

I see this (the last visible item in the awesomebar cut off) on Windows as well.

Comment 17

[Marco Bonardo [:mak]]

(In reply to Andrei Vaida, QA [:avaida] from comment #15)
> 4. [All platforms] The user is unable to keyword search if the keyword
> assigned to the bookmark contains spaces.

We don't support keywords with spaces. If we allow to set those, it's a bug in the bookmarks dialogs.

> 6. [All platforms] Keyword search does not work if the keyword associated to
> that bookmark is an actual URL
>  * i.e.: for the http://dictionary.reference.com/ bookmark, use
> http://dictionary.com/ keyword

This use-case is (imo) uninteresting, we should not even support it, if not that I think it's not even worth to add code to avoid it.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Andrei Vaida, QA [:avaida] from comment #15)
Thanks for this, Andrei :)

Will explore this in more detail later.


> 1. [All platforms] The awesomebar does not use ellipsis in case of long
> keyword search strings.

Interesting - I remember someone saying recently that they weren't getting an elipsis, but they only saw that on Linux. That was before any of this stuff landed.


> 3. [All platforms] The user is able to search using keyword, despite adding
> multiple spaces before the keyword itself.

This is intentional.


And as Marco said, there are some things we just don't support - but we do allow to be entered in the UI. That's a rather old bug in the UI that lets you set the keyword (though I can't seem to find said bug).

Comment 19

[Alex Bardas :alexbardas]

(In reply to Blair McBride [:Unfocused] from comment #18)
> > 1. [All platforms] The awesomebar does not use ellipsis in case of long
> > keyword search strings.
> 
> Interesting - I remember someone saying recently that they weren't getting
> an elipsis, but they only saw that on Linux. That was before any of this
> stuff landed.
> 

I think you are referring to Bug 1063553.

Comment 20

[Andrei Vaida [:avaida]]

(In reply to Alex Bardas :alexbardas from comment #19)
> I think you are referring to Bug 1063553.
Issue#1 and Issue#2 reported in Comment 15 do seem to point to Bug 1063553, thanks Alex!

Since Issue#3, Issue#4 and Issue#6 have already been cleared out, I'll file a separate bug for Issue#5 and mark this one as verified. Blair, if you think we should proceed some other way with this bug, feel free to change back its status.