Closed Bug 1038789 Opened 6 years ago Closed 6 years ago

Add clear button to search bar in search activity.

Categories

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

All
Android
defect
Not set

Tracking

(firefox34 verified)

VERIFIED FIXED
Firefox 33
Tracking Status
firefox34 --- verified

People

(Reporter: eedens, Assigned: Margaret)

References

Details

Attachments

(1 file)

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 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.
Attachment #8456346 - Flags: review?(eedens) → review+
(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.
I just landed this on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/51964ec6b084

Eric, can you merge the pull request?
All done!

https://github.com/ericedens/FirefoxSearch/commits/master
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/51964ec6b084
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Verified as fixed in build 34.0a1 (2014-08-04);
Device: Samsung Galaxy Nexus (Android 4.2.1).
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.