1172071 - Give visual hint for quick search bar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_quicksearch_mock4.png

There's feedback to suggest that the utility of this bar is not immediately apparent.

Let's try adding a first item on the left (as a kind of "label") to show that these are search shortcuts rather than links to webpages. 

As a power user feature, it'd be nice to have this link to the "customize your search providers" page in settings. This will pair nicely with re-enforcing this feature we've always been telling users about.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Note: the "pressable area" of the label is 48dp x 48dp

Mentioned to Mcomella via IRC that we should try tinting and resizing this icon in code so we don't have to add yet another magnifying glass.

So, the icon's size there is 16dp x 16dp, and the color is our disabled grey (#BFBFBF), pressed remains the same.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

This looks like a good follow-up to the things I have done before.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[Tracking Requested - why for this release]: Should ride the trains with the search engine bar landing (bug 1137483).

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: n9-searchbar-label.png

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: n5-searchbar-label.png

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1172071-searchbar-label.patch

This patch adds the label as an item to the search engine bar (see N5/N9 screenshots). This means that the label will also scroll.

@Anthony: Is this okay or should the label be fixed and not scroll?

In this version the label does not handle click events. This will be part of bug 1172083.

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Looks awesome! 

thanks Sebastian!

(In reply to :Sebastian Kaspari from comment #6)
> Created attachment 8623788 [details] [diff] [review]
> 1172071-searchbar-label.patch
> 
> This patch adds the label as an item to the search engine bar (see N5/N9
> screenshots). This means that the label will also scroll.
> 
> @Anthony: Is this okay or should the label be fixed and not scroll?

Having it scroll is intentional so this is perfect!

> In this version the label does not handle click events. This will be part of
> bug 1172083.

Gotcha.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8623788 [details] [diff] [review]
1172071-searchbar-label.patch

I just realized that I broke the code to automatically adjust the width of the buttons. Will upload a fixed patch later.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1172071 - Add icon label to search engine bar. r?mcomella.

Bug 1172071 - Add icon label to search engine bar. r?mcomella.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Adding a tracking flag for FF41.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8624145 [details]
MozReview Request: Bug 1172071 - Add icon label to search engine bar. r?mcomella.

https://reviewboard.mozilla.org/r/11655/#review10313

Looks like it'll work but I think we can clean this up a bit.

::: mobile/android/base/resources/values/colors.xml:165
(Diff revision 1)
> +  <color name="search_engine_bar_label_icon_tint">#FFBFBFBF</color>

Can you use disabled_grey here?

::: mobile/android/base/home/SearchEngineBar.java:188
(Diff revision 1)
> +            iconView.setLayoutParams(new FrameLayout.LayoutParams(labelIconSize, labelIconSize, Gravity.CENTER));

Since we're spending all the effort to overwrite search_engine_bar_item dynamically, I think we should just define it statically despite the slight redundancy.

::: mobile/android/base/home/SearchEngineBar.java:142
(Diff revision 1)
> -            return searchEngines.size();
> +            return searchEngines.size() + 1;

Add a comment saying this offset is for the label.

::: mobile/android/base/home/SearchEngineBar.java:147
(Diff revision 1)
> -            return searchEngines.get(position);
> +            return position == 0 ? null : searchEngines.get(position - 1);

Add a comment saying why we return null for the label.

::: mobile/android/base/home/SearchEngineBar.java:179
(Diff revision 1)
> +            view.setLayoutParams(new LayoutParams(labelContainerWidth, ViewGroup.LayoutParams.MATCH_PARENT));

Probably negligible but you can reuse the old layout params (getLayoutParams) to save an allocation (here and below).

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8624145 [details]
MozReview Request: Bug 1172071 - Add icon label to search engine bar. r?mcomella.

Bug 1172071 - Add icon label to search engine bar. r?mcomella.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae9f84788acc

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8624145 [details]
MozReview Request: Bug 1172071 - Add icon label to search engine bar. r?mcomella.

testAddSearchEngine needs to be fixed. It checks the number of items in the adapter.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=568279093f62

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8624145 [details]
MozReview Request: Bug 1172071 - Add icon label to search engine bar. r?mcomella.

Bug 1172071 - Add icon label to search engine bar. r?mcomella.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8624145 [details]
MozReview Request: Bug 1172071 - Add icon label to search engine bar. r?mcomella.

r+: The update just included the nits/test after initial r+

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=3592553&repo=fx-team

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

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> Backed out for bustage.
> https://treeherder.mozilla.org/logviewer.html#?job_id=3592553&repo=fx-team
> 
> https://hg.mozilla.org/integration/fx-team/rev/587531672cd9

Thank you. The suggestion_item_search drawable has been removed in the meantime. Looking for an alternative.

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

suggestion_item_search has been removed in bug 1158275. Replaced it with search_icon_active.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cdcfb0ec9848

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sebastian, this looks like a scary uplift (to chase bug 1137483) - what do you think?

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #24)
> Sebastian, this looks like a scary uplift (to chase bug 1137483) - what do
> you think?

Yeah, agreed. At first I wanted to get this uplifted. But then we also need to uplift your fixing patch (Drawable.mutate()) and there's also bug 1172083 - implementing the click handler. So maybe it should wait another round. This means we'll release the search engine bar without the hint - maybe making the hint unnecessary? But let's see whether the hint + click handler turns out to be useful/helpful in Nightly/42 and then ship this in 42 as enhancement?

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Sebastian Kaspari from comment #25)
> But let's see whether the hint + click handler
> turns out to be useful/helpful in Nightly/42 and then ship this in 42 as
> enhancement?

Works for me!