1252823 - Find-in-page doesn't give enough visual feedback when no matches are found

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, enhancement)

Description

[cousteau]

Currently, Firefox for Android doesn't show any visual feedback that it didn't find any match in the page, aside from hiding the match counter ("1/3").  Honestly I hadn't noticed this happened until today; maybe the visual feedback for "no matches" should be clearer.

Option 1: change background of search box to red (as Firefox for desktop does).
Option 2: hide or gray out previous/next buttons
Option 3: show a "Not found" label covering the previous/next buttons

(I think option 1 would be enough; option 2 would help though)

Comment 1

[:Margaret Leibovic]

Let's see if we can do something really simple here. Proposal: how about showing "0/0" for the match count when no matches are found?

Comment 2

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

(In reply to :Margaret Leibovic from comment #1)
> Let's see if we can do something really simple here. Proposal: how about
> showing "0/0" for the match count when no matches are found?

Works for me!

Comment 3

[cousteau]

(In reply to :Margaret Leibovic from comment #1)
> how about showing "0/0" for the match count when no matches are found?

Yes, that'd probably do it.  (Although if the 0/0 could appear highlighted somehow that'd be even better.)

Comment 4

[Tushar Saini (:shatur)]

Attachment: Find-Pagev1.patch

Hi, 
      Displaying 0/0, when there were no matches found, or if matching has been turned off by setting pref accessibility.typeaheadfind.matchesCountLimit to 0.

Regards.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8743962 [details] [diff] [review]
Find-Pagev1.patch

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

Looking good! I just have one small UX concern to address.

::: mobile/android/base/java/org/mozilla/gecko/FindInPageBar.java
@@ +210,5 @@
>                  } else if (total > 0) {
>                      final int current = nativeJSObject.optInt("current", 0);
>                      updateResult(Integer.toString(current) + "/" + Integer.toString(total));
>                  } else {
> +                    // We display 0/0 information, when there were no

Nit: Just say "0/0" (drop the "information").

@@ +215,3 @@
>                      // matches found, or if matching has been turned off by setting
>                      // pref accessibility.typeaheadfind.matchesCountLimit to 0.
> +                    updateResult("0/0");

This is a bit strange because the first time we show the find in page bar, we don't show anything here, but for the rest of the app's lifecycle we'll show "0/0", even when the search field is blank.

I think we should hide this text when the search field is empty.

Comment 6

[Tushar Saini (:shatur)]

Attachment: Find-Pagev2.patch

Hi
  Done changes as asked. Please Review.

Thanks

Comment 7

[:Margaret Leibovic]

Comment on attachment 8744250 [details] [diff] [review]
Find-Pagev2.patch

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

::: mobile/android/base/java/org/mozilla/gecko/FindInPageBar.java
@@ +209,5 @@
>                      updateResult(Integer.toString(limit) + "+");
>                  } else if (total > 0) {
>                      final int current = nativeJSObject.optInt("current", 0);
>                      updateResult(Integer.toString(current) + "/" + Integer.toString(total));
> +                } else if (searchString.equals("")) {

Can searchString be null? Let's use TextUtils.isEmpty(searchString) here just to be safe.

@@ +210,5 @@
>                  } else if (total > 0) {
>                      final int current = nativeJSObject.optInt("current", 0);
>                      updateResult(Integer.toString(current) + "/" + Integer.toString(total));
> +                } else if (searchString.equals("")) {
> +                    updateResult("");

Is this the right place to do this? What happens if the user closes the find bar and then re-opens it?

I haven't built and tested this locally, please make sure it doesn't behave weirdly (e.g. opening the find bar for a subsequent search displays a "0/0".

Comment 8

[Tushar Saini (:shatur)]

Attachment: Find-Pagev3.patch

Hi
 Done changes as asked above.

Regarding This :
> @@ +210,5 @@
> >                  } else if (total > 0) {
> >                      final int current = nativeJSObject.optInt("current", 0);
> >                      updateResult(Integer.toString(current) + "/" + Integer.toString(total));
> > +                } else if (searchString.equals("")) {
> > +                    updateResult("");
> 
> Is this the right place to do this? What happens if the user closes the find
> bar and then re-opens it?

   I have build and tested it locally (above case and many others) and till now I am not facing any weird behavior.

Regards.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8744425 [details] [diff] [review]
Find-Pagev3.patch

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

Great, thanks!

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9d71a501fd32

Comment 12

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2016-05-05-10-51-20.png

Verified as fixed using:
Device: Moto X (Android 4.4)
Build: Firefox for Android 49.0a1 (2016-05-04)