Closed
Bug 1252823
Opened 8 years ago
Closed 8 years ago
Find-in-page doesn't give enough visual feedback when no matches are found
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, enhancement)
Firefox for Android Graveyard
Theme and Visual Design
Tracking
(firefox49 verified, fennec+)
RESOLVED
FIXED
Firefox 49
People
(Reporter: cousteaulecommandant, Assigned: shatur, Mentored)
Details
(Whiteboard: [lang=java])
Attachments
(2 files, 2 obsolete files)
1.99 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
125.67 KB,
image/png
|
Details |
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)
Updated•8 years ago
|
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Comment 1•8 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]
Comment 2•8 years ago
|
||
(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)
(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•8 years ago
|
||
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•8 years ago
|
Assignee: nobody → tushar.saini1285
Comment 5•8 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•8 years ago
|
||
Hi Done changes as asked. Please Review. Thanks
Attachment #8743962 -
Attachment is obsolete: true
Attachment #8744250 -
Flags: review?(margaret.leibovic)
Comment 7•8 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•8 years ago
|
||
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•8 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•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9d71a501fd32
Keywords: checkin-needed
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9d71a501fd32
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 12•8 years ago
|
||
Verified as fixed using: Device: Moto X (Android 4.4) Build: Firefox for Android 49.0a1 (2016-05-04)
Updated•8 years ago
|
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•