Closed Bug 1449338 Opened 6 years ago Closed 6 years ago

Replace search icon with logo of user's search engine

Categories

(Firefox :: Search, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 61
Iteration:
61.3 - Apr 23
Tracking Status
firefox60 --- verified
firefox61 --- verified

People

(Reporter: verdi, Assigned: daleharvey)

References

Details

User Story

https://github.com/mozilla/activity-stream/compare/firefox-60b14...firefox-60b15

Attachments

(6 files)

Attached image logo-spec.png
We think that if people know what search engine they are using (specifically now that they are using Google by default) they will be more likely to use a search access point. Therefore we'd like to replace the generic magnifying glass icon in the new tab search bar with the logo of the user's current search engine.

The logo should occupy an 18px x 18 px box. It should be 8px from the left with a spacer 8px to it's right. See attachment.
As per bug 1449317, we have a small number of users who don't fully understand the search access points in Firefox. 

In this bug, we look to add another visual "nudge" on the new tab page. We know via user testing that some users think the search bar on the new tab page may be for filtering the results displayed on that page. We want users to understand that this is a web search box. We will show the iconography as a small contextual clue
Group: mozilla-employee-confidential
Severity: normal → enhancement
Priority: -- → P3
Component: Activity Streams: Newtab → Search
Priority: P3 → --
Mike asked if I can take this, will take a look into it
Assignee: nobody → dharvey
+Michael Feldman

This one ideally gets uplifted to 60, we'd need to do that next week if possible.
Quick update, so basically I think the plan is to add an <img src="" into https://github.com/mozilla/activity-stream/blob/master/system-addon/content-src/components/Search/Search.jsx

Now in that file we have window.gContentSearchController provided by https://dxr.mozilla.org/mozilla-central/source/browser/base/content/contentSearchUI.js, that contains window.gContentSearchController._defaultEngine.icon which we can use to make the icon, however contentSearchUI doesnt seem to give me hooks for when that data gets set nor when it gets updated

Andrei suggested in IRC that I add the icon to the https://github.com/mozilla/activity-stream/blob/master/system-addon/lib/PrefsFeed.jsm#L91 and keep it updated via there, the icon is not actually contained in a prefs, while getValue seems like it could fetch the value I dont see any mechanism to do manual updates of non pref values, I may try putting browser.search.defaultenginename as the pref to listen to, and use window.gContentSearchController._defaultEngine.icon as the value, however I think there will be timing issues there as contentSearchController needs to do conversions on the icon
What about the normal searchbar in the main UI ? Maybe it should be updated for consistency too ?
I have an in progress PR that only does the new tab page @ https://github.com/mozilla/activity-stream/pull/4096#partial-pull-merging

If we were to update the normal searchbar in the main UI, then I would expect it would be easiest to have https://dxr.mozilla.org/mozilla-central/source/browser/base/content/contentSearchUI.js directly set the input background itself as it has all the information and a reference to the input itself, will do a prototype of that way quickly and see if it works easily
(In reply to Tim Nguyen :ntim from comment #5)
> What about the normal searchbar in the main UI ? Maybe it should be updated
> for consistency too ?
No our intention was not to do that at this time.
Linking Bug 1449317 for reference. Not sure if this is helpful here but in that bug we ended up using a fallback before the search service is initialized, storing the selected search engine at shutdown and limiting things to built-in search engines.
Comment on attachment 8968500 [details]
Bug 1449338 - Show currently selected engine in newtabs search input.

https://reviewboard.mozilla.org/r/237204/#review243016

As per discussion face-to-face, we decided to go with a CSS variable based solution.
Attachment #8968500 - Flags: review?(mdeboer)
Switched both patches to use CSS vars, this is nicer in that it doesnt matter which lands first it wont look broken, mike had suggested using glass.svg as a default (in case the engine has no icon) however that cases the ui to flash from glass.svg -> engine icon which doesnt look great. Its possible to do the fallback in ContentSearchUI.js but mike said to avoid in case it interferes with non as consumers.
Comment on attachment 8968500 [details]
Bug 1449338 - Show currently selected engine in newtabs search input.

https://reviewboard.mozilla.org/r/237204/#review243066

LGTM, thanks!

::: browser/base/content/contentSearchUI.js:614
(Diff revision 2)
>      document.getElementById("contentSearchSettingsButton").textContent =
>        this._strings.searchSettings;
>    },
>  
> +  _updateDefaultEngineIcon() {
> +    document.body.style.setProperty("--newtab-search-icon",

I've seen vars set on `document.documentElement` more often, but this works too, of course.
Attachment #8968500 - Flags: review?(mdeboer) → review+
Awesome work! Two quick questions:

1. Will the new tab icon only appear for engines that ship in Fx or for third-party installed engines too? The partnership teams needs to advise

2. What happens when a user switches search defaults? (i.e. Will there be a slight delay as it loads and then it will process?)
Flags: needinfo?(dharvey)
(In reply to Javaun Moradi [:javaun] from comment #15)
> Awesome work! Two quick questions:
> 
> 1. Will the new tab icon only appear for engines that ship in Fx or for
> third-party installed engines too? The partnership teams needs to advise

With the patches here: all engines, which I would recommend for UX consistency.

> 2. What happens when a user switches search defaults? (i.e. Will there be a
> slight delay as it loads and then it will process?)

That is possible, but the delay should be barely noticeable. Perhaps Dale can post a short screencast?
(In reply to Mike de Boer [:mikedeboer] from comment #16)
> (In reply to Javaun Moradi [:javaun] from comment #15)
> > Awesome work! Two quick questions:
> > 
> > 1. Will the new tab icon only appear for engines that ship in Fx or for
> > third-party installed engines too? The partnership teams needs to advise
> 
> With the patches here: all engines, which I would recommend for UX
> consistency.

In bug Bug 1449317 we limited this to engines that ship in Fx.
(In reply to Verdi [:verdi] from comment #17)
> (In reply to Mike de Boer [:mikedeboer] from comment #16)
> > (In reply to Javaun Moradi [:javaun] from comment #15)
> > > Awesome work! Two quick questions:
> > > 
> > > 1. Will the new tab icon only appear for engines that ship in Fx or for
> > > third-party installed engines too? The partnership teams needs to advise
> > 
> > With the patches here: all engines, which I would recommend for UX
> > consistency.
> 
> In bug Bug 1449317 we limited this to engines that ship in Fx.

That was only due to the uncertainty about the length of the title string for third-party installed engines. The icons shouldn't be an issue.
(In reply to Mark Banner (:standard8) from comment #18)

> That was only due to the uncertainty about the length of the title string
> for third-party installed engines. The icons shouldn't be an issue.

Right but we'll end up in the situation of having a 3rd party icon in the new tab and no name in the address bar 2 inches away. While a built-in engine would have both the icon and the string. To be consistent, the 3rd party should just get the fallbacks. 

Additionally the change was meant to help with hijacking. We could end up with a logo that looks similar enough to a built-in engine to make someone think nothing has changed.
Updated, this time it shows the current glass.svg as a fallback if the engine is not a default engine, using the engine.identifier is not super clear however seems cleanest without exposing _isDefault in the .idl

If there are non activity-stream consumers that are broken by the glass.svg change I could fix them, however it should have no effect on anyone who isnt setup to use --newtab-search-icon

> 2. What happens when a user switches search defaults? (i.e. Will ]
> there be a slight delay as it loads and then it will process?)

Its pretty much immediate as you click the new engine
Flags: needinfo?(dharvey)
Comment on attachment 8968500 [details]
Bug 1449338 - Show currently selected engine in newtabs search input.

Rerequesting due to changes
Attachment #8968500 - Flags: review+ → review?(mdeboer)
Comment on attachment 8968500 [details]
Bug 1449338 - Show currently selected engine in newtabs search input.

https://reviewboard.mozilla.org/r/237204/#review243380

::: browser/base/content/contentSearchUI.js:617
(Diff revision 3)
>    },
>  
> +  _updateDefaultEngineIcon() {
> +    let engine = this._engines.find(engine => engine.name === this.defaultEngine.name);
> +    // We only show the engines icon for default engines, otherwise show
> +    // a default, default engines have an identifier

nit: "; default engines have an identifier."
Attachment #8968500 - Flags: review?(mdeboer) → review+
Priority: -- → P1
Fixed nit, try run showed a minor mochitest + lint failure so fixed those and doing a new try run then will land @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f1569981d0b31e37eb57202951a160af2846b32
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/413f4d055ade
Show currently selected engine in newtabs search input. r=mikedeboer
Apologies, I hadnt pushed my updated patch to mozreview before I pushed, updated patch has a green try run @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f1569981d0b31e37eb57202951a160af2846b32, updated mozreview and will reland, thanks
Flags: needinfo?(dharvey)
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/34a41ccd3a2e
Show currently selected engine in newtabs search input. r=mikedeboer
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/9ea1416143b869005da00dd809290d3e8fbb8bf5
feat(search): Use engine's icon --newtab-search-icon set by contentSearchUI (#4097)

Bug 1449338 - Show currently selected engine in newtabs search input
Blocks: 1455216
https://hg.mozilla.org/mozilla-central/rev/34a41ccd3a2e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment on attachment 8968500 [details]
Bug 1449338 - Show currently selected engine in newtabs search input.

Approval Request Comment
[Feature/Bug causing the regression]: New Feature
[User impact if declined]: Missing feature
[Is this code covered by automated tests?]: Code in general is, new feature isnt
[Has the fix been verified in Nightly?]: Not yet, awaiting dep to land
[Needs manual test from QE? If yes, steps to reproduce]: Change the search engine and observe the icon inside the search input on about:home change
[List of other uplifts needed for the feature/fix]: Needs https://github.com/mozilla/activity-stream/pull/4097 to work
[Is the change risky?]: Nope
[Why is the change risky/not risky?]: It sets a CSS variable thats unused if not consumed, not much change of breaking
[String changes made/needed]: none
Attachment #8968500 - Flags: approval-mozilla-beta?
Depends on: 1454807
User Story: (updated)
User Story: (updated)
Comment on attachment 8969746 [details]
Bug 1449338 - Activity Stream part of Replace search icon with logo of user's search engine.

https://reviewboard.mozilla.org/r/238552/#review244298

Looks good!
Attachment #8969746 - Flags: review?(khudson) → review+
Comment on attachment 8969746 [details]
Bug 1449338 - Activity Stream part of Replace search icon with logo of user's search engine.

This is the patch from comment 33 "Needs https://github.com/mozilla/activity-stream/pull/4097 to work"
Attachment #8969746 - Flags: approval-mozilla-beta?
I'd like QE to verify this in nightly before uplift to beta. Can you fill out a PI request please? (https://mana.mozilla.org/wiki/display/PI/PI+Request) 

It is also quite late in beta to uplift, with only 2 beta builds left before the release candidate build.
Flags: needinfo?(dharvey)
After talking with Javaun, sounds like the product team really wants this for 60. Let's give it a try for beta 15.
Attachment #8968500 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8969746 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
User Story: (updated)
This is failing tests on the beta branch that it wasnt on inbound / central, guessing due to a different search engine configuration hitting a different code path.

This patch puts in a check just to make sure we dont access a non existing engine
Flags: needinfo?(dharvey)
Flags: qe-verify+
I can confirm this issue is fixed, I verified using Fx 61.0a1 (build ID: 20180427102245), and Fx 60.0b16, on Windows 10 x64, mac os 10.13 and Ubuntu 16.06 LTS x86.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.