Closed Bug 1034381 Opened 10 years ago Closed 10 years ago

Enhance previous searches in awesomebar dropdown by removing URL

Categories

(Firefox :: Address Bar, defect)

x86_64
All
defect
Not set
normal
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.2

People

(Reporter: jboriss, Assigned: Paolo)

References

Details

(Whiteboard: [search])

Attachments

(3 files, 7 obsolete files)

Currently in Firefox, if a user performs a search through a search engine, the search results page enters their places database.  If the user enters a matching string to that results page in the URL bar, it appears as other URLS: a title at the top and a URL string at the bottom.

However, this search result is unlike other URLs.  Navigating to an old search effectively re-performs the search rather than goes to a destination.  This is not apparent in the interface.  Worse, the awesomebar suggestion contains a great deal of unneeded and useless information, such as the very long and unreadable search query URL.

A better design would highlight only the parts of the entry relevant to users: the term, the fact that it's a search, and the search engine.  Specifying that the search is from a user's history is not necessary, since it's within awesomebar results, and could potentially be embarrassing if noted explicitly.

The proposed design simplifies a re-search in awesomebar to the minimal elements meaningful to users: an icon to indicate search, a term, and the search engine.

Acceptance criteria:

1. Favicon of previous search queries is replace with magnifying glass in URL bar
2. Instead of a URL, the blue text underneath a search term shows "[SEARCH ENGINE] Results"
Note: this bug is the engineering counterpart of ux bug 1029849
Depends on: 1029849
Flags: firefox-backlog+
Whiteboard: [search] [qa] → [search] [qa?]
Component: Firefox Operations → Location Bar
Product: Tracking → Firefox
Version: --- → Trunk
Iteration: 33.3 → ---
Points: --- → 8
Whiteboard: [search] [qa?] → [search]
Marco, do we have the infrastructure to identify URLs from previous searches now?

Can you point me briefly to the required changes?
Assignee: nobody → paolo.mozmail
Iteration: --- → 33.3
QA Whiteboard: [qa+]
Flags: needinfo?(mak77)
Added to Iteration 33.3
Status: NEW → ASSIGNED
(In reply to :Paolo Amadini from comment #3)
> Marco, do we have the infrastructure to identify URLs from previous searches
> now?
> 
> Can you point me briefly to the required changes?

Nope, we need to build a code module that starting from a url can tell if it's a search and extract the searchterms from it.
it might need a hosts cache to speed up the lookups.
Flags: needinfo?(mak77)
Also, this should probably be built on top of UnifiedComplete.js that should become the default search, see bug 995091. On the other side that should make things a little easier since everything is in the same component.
finally, notice PriorityUrlProvider.jsm has some search service read code, maybe that could be shared or refactored somehow.
(In reply to Marco Bonardo [:mak] from comment #6)
> Also, this should probably be built on top of UnifiedComplete.js that should
> become the default search, see bug 995091. On the other side that should
> make things a little easier since everything is in the same component.

This would definitely simplify things!

How can I enable the component in local builds? Is the patch on bug 995092 required?
Depends on: 1038225
Depends on: 959582
Blocks: 1038233
Depends on: 1038239
Depends on: 995092
Attached patch Proof of concept (obsolete) — Splinter Review
This is a preliminary patch for starting the discussion of where the code should live and how it should work. On mozilla-central, with browser.urlbar.unifiedcomplete set to true, this patch gives results that are quite close to the mockup.

This filters out irrelevant results. For example, when searching for "firefox" or "utf", all past searches will be included because the string will match the product name or character set that are part of the URL itself. Filtering out these matches from search results is definitely an improvement in usability.
Attachment #8456155 - Flags: feedback?(mak77)
(In reply to :Paolo Amadini from comment #8)
> How can I enable the component in local builds? Is the patch on bug 995092
> required?

sorry for late answer, that patch is recommended, as well as the patches in bug 993372 (first). That's cause those are fixing some bugs. I hope I can land most of them soon, so that the only thing left to do is to flip the pref.
Removed from Iteration 33.3
Status: ASSIGNED → NEW
Iteration: 33.3 → ---
Marco: please drop this from 33.3, work is going on in dependencies.
Flags: needinfo?(mmucci)
(In reply to Marco Mucci [:MarcoM] from comment #11)
> Removed from Iteration 33.3
Flags: needinfo?(mmucci)
Blocks: 1040725
No longer depends on: 1038239
I've split the favicon part in bug 1036916, since it is simple once the rest is fixed, but depends on the UX work for the icon itself.
Summary: Enhance previous searches in awesomebar dropdown by removing URL, replacing favicon with magifying glass → Enhance previous searches in awesomebar dropdown by removing URL
Attached patch Updated proof of concept (obsolete) — Splinter Review
This new patch depends on bug 1040721 and bug 959582 in order, as well as the latest UnifiedComplete patches that are landing now, that must be applied before them.

This proof of concept now works universally for all the currently registered search engines that use a simple GET query.
Attachment #8456155 - Attachment is obsolete: true
Attachment #8456155 - Flags: feedback?(mak77)
Attachment #8458837 - Flags: feedback?(mak77)
Attached image Screenshot
Result of the proof-of-concept patch applied on mozilla-central.
Assignee: paolo.mozmail → nobody
Depends on: 1040721
Attached patch Latest approach (obsolete) — Splinter Review
Updated to use the function defined in the new patch in bug 1040721.

This is still a preliminary patch, the part I'd like feedback on is the placement of the code and how to solve the issue with the value that appears in the address bar when an entry is selected with the keyboard. We could switch "value" and "comment" and invert their positions in the XUL file, or add a new field similar to finalCompleteValue but overriding the displayed value.
Assignee: nobody → paolo.mozmail
Attachment #8458837 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8458837 - Flags: feedback?(mak77)
Attachment #8460235 - Flags: feedback?(mak77)
Iteration: --- → 34.1
Whiteboard: [search] → [search][qa+]
QA Contact: alexandra.lucinet
Comment on attachment 8460235 [details] [diff] [review]
Latest approach

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

::: toolkit/components/places/UnifiedComplete.js
@@ +604,5 @@
>  
> +    // Ensure the search service has been initalized asynchronously before the
> +    // query starts, to avoid hitting the synchronous initialization callback.
> +    // This is done through the PlacesSearchUrlProvider initialization.
> +    PlacesSearchUrlProvider.ensureInitialized();

doesn't initialization already happen the first time we try to access PrioritySearchProvider? At that point we should be fine, otherwise, what's the point of calling this without waiting for it?

@@ +911,5 @@
>                                .getFaviconLinkForIcon(NetUtil.newURI(iconurl)).spec;
>      }
>      match.frecency = frecency;
>  
> +    // Exclude bookmarked searches

I think that I'd rather move this into addMatch, we can filter stuff using style already, or add a further "source" property to the matches.

@@ +913,5 @@
>      match.frecency = frecency;
>  
> +    // Exclude bookmarked searches
> +    if (!match.style || match.style == "favicon") {
> +      let parseUrlResult = Services.search.parseSubmissionURL(url);

How fast is this? I think the price we are going to pay here is quite high.

We need a really fast code path that filters out anything that should NOT be parsed. a filter on the domain should be the best path forward. Nothing should happen until we know the domain is from an installed engine.

The patch I see in bug 1040721 doesn't seem to do that, it has a fast exit path for the scheme and then it tries to parse the params, then it goes through a large loop... We cannot pay this cost in the awesomebar. We need a really quick bailout after a domain lookup from a memory cache.
That may be implemented in the search service (it should make sense) or in a Places module that keeps the engines up to date and does a quick domains lookup (PrioritySearchProvider has an updated list of engines for example)

@@ +915,5 @@
> +    // Exclude bookmarked searches
> +    if (!match.style || match.style == "favicon") {
> +      let parseUrlResult = Services.search.parseSubmissionURL(url);
> +      if (parseUrlResult.engine) {
> +        // Hide the result in case irrelevant portions of the URL were found.

I don't understand this, why should we hide the result, it might still be useful to the user, just that we won't parse it specially...

@@ +922,5 @@
> +          return null;
> +        }
> +        // This unfortunately shows the engine name instead of the search
> +        // terms when the item is selected, though the correct full URI is
> +        // shown when Enter is pressed.

So, I don't think I'm prone to add further stuff in addition to finalCompleteValue, that one was already sort of an hack I'm not completely happy about.
I think we could make some magic assigning a special style to this entry, I don't have a whole idea so far, but we could definitely "replace" the value just in the ui.

@@ +927,5 @@
> +        match.finalCompleteValue = url;
> +        match.value = parseUrlResult.engine.name + " Search";
> +        match.comment = parseUrlResult.terms;
> +        // Will need to show the search icon instead.
> +        //match.icon = PlacesUtils.favicons.defaultFavicon.spec;

we might give it a different style than "favicon"
Attachment #8460235 - Flags: feedback?(mak77)
(In reply to Marco Bonardo [:mak] from comment #18)
> doesn't initialization already happen the first time we try to access
> PrioritySearchProvider? At that point we should be fine, otherwise, what's
> the point of calling this without waiting for it?

This is here to prevent hitting the synchronous initialization path in the Search Service, as the calls for each result are synchronous. The Search Service is indirectly initialized by the component with this Promise-based interface, but we can make this explicit by duplicating the Search Service initialization code here.

> @@ +911,5 @@
> I think that I'd rather move this into addMatch, we can filter stuff using
> style already, or add a further "source" property to the matches.

I'll look into this.

> @@ +913,5 @@
> >      match.frecency = frecency;
> >  
> > +    // Exclude bookmarked searches
> > +    if (!match.style || match.style == "favicon") {
> > +      let parseUrlResult = Services.search.parseSubmissionURL(url);
> 
> How fast is this? I think the price we are going to pay here is quite high.

I think this is pretty fast, especially compared to all the other layout tasks for the items, but I can do some micro-measurements to see how much a hosts cache could help.

> @@ +915,5 @@
> > +        // Hide the result in case irrelevant portions of the URL were found.
> 
> I don't understand this, why should we hide the result, it might still be
> useful to the user, just that we won't parse it specially...

Yeah, we probably shouldn't hide them for now, but just avoid the special styling, to reduce the scope of this bug.

What is confusing for the user, in my opinion, is that when you type "firefox" in the awesome bar you see results for totally unrelated searches at the top (because of the "client=firefox" affiliate code sent with every search), while a search for "firefox download" may appear much lower in the list. We can hide or place these results at the bottom in another bug.

> @@ +922,5 @@
> So, I don't think I'm prone to add further stuff in addition to
> finalCompleteValue, that one was already sort of an hack I'm not completely
> happy about.

After thinking about this, we still need to attach an additional piece of information to the item regardless of styling, that is the search engine name. It is a third value, in addition to the final URL and the search terms.

Also, ideally we would autofill with the full search terms, instead of the URL, while typing? It looks like this can be achieved by placing the search terms in the "value" attribute, and the URL in finalCompleteURL.

> I think we could make some magic assigning a special style to this entry, I
> don't have a whole idea so far, but we could definitely "replace" the value
> just in the ui.

The question is what the code and binding changes will look like. We're also about to do several styling changes in the items. Do we work directly in Toolkit's "autocomplete.xml"? Is it only used in Firefox Desktop?

> @@ +927,5 @@
> we might give it a different style than "favicon"

Yeah, we may reference a fixed icon directly in the binding.
(In reply to :Paolo Amadini from comment #19)
> This is here to prevent hitting the synchronous initialization path in the
> Search Service, as the calls for each result are synchronous. The Search
> Service is indirectly initialized by the component with this Promise-based
> interface, but we can make this explicit by duplicating the Search Service
> initialization code here.

as discussed on IRC should not be needed cause when we arrive at this code we can already tell the SS is initialized, by PrioritySearchProvider.
If we want to make that explicit it should yield.

off-hand I think I'd prefer if all of the search code would be in a separate module and we could disable all of the search features with a pref or a urlbar "behavior". IF we depend on PrioritySearchProvider doing init it might make sense for it to become the AutocompleteSearchHelper module (fancy name, I know) and be "pluggable".

> > @@ +913,5 @@
> > >      match.frecency = frecency;
> > >  
> > > +    // Exclude bookmarked searches
> > > +    if (!match.style || match.style == "favicon") {
> > > +      let parseUrlResult = Services.search.parseSubmissionURL(url);
> > 
> > How fast is this? I think the price we are going to pay here is quite high.
> 
> I think this is pretty fast, especially compared to all the other layout
> tasks for the items, but I can do some micro-measurements to see how much a
> hosts cache could help.

My opinion is that whatever we can gain in the autocomplete, we should do, even if minimal. The user is typing and we are retuning stuff at the same time.

> Yeah, we probably shouldn't hide them for now, but just avoid the special
> styling, to reduce the scope of this bug.

I think it would be fine, yeah.

> What is confusing for the user, in my opinion, is that when you type
> "firefox" in the awesome bar you see results for totally unrelated searches
> at the top (because of the "client=firefox" affiliate code sent with every
> search), while a search for "firefox download" may appear much lower in the
> list. We can hide or place these results at the bottom in another bug.

We should rather tweak frecency to fix the order, rather than hiding things, since we might end up hiding the wrong things.

> Also, ideally we would autofill with the full search terms, instead of the
> URL, while typing? It looks like this can be achieved by placing the search
> terms in the "value" attribute, and the URL in finalCompleteURL.

Sure, it's something that you could experiment with.

> > I think we could make some magic assigning a special style to this entry, I
> > don't have a whole idea so far, but we could definitely "replace" the value
> > just in the ui.
> 
> The question is what the code and binding changes will look like. We're also
> about to do several styling changes in the items. Do we work directly in
> Toolkit's "autocomplete.xml"? Is it only used in Firefox Desktop?

Nope, we should be working in urlbarbindings.xml afaict. autocomplete.xml is used by various products and not only by the awesomebar.
No longer depends on: 1040721
Attached patch Update (obsolete) — Splinter Review
We moved to a more efficient map of domains and paths in bug 1040721, and that should alleviate the performance concerns expressed here.

This patch incorporates most of the current feedback and is based on the helper in bug 959582.

One case that still looks odd is when you see two different autocomplete entries that look exactly the same, when the search URL is different but the terms are equal. This can easily happen when the same search is started from different locations in the browser UI, because the codes in the URL are different. How do you think we should deal with those?
Attachment #8460235 - Attachment is obsolete: true
Flags: needinfo?(mak77)
Attached patch Initial patch (obsolete) — Splinter Review
Went for something simple here that may land without waiting for the improvements in bug 951624, but also doesn't impact that bug significantly, being easy to incorporate there.

The binding should probably look for a special "type" before splitting the title, in this implementation.

There is no localization yet, this means the patch shows "Google" instead of "Google Search", which I think is enough for now. I'm not sure where to get the string from, as it needs to be formatted so probably from a ".properties" file, but this can be defined in a follow-up.

The URI is now shown in the location bar when the item is selected with the keyboard.

The question on how to handle visually similar entries is still open.
Attachment #8463148 - Attachment is obsolete: true
Attachment #8465535 - Flags: feedback?(mak77)
(In reply to :Paolo Amadini from comment #21)
> One case that still looks odd is when you see two different autocomplete
> entries that look exactly the same, when the search URL is different but the
> terms are equal. This can easily happen when the same search is started from
> different locations in the browser UI, because the codes in the URL are
> different. How do you think we should deal with those?

First of all, I think that should be in a follow-up. I see some possibilities:
1. parseSubmissionURL should be able to split out the affiliate codes from the url.
2. we just consider domain and search terms to identify dupes

If we want different affilate code when the locationbar is starting a re-search (this should be clarified with PM) then the only way I see is 1, then we should ask the search service to rebuild a new search url for the given engine and the given search terms explicitly for searches started from the locationbar.

Otherwise we can just repeat the first returned search and discard the others using domain and terms, the second option.
Flags: needinfo?(mak77)
Comment on attachment 8465535 [details] [diff] [review]
Initial patch

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

please add a test in unifiedcomplete/ for the engine parsing

::: toolkit/components/places/UnifiedComplete.js
@@ +736,5 @@
> +  _maybeRestyleSearchMatch: function (match) {
> +    // Don't process matches that are not usual results, for example bookmarks.
> +    if (match.style && match.style != "favicon") {
> +      return;
> +    }

I'd move this condition out of the function

As discussed we need a follow-up for the case a search url is bookmarked, then we'd not restyle it, we should figure whether it's expected or not

@@ +748,5 @@
> +
> +    // Ignore the result in case irrelevant portions of the URL were matched,
> +    // for example "https://www.google.com/search?q=terms&client=firefox" when
> +    // searching for "Firefox".  Always include the URL in case no search is
> +    // in progress, for example when the location bar dropdown button is used.

please clarify the comment as discussed
Attachment #8465535 - Flags: feedback?(mak77) → feedback+
Blocks: 1047433
Blocks: 1047434
Blocks: 1047436
Attached patch The patch (obsolete) — Splinter Review
Filed bug 1047433, bug 1047434, and bug 1047436 as follow-ups.
Attachment #8465535 - Attachment is obsolete: true
Attachment #8466230 - Flags: review?(mak77)
Attached patch The patch with test (obsolete) — Splinter Review
Now with "hg add"!
Attachment #8466230 - Attachment is obsolete: true
Attachment #8466230 - Flags: review?(mak77)
Attachment #8466232 - Flags: review?(mak77)
Comment on attachment 8466232 [details] [diff] [review]
The patch with test

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

::: toolkit/components/places/UnifiedComplete.js
@@ +684,2 @@
>      if (!Prefs.autofillPriority)
>        return;

it is worth to rename this pref to autoFill.searchEngines I think, you basically removed priority word from everywhere and could make sense to allow to disable single providers.

@@ +735,5 @@
>  
> +  _maybeRestyleSearchMatch: function (match) {
> +    // Return if the URL does not represent a search result.
> +    let parseResult = PlacesSearchAutocompleteProvider.parseSubmissionURL(
> +                                                            match.value);

nit: please indent as
let parseResult =
  PlacesSearchAutocompleteProvider.parseSubmissionURL(match.value);

@@ +742,5 @@
> +    }
> +
> +    // Do not apply the special style if the user is doing a search from the
> +    // location bar, but the entered terms match an irrelevant portion of the
> +    // URL, for example "https://www.google.com/search?q=terms&client=firefox"

nit: I'd remove the comma before "but" and put a period after "URL".

::: toolkit/components/places/tests/unifiedcomplete/test_searchEngine.js
@@ +11,5 @@
> +
> +  let uri1 = NetUtil.newURI("http://s.example.com/search?q=Terms&client=1");
> +  let uri2 = NetUtil.newURI("http://s.example.com/search?q=Terms&client=2");
> +  yield promiseAddVisits({ uri: uri1, title: "Terms - SearchEngine Search",
> +                           transition: TRANSITION_TYPED });

doesn't need to be typed (autoFill is not checked)
Attachment #8466232 - Flags: review?(mak77) → review+
The manual verification should check that past searches in the location bar are styled as shown in the attached mockup, with two differences:
* The icon is the favicon of the engine instead of the search icon (bug 1040725)
* The label says "Google" instead of "Google Search" (bug 1047433)
https://hg.mozilla.org/mozilla-central/rev/55f293b3d816
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Iteration: 34.1 → 34.2
QA Contact: alexandra.lucinet → andrei.vaida
Testing performed on Nightly 34.0a1 (2014-08-11) using Windows 7 64-bit, Ubuntu 13.04 32-bit and Mac OS X 10.9.4.

In addition to what has not yet been implemented (Comment 29), there were a few issues/inconsistencies found while comparing the current form of the awesomebar with the one depicted in the mockup, but I believe most of them are already known:
1) the results pane displays duplicates or multiple results for the same search term (bug 1047434)
2) bookmarked searches are displayed with the full URL and label (bug 1047436)
3) the results pane is flickering after typing parts of a previously searched for keyword, for a few times (bug 1047613)
4) "keyword.com" is not displayed in the results pane along with the searches performed for "keyword" if the user received 403 (Forbidden) for that URL
* STR: search for "blabla" (without the quotation marks) on a few different search engines (e.g. Google, Bing, Yahoo) > access "blabla.com" > open a new tab and type blabla
* AR: the only items displayed in the results pane are the searches performed for the "blabla" keyword, while "blabla.com" is missing entirely
5) the following JS error is thrown in the Browser Console after (e.g.) accessing a URL from the location bar, performing a search via search bar, opening the Add-ons Manager:
> chrome://browser/content/browser.xul : Unable to run script because scripts
> are blocked internally.

Paolo, please let me know what are your thoughts on this.
Flags: needinfo?(paolo.mozmail)
Thank you for the thorough testing!

Marco, are issues 4 and 5 already known?
Flags: needinfo?(paolo.mozmail) → needinfo?(mak77)
(In reply to Andrei Vaida, QA [:avaida] from comment #32)
> 4) "keyword.com" is not displayed in the results pane along with the
> searches performed for "keyword" if the user received 403 (Forbidden) for
> that URL
> * STR: search for "blabla" (without the quotation marks) on a few different
> search engines (e.g. Google, Bing, Yahoo) > access "blabla.com" > open a new
> tab and type blabla
> * AR: the only items displayed in the results pane are the searches
> performed for the "blabla" keyword, while "blabla.com" is missing entirely

We don't display history pages that always got 4xx error codes by design.

> 5) the following JS error is thrown in the Browser Console after (e.g.)
> accessing a URL from the location bar, performing a search via search bar,
> opening the Add-ons Manager:
> > chrome://browser/content/browser.xul : Unable to run script because scripts
> > are blocked internally.

I have no idea what this is, but looks like being tracked already: bug 1050358
Flags: needinfo?(mak77)
Paolo, Marco - thank you for your feedback.

This is now verified fixed.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Whiteboard: [search][qa+] → [search]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: