1119515 - Implement initial search suggestions

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Search, defect)

Comment 1

[Dale Harvey (:daleharvey)]

A chunk of this is done in https://github.com/daleharvey/gaia/commit/7100f9df3ef836003306f109e7f4375bb10df692#diff-a72719dbedcb33e670a082ee4341d876R10, I need to properly handle the configuration via our existing settings and we should be good

One question, Stas, how do we properly set i10n values for dynamic strings, we will be picking up 'google' via the settings api, and we need to have 'GOOGLE SEARCH' show in the UI, I assume we will have search-google="GOOGLE SEARCH" in en-US properties, but what do I replace https://github.com/daleharvey/gaia/commit/7100f9df3ef836003306f109e7f4375bb10df692#diff-a72719dbedcb33e670a082ee4341d876R10 with? Cheers

Comment 2

[Staś Małolepszy :stas]

If search providers are dynamic, i.e. the complete list if unknown, we should have a generic string that works for most of the cases.  For instance:

  search-header = {{ provider }} Search

  var elem = document.getElementById('suggestion-provider');
  navigator.mozL10n.setAttributes(elem, 'search-header', { provider: searchConfig[key].title });

If some of the providers are known (Google, Yahoo) and we want to have customized titles for them, we could add additional strings just for them:

  search-header = {{ provider }} Search
  search-header-yahoo = Search with Yahoo!

  var elem = document.getElementById('suggestion-provider');
  var l10nId = 'search-header';
  if (KNOWN_PROVIDERS.indexOf(key) !== -1) {
    l10nId += '-' + key;
  }
  navigator.mozL10n.setAttributes(elem, l10nId, { provider: searchConfig[key].title});

Also, looking at the UX spec it seems that the headers are supposed to be uppercase.  Some locales choose not to follow that style.  We should definitely avoid using text-transform: uppercase in CSS or toUpperCase() on the whole string.  This leaves us with something like this:

    navigator.mozL10n.setAttributes(elem, 'search-header', {
      provider: searchConfig[key].title.toUpperCase(),
    });

    search-header = {{ provider }} SEARCH

…which for locales preferring sentence case will look a bit weird (an example in Polish):

    Wyszukiwanie DUCKDUCK GO

Comment 3

[Francesco Lodolo [:flod]]

(In reply to Staś Małolepszy :stas from comment #2)
>   search-header = {{ provider }} Search

This is definitely the safest approach. I expect some locales to end up with {{provider}} in the middle of the sentence too.

This is an example coming from Firefox desktop and its recent search UI redesign
http://transvision.mozfr.org/string/?entity=browser/chrome/browser/search.properties:searchHeader&repo=aurora

English: "%S Search", where %S is replaced with the default search engine name.
Most locales revert the structure, because it sounds unnatural (e.g. Italian uses the equivalent of "Search with %S").

Not a huge fan of uppercase either, even if I usually use the same style English adopts.

Comment 4

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/daleharvey/gaia/commit/646c6be5a2586d74810655c30fb8b2100e5077c6

This is the initial implementation, I havent changed any search data just reused what already exists.

It works as to spec, we need a follow up to fix the search data (yahoo has an incorrect link and we need to add duck duck go)

It also in some cases highlights bugs with the gaia-grid sizing, those I was planning to fix in a follow up

Comment 5

[Kevin Grandon :kgrandon]

Comment on attachment 8546686 [details]
https://github.com/daleharvey/gaia/commit/646c6be5a2586d74810655c30fb8b2100e5077c6

Good start. Left a few minor comments on github.

I also noticed that there was a 2.5x asset in there, where did that come from? I think we generally only ship 2.25x, and not 2.5?

Can you point me to a pull request where I can apply this for the next review round? Thanks!

Comment 6

[Dale Harvey (:daleharvey)]

> I also noticed that there was a 2.5x asset in there, where did that come from? I think we generally only ship 2.25x, and not 2.5?

Icons were picked up from https://mozilla.app.box.com/s/u5j5cb5pt2fd160v9era, Eric do we need a 2.25x here?

Comment 7

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/daleharvey/gaia/commit/1105180fd5d3a24823a09364c715418ebde05277

The PR as a whole is @ https://github.com/mozilla-b2g/gaia/pull/27258, its based on top of https://bugzilla.mozilla.org/show_bug.cgi?id=1117770 which still needs reviewed, will reflag

Due to the way settings are configure, this will work as long as you have visited the search panel in settings before, otherwise we dont have access to the suggestions data, I am addressing that in https://bugzilla.mozilla.org/show_bug.cgi?id=1119820.

Comment 8

[Eric Pang [:epang] UX]

(In reply to Dale Harvey (:daleharvey) from comment #6)
> > I also noticed that there was a 2.5x asset in there, where did that come from? I think we generally only ship 2.25x, and not 2.5?
> 
> Icons were picked up from
> https://mozilla.app.box.com/s/u5j5cb5pt2fd160v9era, Eric do we need a 2.25x
> here?

I believe they are used for the Mai Dai device, so I would keep them in just in case.

Comment 9

[Kevin Grandon :kgrandon]

(In reply to Eric Pang [:epang] from comment #8)
> I believe they are used for the Mai Dai device, so I would keep them in just
> in case.

I don't think this is correct, we've never added 2.5x assets, and we don't have any in trunk? 2.25 is a common resolution which we use, so I think we'll need that instead.

Comment 10

[Eric Pang [:epang] UX]

(In reply to Kevin Grandon :kgrandon [INACTIVE - heads down on Gij Issue] from comment #9)
> (In reply to Eric Pang [:epang] from comment #8)
> > I believe they are used for the Mai Dai device, so I would keep them in just
> > in case.
> 
> I don't think this is correct, we've never added 2.5x assets, and we don't
> have any in trunk? 2.25 is a common resolution which we use, so I think
> we'll need that instead.

My bad, sorry the icons were misnamed.  They are already sized at 2.25x.  I've updated the file names (syncing now).  Sorry for any confusion.
Thanks!

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 8547577 [details]
https://github.com/daleharvey/gaia/commit/1105180fd5d3a24823a09364c715418ebde05277

Left some comments on github and the asset still needs to be renamed. Please address and re-flag me. Thanks!

Comment 12

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/daleharvey/gaia/commit/3ee2f03b3bc7123c5a8c1d3fe00f8769e4f5cd72

Full PR to test in https://github.com/mozilla-b2g/gaia/pull/27258/commits, addressed review comments cheers

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8551832 [details]
https://github.com/daleharvey/gaia/commit/3ee2f03b3bc7123c5a8c1d3fe00f8769e4f5cd72

Looks good to me, but I want to understand the steps to landing this fully before leaving an R+.

Dale - is there any additional patches that we need here? Do we need to default the settings to something in the build system? It seems to not work immediately after applying the patch, so I assume we need another bug for the initial settings value?

Comment 14

[Dale Harvey (:daleharvey)]

https://github.com/mozilla-b2g/gaia/pull/27258 has both the patches needed, once those are applied it should be working if you have google or bing as your provider, its possible due the current config setup that you may need to go into the settings > search, pick bing or google, restart and then it will work. 

The work to fix the config bugs are being done in https://bugzilla.mozilla.org/show_bug.cgi?id=1119820 but not ready yet.

Comment 15

[Dale Harvey (:daleharvey)]

Reviewed in https://bugzilla.mozilla.org/show_bug.cgi?id=1119820, Green try run @ https://github.com/mozilla-b2g/gaia/pull/27625

https://github.com/mozilla-b2g/gaia/commit/f2e9115aa3ba4b6e8bafc3c0622d6c39f08018b9

Comment 17

[Dale Harvey (:daleharvey)]

Pushed to 2.2 in https://github.com/mozilla-b2g/gaia/commit/0f98995c5890232dd01ee3d17183937caec54337

Green try run @ https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=4a6c9879b64e

Comment 18

[Dale Harvey (:daleharvey)]

Comment on attachment 8551832 [details]
https://github.com/daleharvey/gaia/commit/3ee2f03b3bc7123c5a8c1d3fe00f8769e4f5cd72

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 2.2 Feature Development, accidentally already landed without approval, proactively asking for approval
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 19

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

This issue is verified fixed for Flame 3.0

The user can search from Google, Yahoo, Bing, and DuckDuckGo. They will all appear with all uppercase lettering when selected within the search page, and can easily be switched to, from the search page.

Environmental Variables:
Device: Flame 3.0 (319mb)(Kitkat)(Full Flash)
Build ID: 20150210010523
Gaia: 0cf517083f7eb5fc269e1236edba50534f65e3cd
Gecko: 2cb22c058add
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0
---------------------------------------------------------------------------------
Adding verifyme keyword for this bug to be verified on Flame 2.2

Comment 20

[Lance（leave from Mozilla）]

Attachment: Verify_video.mp4

The problem is verified not happen on latest Flame 2.2 build.

Steps:
1. Flash latest Flame 2.2 build.
2. Navigation to homescreen.
3. Tap search box.
4. Input any words.

Actual Result:
4.The user can search from Google, Yahoo, Bing, and DuckDuckGo. They will all appear with all uppercase lettering when selected within the search page, and can easily be switched to, from the search page.


Fail rate:0/5
See attachment:Verify_video.MP4

Flame 2.2 version:
Build ID               20150210002516
Gaia Revision          b30c8e4303595a0fcb5b640d673cf8503b954701
Gaia Date              2015-02-10 04:09:47
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3e9fa4e70a1b
Gecko Version          37.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150210.041059
Firmware Date          Tue Feb 10 04:11:10 EST 2015
Bootloader             L1TC000118D0