Enhance previous searches in awesomebar dropdown by removing URL

VERIFIED FIXED in Firefox 34

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: jboriss, Assigned: Paolo)

Tracking

Trunk
Firefox 34
x86_64
All
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [search])

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

5 years ago
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"
(Reporter)

Comment 1

5 years ago
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]
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 8

5 years ago
(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?
(Assignee)

Updated

5 years ago
Depends on: 1038225
(Assignee)

Updated

5 years ago
Depends on: 959582
(Assignee)

Updated

5 years ago
Blocks: 1038233
(Assignee)

Updated

5 years ago
Depends on: 1038239
(Assignee)

Updated

5 years ago
Depends on: 995092
(Assignee)

Comment 9

5 years ago
Posted 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)
(Assignee)

Updated

5 years ago
Blocks: 1040725
(Assignee)

Updated

5 years ago
No longer depends on: 1038239
(Assignee)

Comment 14

5 years ago
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
(Assignee)

Comment 15

5 years ago
Posted 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)
(Assignee)

Comment 16

5 years ago
Posted image Screenshot
Result of the proof-of-concept patch applied on mozilla-central.
Assignee: paolo.mozmail → nobody
Depends on: 1040721
(Assignee)

Comment 17

5 years ago
Posted 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)
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 19

5 years ago
(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.
(Assignee)

Updated

5 years ago
No longer depends on: 1040721
(Assignee)

Comment 21

5 years ago
Posted 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)
(Assignee)

Comment 22

5 years ago
Posted 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+
(Assignee)

Updated

5 years ago
Blocks: 1047433
(Assignee)

Updated

5 years ago
Blocks: 1047434
(Assignee)

Updated

5 years ago
Blocks: 1047436
(Assignee)

Updated

5 years ago
Duplicate of this bug: 1038233
(Assignee)

Comment 26

5 years ago
Posted 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)
(Assignee)

Comment 27

5 years ago
Posted 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+
(Assignee)

Comment 29

5 years ago
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
Last Resolved: 5 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)
(Assignee)

Comment 33

5 years ago
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.