1041738 - UX/UI polish for the search activity actionbar and search suggestions

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

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

Attachment: prev_currentstate.png

Splitting off from bug 1022102 as part of a polishing effort for the action bar specifically.

Also attaching a screen shot of what we currently have to easily compare and tweak :)

Comment 1

[:Margaret Leibovic]

I'd like to raise the issue that the inactive state of the search bar in this mock-up really doesn't look like a click target.

The main things I see that we need to work on here are:

* Tweak the background color for the activity (color value?)
* Tweak the inactive state text color (color value?)
* Use a black magnifying glass icon in the active state (right now I only have a gray icon)
* Use an orange underline for the active state

Do we also want to use orange text selection handles to match Fennec? I can copy over the assets for now, and we can remove dupes when we deprecate the github repo build.

I've been trying to dig into where the current background gray color for the activity comes from, but I'm having trouble finding it. It looks like all the fragments just have transparent backgrounds, so the background that's showing is the app's background, but I can't figure out where that's being specified.

Comment 2

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

(In reply to :Margaret Leibovic from comment #1)
> I'd like to raise the issue that the inactive state of the search bar in
> this mock-up really doesn't look like a click target.

Fair point, I'll experiment with some options here. My hope was to really draw focus to the search terms/ history items. Also, since there's not much to do on the page in terms of CTA's that demand the users attention, that keeping it clean and simple would not only lend itself to feeling light and quick but also be just enough.

That being said, I will give this some more thought still. stay tuned! :)

> The main things I see that we need to work on here are:
> 
> * Tweak the background color for the activity (color value?)

Here it is! #EBEBF0

> * Tweak the inactive state text color (color value?)

Roboto light, 15 dp, #777777

Comment 3

[Eric Edens [:eedens]]

> I've been trying to dig into where the current background gray color for the
> activity comes from

The style is specified in AndroidManifest as:

            android:theme="@style/AppTheme"

AppTheme is defined in values/search_styles.xml (older devices) and values-v13/search_styles.xml (newer devices).

Both XML files will need to be updated; something like this would work:

    <style name="AppTheme" parent="@android:style/<theme for sdk level>">
        <item name="android:windowBackground">@android:color/global_background_color</item>
        <item name="android:colorBackground">@android:color/global_background_color</item>
    </style>

Comment 4

[:Margaret Leibovic]

Morphing this to also be about styling the search suggestions, since I decided to just make one big polish pass.

WIP here: https://github.com/leibovic/FirefoxSearch/commit/431ef13d761878d7c1766022610d3a83e9e5508e

Build here: http://people.mozilla.org/~mleibovic/search.apk

Comment 5

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

Attachment: icon_search.zip

Attaching magnifying glass..

Comment 6

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

Attachment: icon_x.zip

...and the x

Comment 7

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

In case the border is hard to catch, I thought I'd try to be helpful and provide the color code: #BFBFBF

Also, if it helps - the margins in the mock up are as follows:)
 - on either side are 15dp
 - in between each "card" are 7 dp
 - The first card is 68 dp away from the bottom of the top menu bar.

Comment 8

[:Margaret Leibovic]

Attachment: Style search bar and suggestions

Here's a cleaned up version of my patch that addresses some spacing and color issues. Outstanding issues from feedback that I will address in follow-up bugs (issues that haven't already been filed as other bugs):

* Clear button shouldn't show unless there is text in the input field
* Active color for cards when they are pressed

Comment 9

[Eric Edens [:eedens]]

Comment on attachment 8461189 [details] [diff] [review]
Style search bar and suggestions

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

Just two small things; looks good!

::: mobile/android/search/java/org/mozilla/search/autocomplete/ClearableEditText.java
@@ +100,5 @@
>          editText.setFocusable(active);
>          editText.setFocusableInTouchMode(active);
>  
> +        final int left = active ? R.drawable.search_icon_active : R.drawable.search_icon_inactive;
> +        editText.setCompoundDrawablesWithIntrinsicBounds(left, 0, 0, 0);

Nit: var name `left` is a bit ambiguous -- initially I thought it was a distance value. Maybe rename (leftDrawable) or skip the var and inline the ternary.

::: mobile/android/search/res/layout/search_auto_complete_row.xml
@@ +20,4 @@
>          android:layout_height="wrap_content"
> +        android:paddingTop="@dimen/card_padding_y"
> +        android:paddingBottom="@dimen/card_padding_y"
> +        android:textSize="16sp"/>

This TextView is the same as search_card_history, right? If so, could we do an <include> here?

Comment 10

[:Margaret Leibovic]

(In reply to Eric Edens [:eedens] from comment #9)
> Comment on attachment 8461189 [details] [diff] [review]
> Style search bar and suggestions
> 
> Review of attachment 8461189 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just two small things; looks good!
> 
> :::
> mobile/android/search/java/org/mozilla/search/autocomplete/ClearableEditText.
> java
> @@ +100,5 @@
> >          editText.setFocusable(active);
> >          editText.setFocusableInTouchMode(active);
> >  
> > +        final int left = active ? R.drawable.search_icon_active : R.drawable.search_icon_inactive;
> > +        editText.setCompoundDrawablesWithIntrinsicBounds(left, 0, 0, 0);
> 
> Nit: var name `left` is a bit ambiguous -- initially I thought it was a
> distance value. Maybe rename (leftDrawable) or skip the var and inline the
> ternary.

Will do.

> ::: mobile/android/search/res/layout/search_auto_complete_row.xml
> @@ +20,4 @@
> >          android:layout_height="wrap_content"
> > +        android:paddingTop="@dimen/card_padding_y"
> > +        android:paddingBottom="@dimen/card_padding_y"
> > +        android:textSize="16sp"/>
> 
> This TextView is the same as search_card_history, right? If so, could we do
> an <include> here?

Unfortunately, no, they're not the same. For the search_auto_complete_row, the background is applied to the FrameLayout that hold the TextView and the Button, but for search_card_history, the background is applied to the TextView itself. Also unfortunately, I couldn't apply the same padding to the search_auto_complete_row FrameLayout and the search_card_history TextView, since the jump button takes up a decent amount of vertical space and would make the search suggestion rows too high if there was also padding around it.

Comment 11

[:Margaret Leibovic]

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

PR here for you to merge: https://github.com/ericedens/FirefoxSearch/pull/30

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Richard Newman [:rnewman]]

For the record, this change added 100KB to our APK size.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f9060993657	armv6	28935087
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2176af4ed1	armv6	28934535
https://hg.mozilla.org/integration/mozilla-inbound/rev/b83bb45e9010	armv6	29040131
https://hg.mozilla.org/integration/mozilla-inbound/rev/707fe2b64a85	armv6	29038759
https://hg.mozilla.org/integration/mozilla-inbound/rev/69e731a9f604	armv6	29040811

Comment 14

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #13)
> For the record, this change added 100KB to our APK size.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3f9060993657	armv6
> 28935087
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2176af4ed1	armv6
> 28934535
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b83bb45e9010	armv6
> 29040131
> https://hg.mozilla.org/integration/mozilla-inbound/rev/707fe2b64a85	armv6
> 29038759
> https://hg.mozilla.org/integration/mozilla-inbound/rev/69e731a9f604	armv6
> 29040811

Well, resources will do that. I'm not sure of a way around this if we want things to look a certain way.

In fact, I bet the whole search activity adds a lot to our APK size.

Comment 15

[Richard Newman [:rnewman]]

Very likely!

We should make sure that we're shipping crunched PNGs. Eventually the split APK work will help to reduce the impact.

Comment 16

[Flaviu Cos]

Verified as fixed in build 34.0a1 (2014-08-04);
Devices: Samsung Galaxy Nexus (Android 4.2.1) and Asus Transformer Tab (Android 4.2.1).