1199335 - Polish the search engine row object UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Allison Naaktgeboren :ally]

in Bug 1189719, the mocks call for some tweaking of the existing search engine row. Since that has nothing to do with showing search engine history & the search provider, breaking it out to a separate bug.

Comment 1

[Allison Naaktgeboren :ally]

https://bug1189719.bmoattachments.org/attachment.cgi?id=8650667

Comment 2

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

Ally, I'd like to polish up this UI regardless of whether or not the user is getting search history terms. let's make this one block bug 1189719?

Comment 3

[Allison Naaktgeboren :ally]

note that magnification glass can go away.

Comment 4

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

Attachment: spec_awes_searchhistory1b.png

Comment 5

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

Color for pill background: #EBEBF0 (Toolbar grey)
Tint color for clock icon: #AFB1B3 (Tabs tray icon grey)

Comment 6

[Allison Naaktgeboren :ally]

(In reply to Anthony Lam (:antlam) from comment #5)
> Color for pill background: #EBEBF0 (Toolbar grey)
> Tint color for clock icon: #AFB1B3 (Tabs tray icon grey)

check.

Comment 7

[Allison Naaktgeboren :ally]

I don't think we can keep the 80 dp height for the entire search row. I think it only works for single work searches. For search terms that end up being multiple words, you can sometimes get only 2 entry total (one per line). I almost never do the former (single word searches).

Comment 8

[Allison Naaktgeboren :ally]

Attachment: PolishSearchEngineRow-wip

So division, much multiply! I think I've done more math today than I've done in one sitting since college. The android device monitor will give the hierarchy and their bounding boxes in screen pixels. Unfortunately we specify our layout in dp.I have noticed that the exact conversion does not always drop out. Sometimes the reported dimensions end with non integer dp values. padding of 4.5 is probably not in our spec. :)

Number crunching aside, the biggest issue seems to be anonymous relative layout that is the child of homelist view and the parent of suggestion_layout. The two remaining places that we do not match spec deal with this shady character. 

This patch:
matching spec
-search engine icon is 32 dp square 
-inside a 64 dp wide 
-margin between pills is 5dp
-margin around clock icon is 6dp (except left one where it is 6 total)
-margin around pill text is 6dp
-pills are 32 dp high
-font is roboto regular/bold 14sp, text gray
-Color for pill background: #EBEBF0 (Toolbar grey)
-Tint color for clock icon: #AFB1B3 (Tabs tray icon grey)

problems:
- relative layout for entire row, height flexes, 80 dp (I oppose this part. I don't think a hard coded height is a good idea for this part of the ui)
- outer margin is 7dp and I cant see to change that, desired is 5dp

Comment 9

[Allison Naaktgeboren :ally]

Attachment: PolishSearchEngineRow v0

This is has everything in the spec except:

- the top divider. This is due to Bug 1133560 - Underline divider missing from top and second level search results in Android 5.0+ 

- the 80 dp hard height limit. After using this build as my personal browser this weekend, the hard limit is awful for searches on anything more than 7 characters long. Important text and extra words are not visible and all of my search terms would look the same or be unintelligible!

 I am now adamantly opposed to this in practice, though it looks nice enough on the mocks.

Comment 10

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

Can I get a build to test this Ally?

Comment 11

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

Comment on attachment 8660510 [details] [diff] [review]
PolishSearchEngineRow v0

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

::: mobile/android/base/resources/layout/suggestion_item.xml
@@ +3,5 @@
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
>                android:layout_width="wrap_content"
> +              android:layout_height="32dp"

I can't recall the relation between layout_height & minHeight, but intuitively I'd think layout_height should be wrap_content for this to work.

@@ +16,5 @@
>                 android:layout_marginRight="3dip"
>                 android:layout_width="18dip"
>                 android:layout_height="18dip"
> +               android:visibility="gone"
> +               android:backgroundTint="@color/tabs_tray_icon_grey"/>

backgroundTint is API 21 only. You can access these methods dynamically via DrawableCompat (i.e. getBackgroundDrawable() & run DrawableCompat on it). Note that we have a DrawableUtil to make the DrawableCompat operations simpler.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/DrawableUtil.java

Comment 12

[Allison Naaktgeboren :ally]

(In reply to Anthony Lam (:antlam) from comment #10)
> Can I get a build to test this Ally?

Yes. apk is on its way over to you like you requested on irc.

Comment 13

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

Hey ally!

I checked out the APK but I can't see any of these polish items in that build... Can we try another one mayhaps?

Comment 14

[Allison Naaktgeboren :ally]

I think that's the right build. I'm not sure you realize that the existing layout is very close to your requested numbers, most of them already match your requests. 

padding around pills' container 7 -> 5dp
favicon margin 10 -> 16 dp
text color @color/placeholder_active_grey" -> "@color/text_and_tabs_tray_grey"
pills now have a min height of 32

icon tint -> needs to be reworked for version compat according to mcomella's comments.

Note: the divider is missing due to bug Bug 1133560

Comment 15

[Allison Naaktgeboren :ally]

(In reply to Michael Comella (:mcomella) from comment #11)
> Comment on attachment 8660510 [details] [diff] [review]
> PolishSearchEngineRow v0
> 
> Review of attachment 8660510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/layout/suggestion_item.xml
> @@ +3,5 @@
> >     - License, v. 2.0. If a copy of the MPL was not distributed with this
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> >  <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> >                android:layout_width="wrap_content"
> > +              android:layout_height="32dp"
> 
> I can't recall the relation between layout_height & minHeight, but
> intuitively I'd think layout_height should be wrap_content for this to work.

I don't follow. :antlam's mocks call for the pills to have a fixed height of 32dp, why would we need to wrap_content ?

Comment 16

[Allison Naaktgeboren :ally]

Attachment: PolishSearchEngineRow v1

has drawable compat code

Comment 17

[Allison Naaktgeboren :ally]

Attachment: device-2015-09-21-150419.png

Comment 18

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

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #14)
> I think that's the right build. I'm not sure you realize that the existing
> layout is very close to your requested numbers, most of them already match
> your requests. 
> 
> padding around pills' container 7 -> 5dp
> favicon margin 10 -> 16 dp
> text color @color/placeholder_active_grey" ->
> "@color/text_and_tabs_tray_grey"
> pills now have a min height of 32

And what about the pill's color itself? 

> icon tint -> needs to be reworked for version compat according to mcomella's
> comments.

Ok, I didn't know this was not included in the build.

> Note: the divider is missing due to bug Bug 1133560

Also, I noticed that the text in the pills (not typed by the user) is not bolded. Is this a technical limitation that we can't do or is it also just not included in this build?

Just to double check, are the corner's rounded at 4dp as well? 

Thanks Ally, Another build would be great!

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #17)
> Created attachment 8663903 [details]
> device-2015-09-21-150419.png

Comment 19

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

oops, looped this into my last comment

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #17)
> Created attachment 8663903 [details]
> device-2015-09-21-150419.png

I think you forgot the pill's background color ;)

Comment 20

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

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #15)
> I don't follow. :antlam's mocks call for the pills to have a fixed height of
> 32dp, why would we need to wrap_content ?

Both minHeight and layout_height are specified here. If you want a fixed height, you should just use layout_height (and remove minHeight). I think this is what you want.

If you want to use a minHeight (i.e. the View expands based on content but has a minimum height), I'd expect layout_height to be wrap_content and minHeight to be the minimum height.

Comment 21

[Allison Naaktgeboren :ally]

Attachment: device-2015-09-23-140430.png

> Also, I noticed that the text in the pills (not typed by the user) is not bolded. Is this a technical limitation that we can't do or is it also just not included in this build?

Attached is a shot with bold. It doesn't really look like the text in your mock. Are you sure this is what you want?

(yes I know the corners aren't rounded. background color changes are not as straightforward as I might like)

Comment 22

[Allison Naaktgeboren :ally]

(In reply to Anthony Lam (:antlam) from comment #18)
> > icon tint -> needs to be reworked for version compat according to mcomella's
> > comments.
> 
> Ok, I didn't know this was not included in the build.
> 

Well it was, but as mcomella pointed out, it only works in recent versions of android. The shot I posted in comment 21 does have the drawable compat code in it.

> 
> Just to double check, are the corner's rounded at 4dp as well? 

@drawable/selection_selector, http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable/suggestion_selector.xml corner radius is 4dp.

Comment 23

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

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #21)
> Created attachment 8665120 [details]
> device-2015-09-23-140430.png
> 
> > Also, I noticed that the text in the pills (not typed by the user) is not bolded. Is this a technical limitation that we can't do or is it also just not included in this build?
> 
> Attached is a shot with bold. It doesn't really look like the text in your
> mock. Are you sure this is what you want?

Hey Ally, as with comment 4, you'll notice that only what the user has not typed is bolded. Not all. It helps draw attention to what is most relevant I think so bolding it all kind of defeats the purpose.

I also notice that the first "pill" with just "d" in it is taller than the rest.

Comment 24

[Allison Naaktgeboren :ally]

Attachment: device-2015-09-23-174752.png

Comment 25

[Allison Naaktgeboren :ally]

(In reply to Anthony Lam (:antlam) from comment #23)
> (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #21)
> > Created attachment 8665120 [details]
> > device-2015-09-23-140430.png
> > 
> > > Also, I noticed that the text in the pills (not typed by the user) is not bolded. Is this a technical limitation that we can't do or is it also just not included in this build?
> > 
> > Attached is a shot with bold. It doesn't really look like the text in your
> > mock. Are you sure this is what you want?
> 
> Hey Ally, as with comment 4, you'll notice that only what the user has not
> typed is bolded. Not all. It helps draw attention to what is most relevant I
> think so bolding it all kind of defeats the purpose.

That's subproject in its own right. Broken out in Bug 1207845.
> 
> I also notice that the first "pill" with just "d" in it is taller than the
> rest.

There were a number of things wrong with the previous screenshot. I just wanted your take on the bolding bit.

That layout bug has consumed most of my day. It's fixed see screenshot in Comment 24.

Comment 26

[Allison Naaktgeboren :ally]

Attachment: PolishSearchEngineRow v2

While :antlam has a look at the latest screenshot, I'd like you to have a look at the new code in this patch, namely usage of DrawableUtils, and my very first drawable file. While I leaned heavily on the suggestion_selector drawable probably still has a ton of nits I don't even know about.

Comment 27

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

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #24)
> Created attachment 8665199 [details]
> device-2015-09-23-174752.png

Looking soooo much better! :) \o/

Comment 28

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

Comment on attachment 8665203 [details] [diff] [review]
PolishSearchEngineRow v2

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

::: mobile/android/base/home/SearchEngineRow.java
@@ +153,5 @@
>          final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);
> +
> +        // android:backgroundTint only works in Android 21 and higher so we can't do this staticly in the xml
> +        if (isUserSavedSearch) {
> +            final Drawable tintedHistoryIcon = DrawableUtil.tintDrawable(getContext(), R.drawable.icon_most_recent_empty, R.color.tabs_tray_icon_grey);

Correct usage, but I'd cache the returned Drawable in the class instance so you can re-use it instead of creating a new one for each saved suggestion.

::: mobile/android/base/resources/drawable/search_suggestion_button.xml
@@ +1,1 @@
> +<?xml version="1.0" encoding="utf-8"?>

Don't forget the license!

@@ +19,5 @@
> +            <corners android:radius="4dp"/>
> +        </shape>
> +    </item>
> +
> +    <item android:state_enabled="true">

You can specify a default item, i.e. `<item>`. This has the benefit of being explicit about what the state will be if the button gets into a state not listed in this file.

Comment 29

[Allison Naaktgeboren :ally]

Attachment: PolishSearchEngineRow v3

Has all the feedback from 28 and antlam is happy with the current visuals.

Comment 30

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

Comment on attachment 8666541 [details] [diff] [review]
PolishSearchEngineRow v3

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

If it works for The Antlam, it works for me.

::: mobile/android/base/home/SearchEngineRow.java
@@ +68,5 @@
>      // Selected suggestion view
>      private int mSelectedView;
> +    // Clock icon for search history suggestions
> +    // android:backgroundTint only works in Android 21 and higher so we can't do this statically in the xml
> +    private Drawable mTintedHistoryIcon;

nit: I don't think the var needs to include "tinted" in the name (otherwise we'd call everything tinted!). If you make the name more elaborate, you can drop the comment too – e.g. `mSearchHistorySuggestionIcon`

::: mobile/android/base/resources/drawable/search_suggestion_button.xml
@@ +20,5 @@
> +            <corners android:radius="4dp"/>
> +        </shape>
> +    </item>
> +
> +    <item android:state_enabled="true">

You don't need this if you have the default state below (at least, I assume the default is state_enabled="true"...).

Same w/ state_focused, unless you're separating them to be explicit.

Comment 31

[Pulsebot]

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

Comment 32

[Carsten Book [:Tomcat]]

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