1038789 - Add clear button to search bar in search activity.

Status: VERIFIED FIXED

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

Description

[Eric Edens [:eedens]]

The search bar needs an "x" button; the behavior isn't completely specced yet. It will either:
 (1) Clear the search bar and keep the search bar focused
 (2) Remove focus from the search bar and send the user back to the dashboard.

As a first pass, let's add the "x" button along with a click handler.

Comment 1

[:Margaret Leibovic]

Attachment: Add clear button to search bar in search activity

PR here: https://github.com/ericedens/FirefoxSearch/pull/22

Comment 2

[Eric Edens [:eedens]]

Comment on attachment 8456346 [details] [diff] [review]
Add clear button to search bar in search activity

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

Looks good!

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +60,5 @@
>      }
>  
>      public void startSearch(String query) {
>          setUrl(Constants.YAHOO_WEB_SEARCH_BASE_URL + Uri.encode(query));
>      }

Nit: I don't think we need to remove this line.

::: mobile/android/search/res/layout/search_auto_complete.xml
@@ +38,5 @@
> +                android:layout_width="26dp"
> +                android:layout_height="26dp"
> +                android:layout_gravity="right|center_vertical"
> +                android:layout_marginRight="3dp"
> +                android:background="@drawable/clear" />

Once the resources get added, you'll want to add the prefix of `search_` -- since the resources get copied into Fennec's resources directory, we use the prefix to avoid collisions.

Comment 3

[:Margaret Leibovic]

(In reply to Eric Edens [:eedens] from comment #2)
> Comment on attachment 8456346 [details] [diff] [review]
> Add clear button to search bar in search activity
> 
> Review of attachment 8456346 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> ::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
> @@ +60,5 @@
> >      }
> >  
> >      public void startSearch(String query) {
> >          setUrl(Constants.YAHOO_WEB_SEARCH_BASE_URL + Uri.encode(query));
> >      }
> 
> Nit: I don't think we need to remove this line.

Oops, I think that must have been generated by the grunt script.

> ::: mobile/android/search/res/layout/search_auto_complete.xml
> @@ +38,5 @@
> > +                android:layout_width="26dp"
> > +                android:layout_height="26dp"
> > +                android:layout_gravity="right|center_vertical"
> > +                android:layout_marginRight="3dp"
> > +                android:background="@drawable/clear" />
> 
> Once the resources get added, you'll want to add the prefix of `search_` --
> since the resources get copied into Fennec's resources directory, we use the
> prefix to avoid collisions.

Oh, good call. Also, not sure why the grunt script didn't move over the new resources into this patch... maybe a bug with the script? You'll see them added in the pull request.

Comment 4

[:Margaret Leibovic]

I just landed this on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/51964ec6b084

Eric, can you merge the pull request?

Comment 5

[Eric Edens [:eedens]]

All done!

https://github.com/ericedens/FirefoxSearch/commits/master

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51964ec6b084

Comment 7

[Flaviu Cos]

Verified as fixed in build 34.0a1 (2014-08-04);
Device: Samsung Galaxy Nexus (Android 4.2.1).