Implement initial search suggestions

VERIFIED FIXED in 2.2 S4 (23jan)

Status

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: daleharvey, Assigned: daleharvey)

Tracking

unspecified
2.2 S4 (23jan)
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

5 years ago
No description provided.
Assignee

Updated

5 years ago
Blocks: 1098494
Assignee

Updated

5 years ago
Summary: Implement initial hard coded search suggestions → Implement initial search suggestions
Assignee

Comment 1

5 years ago
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
Flags: needinfo?(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
Flags: needinfo?(stas)
(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.
Assignee

Updated

5 years ago
Assignee: nobody → dale
Assignee

Comment 4

5 years ago
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
Attachment #8546686 - Flags: review?(kgrandon)
Whiteboard: [systemsfe]
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!
Attachment #8546686 - Flags: review?(kgrandon) → feedback+
Assignee

Comment 6

5 years ago
> 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?
Flags: needinfo?(epang)
Assignee

Comment 7

5 years ago
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.
Attachment #8546686 - Attachment is obsolete: true
Attachment #8547577 - Flags: review?(kgrandon)
(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.
Flags: needinfo?(epang)
(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.
Flags: needinfo?(epang)
(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!
Flags: needinfo?(epang)
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!
Attachment #8547577 - Flags: review?(kgrandon)
Assignee

Comment 12

5 years ago
Full PR to test in https://github.com/mozilla-b2g/gaia/pull/27258/commits, addressed review comments cheers
Attachment #8547577 - Attachment is obsolete: true
Attachment #8551832 - Flags: review?(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?
Flags: needinfo?(dale)
Attachment #8551832 - Flags: review?(kgrandon) → feedback+
Assignee

Comment 14

5 years ago
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.
Flags: needinfo?(dale)
Target Milestone: --- → 2.2 S4 (23jan)
blocking-b2g: --- → 2.2?
blocking-b2g: 2.2? → ---
feature-b2g: --- → 2.2+
Assignee

Updated

4 years ago
Duplicate of this bug: 1117969
Assignee

Comment 18

4 years ago
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]:
Attachment #8551832 - Flags: approval-gaia-v2.2?
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Attachment #8551832 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Posted video 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
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.