Closed Bug 1172071 Opened 5 years ago Closed 5 years ago

Give visual hint for quick search bar

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox41 --- wontfix
firefox42 --- fixed
fennec 42+ ---

People

(Reporter: antlam, Assigned: sebastian)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
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.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(michael.l.comella)
This looks like a good follow-up to the things I have done before.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Flags: needinfo?(michael.l.comella)
[Tracking Requested - why for this release]: Should ride the trains with the search engine bar landing (bug 1137483).
Attached patch 1172071-searchbar-label.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(alam)
Attachment #8623788 - Flags: review?(michael.l.comella)
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.
Flags: needinfo?(alam)
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.
Attachment #8623788 - Flags: review?(michael.l.comella)
Attachment #8623788 - Attachment is obsolete: true
Bug 1172071 - Add icon label to search engine bar. r?mcomella.
Attachment #8624145 - Flags: review?(michael.l.comella)
Attachment #8624145 - Flags: review?(michael.l.comella) → review+
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 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.
Attachment #8624145 - Flags: review+ → review?(michael.l.comella)
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.
Attachment #8624145 - Flags: review?(michael.l.comella)
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.
Attachment #8624145 - Flags: review?(michael.l.comella)
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+
Attachment #8624145 - Flags: review?(michael.l.comella) → review+
(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.
suggestion_item_search has been removed in bug 1158275. Replaced it with search_icon_active.
https://hg.mozilla.org/mozilla-central/rev/cdcfb0ec9848
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Target Milestone: Firefox 41 → Firefox 42
Sebastian, this looks like a scary uplift (to chase bug 1137483) - what do you think?
Flags: needinfo?(s.kaspari)
(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?
Flags: needinfo?(s.kaspari)
(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!
tracking-fennec: --- → 42+
You need to log in before you can comment on or make changes to this bug.