1026568 - about:newtab search should show placeholder text, even when logo is showing

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

+++ This bug was initially created as a clone of Bug #962490 +++

warning: bikeshedding risk, reopening old wounds

Boriss, Bryan, others:

on `about:text`, I think placeholder text should show even when the logo is showing.  Even at the risk of reopening this up, and making more patch work.  

Suggestions: 
- last searched string?  (warning: privacy implication)
- a default 'search' or 'search of the day' (warning: complicated?)
- 'search %engine'
- '%engine search'

More cluttered, maybe, but I think actually friendlier for low-skill users.

Comment 1

[Kev Needham [:kev]]

Adding Bill, because explicitly identifying the search field with text like "Search the intarweebs with [Search Provider]" (or a magnifying glass icon) was a thing we found was important even with the "Search" button, but I don't have the data to back it up. Bill?

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Kev Needham [:kev] from comment #1)
> don't have the data to back it up. Bill?

Comment 3

[Bill Selman]

We haven't tested this explicitly, but we do know from our SAP study that some participants did discover through the tasks that they could search in the awesomebar seeing the "Search or enter address" text in the awesomebar. For much of users' sessions, "Search or enter address" is hidden by the address, further diminishing that label as an affordance. For that reason, it would be better to wager on the side of being overly explicit with something like "Search with %defaultsearchengine" in the text field.

Two asides:
1. Having the search engine logo present (and globally set) in the newtab reinforces the visibility of the set default search engine in Firefox. 

Explanation: I've been running pilots of a search diversion study in which we have specifically screened for search diverted participants. We explicitly ask them what they believe their default search engine within Firefox is set to be (not just default in the sense of they use it maybe by navigating there directly or via a bookmark). So far, a portion of participants are not aware that their (diverted) default search engine is different than their desired default chrome search engine. 


2. Since the newtab search field is explicitly a search box, if we want to improve search usage and navigational flow for our users, it is essential to include search suggestions in the search box. 

Explanation: We observed from the SAP study that one of the primary reasons that the search box is used by many users for search and that the awesomebar is not is that searchbox provides search suggestions. Search suggestions are a strong affordance for search and encourage search. Some participants were not aware that the awesomebar had search functionality in the beginning of the tasks and learned that it did later through completing the tasks (since the "Search or enter address text" only appear on newtab or start). Those participants cited the lack of search suggestions in the awesomebar, a search logo, and/or a magnifying glass icon as reasons for their ignorance of this functionality.

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

re: kev's comment 1 "Search the intarweebs with [Search Provider]"

philipp, didn't you have some similar language for the awesome bar search items?

Comment 5

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #4)
> re: kev's comment 1 "Search the intarweebs with [Search Provider]"
> 
> philipp, didn't you have some similar language for the awesome bar search
> items?

Yes, we are using »Search with %engine« and a magnifying glass icon there.

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

Blake looks like he's touched that file last!

Can you change the placeholder text to always show "Search with %engine%"?

Comment 7

[Blake Winton (:bwinton) (:☕️)]

I believe I could, yes.  Which file are we talking about?
(And next iteration should be fine for this change, right?)

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Blake Winton (:bwinton) from comment #7)
> I believe I could, yes.  Which file are we talking about?
> (And next iteration should be fine for this change, right?)

Next one is fine.

Here's the file: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/search.js#201

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug1026568.diff

Well, here's a quick patch that implements the requested behaviour.

As you can see in https://dl.dropboxusercontent.com/u/2301433/Screenshots/NewtabPlaceholder.png, it looks fine for smaller icons like DuckDuckGo, but for the builtin icons where we show the entire name, I think it's a little repetitive, and thus I'm asking Bryan for a ui-review to make sure that's what he was thinking of.

Thanks,
Blake.

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8515020 [details] [diff] [review]
bug1026568.diff

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

Thanks Blake.  Unfortunately it's not quite this simple since we want to e10s'ify newtab (bug 1021654), which means the page will be unprivileged so we can't call Services from it.  (But we can use Services from content script of course.)

Even setting that concern aside, I want the code that handles search features on newtab to be modularized so that other (unprivileged) content pages can use it trivially to have their own search UI (e.g., about:home, bug 1029889).  This patch is tiny so it's not a huge deal, but it does mean that all clients would need to get this string bundle and then get the string themselves.  Ideally it would be handled by the existing content search code.

Here's what we should do.  http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm is the module that handles the chrome side of searching from content.  It performs searches on behalf of content, and it forwards search engine data and events to content so content can build its UI.  We should add a property to the object returned by _currentEngineObj [1].  Call it description or placeholderString.  Its value should be the formatStringFromName() from the bundle.  Then back in search.js, which receives the object returned from _currentEngineObj, _setCurrentEngine would set the placeholder to engine.description.

Sound OK?  The changes I outlined should still make for a pretty small patch.  I'm ready to help if you have questions.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm#389

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 8515020 [details] [diff] [review]
bug1026568.diff

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

I see what you're saying about repetitive among the other inputs but feel like people only focus on / read text in certain areas of focus so if they were looking at the new tab page this being more descriptive is more helpful than the distraction caused when looking at the interface as a whole.

Thanks for getting this done!

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug1026568.diff

Okay, made the changes :adw requested, and carried forward the ui-r+ from :clarkbw.

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 8515221 [details] [diff] [review]
bug1026568.diff

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

::: browser/modules/ContentSearch.jsm
@@ +95,5 @@
>        getService(Ci.nsIMessageListenerManager).
>        addMessageListener(INBOUND_MESSAGE, this);
>      Services.obs.addObserver(this, "browser-search-engine-modified", false);
>      Services.obs.addObserver(this, "shutdown-leaks-before-check", false);
> +    if (!this._stringBundle) {

No need to check this, just set _stringBundle unconditionally.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug1026568.diff

Done and done.
Carrying forward r=adw and ui-r=clarkbw.

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Carsten Book [:Tomcat]]

sorry had to backout for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1071531&repo=fx-team

could you provide a try run before checkin needed is requested again ? Thanks!

Comment 17

[Drew Willcoxon :adw]

Sorry Blake, I forgot about tests.

You'll need to add `placeholder: "Search with " + engine.name` to the object returned here: http://mxr.mozilla.org/mozilla-central/source/browser/modules/test/browser_ContentSearch.js#391

I think that's the only test that needs fixing.  There's browser_newtab_search.js too, but it doesn't actually check the textbox's placeholder, so it doesn't need to be fixed.

Let me know if you'd like me to push your new patch to tryserver.  It's not a problem.

Comment 18

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug1026568.diff

(In reply to Drew Willcoxon :adw from comment #17)
> Sorry Blake, I forgot about tests.

Yeah, so did I.  D'oh!

> You'll need to add placeholder

So, I did this by looking up the string in the bundle, in case we want to run these tests in a different locale…  Would you prefer I hard-code the string?

> Let me know if you'd like me to push your new patch to tryserver.  It's not
> a problem.

Already pushed.  :)
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bd67eaec5c0a

Thanks!

Comment 19

[Drew Willcoxon :adw]

(In reply to Blake Winton (:bwinton) from comment #18)
> So, I did this by looking up the string in the bundle, in case we want to
> run these tests in a different locale…  Would you prefer I hard-code the
> string?

No, that's good.

Thanks Blake!

Comment 20

[Blake Winton (:bwinton) (:☕️)]

Try runs finished, and none of the failures _seem_ related to the code I changed.  Drew, could you double-check those when you review the new patch, and if it's all good, request a checkin?

Comment 21

[Drew Willcoxon :adw]

Comment on attachment 8516083 [details] [diff] [review]
bug1026568.diff

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

Oops, I missed this new r?.  Yes, the failures look unrelated.

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6818a0733662

Comment 24

[Blake Winton (:bwinton) (:☕️)]

Hey Petruta!  Did you need a list of steps to verify this is fixed, or is it reasonably intuitive from the title of the bug?

Comment 25

[Petruta Horea [:phorea], Desktop QA]

Hi Blake, thanks for asking! I'll verify this next week.

I assume I just have to see if the string "Search with <search provider>" is shown inside the search field on newtab page for the default search engines and the ones that are manually installed by user. Also, this text should disappear when starting to type.

Comment 26

[Blake Winton (:bwinton) (:☕️)]

Yep!  :)

Comment 27

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using Nightly 36.0a1 2014-11-06 under Mac OSX 10.9.5, Win 7 64-bit and Ubuntu 14.04 LTS.

Comment 28

[Petruta Horea [:phorea], Desktop QA]

Blake, I believe this bug should be uplifted sooner because bug 1101122 changed the search styling so now it is hard to know which search engine is selected by default (search preferences must be selected first).

What do you think?

Comment 29

[Blake Winton (:bwinton) (:☕️)]

I completely agree.

Comment 30

[Blake Winton (:bwinton) (:☕️)]

Hmm, after talking with people, I'm downgrading my opinion to "I mostly agree, but suspect there's a large enough conversation around it that we won't be able to get it in in time for the beta release."…  Let's see what lands on Nightly in the next couple of weeks, and we can continue the conversation from that point.