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

RESOLVED FIXED in Firefox 49

Status

()

Firefox for Android
Theme and Visual Design
--
enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cousteau, Assigned: shatur, Mentored)

Tracking

unspecified
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox49 verified, fennec+)

Details

(Whiteboard: [lang=java])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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)
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true

Comment 1

2 years ago
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?
Mentor: margaret.leibovic
tracking-fennec: ? → +
Flags: needinfo?(alam)
Whiteboard: [lang=java]
(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!
Flags: needinfo?(alam)
(Reporter)

Comment 3

2 years ago
(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.)
(Assignee)

Comment 4

2 years ago
Created attachment 8743962 [details] [diff] [review]
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.
Attachment #8743962 - Flags: review?(margaret.leibovic)

Updated

2 years ago
Assignee: nobody → tushar.saini1285

Comment 5

2 years ago
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.
Attachment #8743962 - Flags: review?(margaret.leibovic) → feedback+
(Assignee)

Comment 6

2 years ago
Created attachment 8744250 [details] [diff] [review]
Find-Pagev2.patch

Hi
  Done changes as asked. Please Review.

Thanks
Attachment #8743962 - Attachment is obsolete: true
Attachment #8744250 - Flags: review?(margaret.leibovic)

Comment 7

2 years ago
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".
Attachment #8744250 - Flags: review?(margaret.leibovic) → feedback+
(Assignee)

Comment 8

2 years ago
Created attachment 8744425 [details] [diff] [review]
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.
Attachment #8744250 - Attachment is obsolete: true
Attachment #8744425 - Flags: review?(margaret.leibovic)

Comment 9

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

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

Great, thanks!
Attachment #8744425 - Flags: review?(margaret.leibovic) → review+

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9d71a501fd32
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Created attachment 8749075 [details]
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)
status-firefox49: fixed → verified
You need to log in before you can comment on or make changes to this bug.