1067888 - Add autocomplete result type for searching via current search engine

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Part of bug 951624. Searching via the current search engine is one of the possible actions of typing into the urlbar. currently that's not exposed anywhere in the UI, so is very opaque as to what's going to actually happen. So we want to add a heuristic and a new result type for when this will happen.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

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

Note what you may think is an unexpected first match in test_tabmatches.js for "abc.com" - we get a searchengine result there, which becomes the expected visiturl result when the patch from bug 1067899 is applied.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Am investigating the test failure that showed up on that above Try run.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.1

Fixed failed test from Try run. Seems I'd accidentally left in a decodeURI() in autocomplete.xml - test coverage now covers that because the code is better exercised with this patch.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.2

Sorry for the churn - I missed a test failure as I thought it was in another bug.

Only difference here is the one-line fix in browser_bug1003461-switchtab-override.js


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

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8492160 [details] [diff] [review]
Patch v1.2

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

The try run is still failing in the override test...

there is nothing blocking on the coding side (apart the double notifyResults call that should be clarified) but you should figure out the purpose param with gavin and the favicon with ux.

::: browser/base/content/test/general/browser_action_searchengine.js
@@ +1,4 @@
> +/**
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + **/

nit: This header is wrong, the right one is 2 lines, please check https://www.mozilla.org/MPL/headers/ (PD is there as well)

@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + **/
> +
> + let gOriginalEngine;

please fix indentation

@@ +4,5 @@
> + **/
> +
> + let gOriginalEngine;
> +
> +add_task(function* () {

please check unified pref

@@ +26,5 @@
> +    return promiseClearHistory();
> +  });
> +
> +  info("Wait for autocomplete")
> +  let searchDeferred = Promise.defer();

nit: we are starting seeing this waitForAutocomplete code path in more tests, maybe should be evaluated for head.js

::: browser/base/content/test/general/browser_autocomplete_a11y_label.js
@@ +54,5 @@
>  
> +  ok(gURLBar.popup.richlistbox.children.length > 1, "Should get at least 2 results");
> +  let result = gURLBar.popup.richlistbox.children[1];
> +  is(result.getAttribute("type"), "action switchtab", "Expect right type attribute");
> +  is(result.label, expectedLabel, "Result a11y label should be as expected");

should this be wrapped by unified complete pref check?

::: browser/base/content/test/general/browser_bug1003461-switchtab-override.js
@@ +42,1 @@
>    EventUtils.synthesizeKey("VK_DOWN" , {});

ditto

::: browser/base/content/urlbarBindings.xml
@@ +322,5 @@
>                } else if (action.type == "keyword") {
>                  url = action.params.url;
> +              } else if (action.type == "searchengine") {
> +                let engine = Services.search.getEngineByName(action.params.engineName);
> +                let submission = engine.getSubmission(action.params.searchQuery);

based on bug 1063530, I guess if here we should use the keyword purpose... I think you should try to ask Gavin about the purpose to use here.

@@ +1028,5 @@
>                    break;
>                  }
> +                case "searchengine": {
> +                  let engine = Services.search.getEngineByName(action.params.engineName);
> +                  let submission = engine.getSubmission(action.params.searchQuery);

ditto

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

Ad I said in the other bug, I think doesn't make much sense to repeat the same localization note for each entry, it just makes this unreadable. I'd just put one note at the top.

::: browser/themes/linux/browser.css
@@ +1508,5 @@
>    padding: 0 3px;
>  }
>  
> +richlistitem[type~="action"][actiontype="searchengine"] > .ac-title-box > .ac-site-icon {
> +    list-style-image: url("chrome://browser/skin/Search.png");

are we using the magnifier instead of the search engine icon?
I wonder if we should rather use the search engine icon and append autocomplete-search.svg as we are doing for past searches, it would be more consistent.
And more generally, I'm not sure if it's still sane to use Search.png vs autocomplete-search.svg

please clarify with ux?

The same is valid for all of the themes.

::: browser/themes/windows/browser.css
@@ +1464,5 @@
> +}
> +
> +richlistitem[type~="action"][actiontype="searchengine"] > .ac-title-box > .ac-extra > .ac-comment {
> +  box-shadow: inset 0 0 1px 1px rgba(208,208,208,0.5);
> +  background-color: rgba(208,208,208,0.3);

please check this has no issues with hi-contrast theme.

::: toolkit/components/places/UnifiedComplete.js
@@ +866,5 @@
> +      finalCompleteValue: this._trimmedOriginalSearchString,
> +      frecency: FRECENCY_SEARCHENGINES_DEFAULT,
> +    });
> +
> +    this.notifyResults(true);

why is this needed? doesn't addMatch do it for you?
if it's really really needed, then I'd prefer if addMatch would have an optional argument to enforce it internally.

::: toolkit/components/places/tests/unifiedcomplete/test_actions.js
@@ +1,4 @@
> +/**
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + **/

ditto on header

@@ +6,5 @@
> +add_task(function* test_actions() {
> +  /* The following actions are already tested elsewhere:
> +   * switchtab - test_tabmatches.js
> +   * keyword - test_keyword_search_actions.js
> +   */

what about making this test specific for the search action then (by renaming it)?

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #6)
> The try run is still failing in the override test...

Bah, fixed the test in the wrong patch.

> nit: we are starting seeing this waitForAutocomplete code path in more
> tests, maybe should be evaluated for head.js

Would you believe I already did this in bug 1066358 and forgot to update this patch to use it? :)

> ::: browser/base/content/test/general/browser_autocomplete_a11y_label.js
> should this be wrapped by unified complete pref check?

The entire test is already gated on that.

> based on bug 1063530, I guess if here we should use the keyword purpose... I
> think you should try to ask Gavin about the purpose to use here.

Oh, didn't know of that bug - thanks for mentioning it. Turns out it's stuck on figuring out some extra details, so I've filed bug 1071361 to handle integrating the two later.

> are we using the magnifier instead of the search engine icon?
> I wonder if we should rather use the search engine icon and append
> autocomplete-search.svg as we are doing for past searches, it would be more
> consistent.
> And more generally, I'm not sure if it's still sane to use Search.png vs
> autocomplete-search.svg

Oh, autocomplete-search.svg is new to me. That landed sometime after I did this styling.

Talked with phlsa about all this:
* No search engine icon for this result, to distinguish it from other search results
* But we do want an icon when we're matching on an alias in bug 1067896, to help with recognition
* Change the string to be "QUERY — Search with ENGINE"

> > +richlistitem[type~="action"][actiontype="searchengine"] > .ac-title-box > .ac-extra > .ac-comment {
> > +  box-shadow: inset 0 0 1px 1px rgba(208,208,208,0.5);
> > +  background-color: rgba(208,208,208,0.3);
> 
> please check this has no issues with hi-contrast theme.

It better have no issues - it's the emphasis styling we already use. But, turns out this is leftover from a previous version of the patch anyway - I'm not putting anything in ac-extra anymore.

> > +    this.notifyResults(true);
> 
> why is this needed? doesn't addMatch do it for you?

Oh, yes, indeed.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/9debd2833863

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

And apparently backed out:
https://hg.mozilla.org/integration/fx-team/rev/be50c9825144

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Try came up clean after a test fix (I messed up during a last-second change), so:

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

Comment 11

[Ed Morley [:emorley]]

Reverted for failures in browser_action_searchengine.js:
https://tbpl.mozilla.org/php/getParsedLog.php?id=48761222&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=48768492&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=48778888&tree=Fx-Team

remote:   https://hg.mozilla.org/integration/fx-team/rev/16eb452e62ba

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/98cdbcb925bb

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98cdbcb925bb

Comment 14

[Andrei Vaida [:avaida]]

Testing performed on Nightly 35.0a1 (2014-09-28) using Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 LTS 32-bit.

Acceptance criteria for this patch:
- A "<keyword> - Search with <engine>" entry is displayed for a specific search keyword typed in the awesomebar.
- The "<keyword> - Search with <engine>" also includes the display of a loop icon.

Potential issues:
1. [All platforms] "<keyword> - Search with Google" is displayed despite using a different search engine.
 * note: confirmed this with multiple search engines and also checked the 'browser.search.defaultenginename' pref
 * screenshot: http://i.imgur.com/qOL2mnc.png
2. [All platforms] The search keyword is missing from "- Search with <engine>" if the suggestions pane has been dismissed before pressing the <enter> key.
 * screenshot: http://i.imgur.com/lziGDnW.png
3. [All platforms] The "<keyword> - Search with <engine>" entry is not displayed if the keyword is typed in a private window.
 * screenshot: http://i.imgur.com/qsYohcm.png

Blair, what's your take on these?

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Andrei Vaida, QA [:avaida] from comment #14)
> Blair, what's your take on these?

3 is bug 1075450. Could you file bugs for the others?

Comment 16

[Andrei Vaida [:avaida]]

(In reply to Blair McBride [:Unfocused] from comment #15)
> (In reply to Andrei Vaida, QA [:avaida] from comment #14)
> > Blair, what's your take on these?
> 
> 3 is bug 1075450. Could you file bugs for the others?

Filed Bug 1075532 and Bug 1075549. Since these issues will be tracked separately, I'm marking this one as verified. Testing details for this patch are available in Comment 14.