Closed Bug 596621 Opened 9 years ago Closed 9 years ago

findbar - need to indicate when no matches found

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: madhava, Assigned: wesj)

Details

Attachments

(1 file, 3 obsolete files)

Right now, there's no indication of "no matches found" when using find-in-page.

Disabling the two directional buttons would be an indication.  For something more visible, we could color the border or background of the field.
blocking2.0: --- → ?
(In reply to comment #0)
> background of the field.

I like that
blocking2.0: ? → ---
tracking-fennec: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Version: 1.9.2 Branch → Trunk
Assignee: nobody → wjohnston
tracking-fennec: ? → 2.0+
Attached patch Color + text + disable (obsolete) — Splinter Review
Attachment #476861 - Flags: review?
Comment on attachment 476861 [details] [diff] [review]
Color + text + disable

This colors the bar, disables the buttons, and adds some text to the right hand side reading "Not found". That's maybe more than we wanted, but figured we would give it a whirl.
Attachment #476861 - Flags: review? → review?(21)
Comment on attachment 476861 [details] [diff] [review]
Color + text + disable

This is way too much imo, I think you could just tweak the "FindAssist:Show" case to update the buttons and the input field when there is no results.

Also changing the updateCommands method to handle those cases (empty text, no result)
Attachment #476861 - Flags: review?(21) → review-
Agreed. Let's not add the text. Disabling buttons and changing textbox background should be enough. We explicitly removed the text in an earlier bug.
Attached patch Just color and disable (obsolete) — Splinter Review
Attachment #477175 - Flags: review?(21)
Comment on attachment 477175 [details] [diff] [review]
Just color and disable

Just colors and disables things. I have to call updateCommands in the status change because it fires before receiveMessage does, and hence before we have the status.
Comment on attachment 477175 [details] [diff] [review]
Just color and disable

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
>--- a/chrome/content/browser-ui.js
>+++ b/chrome/content/browser-ui.js
>@@ -1557,21 +1557,34 @@ var BookmarkHelper = {
>     itemIds.forEach(PlacesUtils.bookmarks.removeItem);
> 
>     BrowserUI.updateStar();
>   }
> };
> 
> var FindHelperUI = {
>   type: "find",
>+  _status: 0,
>   commands: {
>     next: "cmd_findNext",
>     previous: "cmd_findPrevious",
>     close: "cmd_findClose"
>   },
>+  
>+  get status() {
>+    return this._status;
>+  },

Nit: move _status: 0 just before the getter and replace 0 by null otherwise it looks like the status is http://mxr.mozilla.org/mozilla-central/source/toolkit/components/typeaheadfind/public/nsITypeAheadFind.idl#106

>+  
>+  set status(val) {
>+    if(val != this._status) {
>+      this._status = val;
>+      this._textbox.setAttribute('status', val);

Nit: use " instead of '

>   updateCommands: function findHelperUpdateCommands(aValue) {
>-    this._cmdPrevious.setAttribute("disabled", aValue == "");
>-    this._cmdNext.setAttribute("disabled", aValue == "");
>+    this._cmdPrevious.setAttribute("disabled", (this.status == Ci.nsITypeAheadFind.FIND_NOTFOUND || aValue == ""));
>+    this._cmdNext.setAttribute("disabled",     (this.status == Ci.nsITypeAheadFind.FIND_NOTFOUND || aValue == ""));
>   },

let's do the work once only:
let disabled =  (this._status == Ci.nsITypeAheadFind.FIND_NOTFOUND || aValue == "");
this._cmdPrevious.setAttribute("disabled", disabled);
this._cmdNext.setAttribute("disabled", disabled);


r+ with nits addressed.
Attachment #477175 - Flags: review?(21) → review+
>+  set status(val) {
>+    if(val != this._status) {
>+      this._status = val;

if (
Attached patch Even better (obsolete) — Splinter Review
Attachment #476861 - Attachment is obsolete: true
Attachment #477175 - Attachment is obsolete: true
Attachment #477221 - Flags: review?(21)
Attachment #477221 - Flags: review?(21)
Comment on attachment 477221 [details] [diff] [review]
Even better


>   updateCommands: function findHelperUpdateCommands(aValue) {
>-    this._cmdPrevious.setAttribute("disabled", aValue == "");
>-    this._cmdNext.setAttribute("disabled", aValue == "");
>+    let disabled = this.status == Ci.nsITypeAheadFind.FIND_NOTFOUND || aValue == "";

This line worry me, why do we have this recursion here?
this.status -> updateCommands -> this.status

Nit: add parenthesis around the expression, this is more readable for the case the expression has nested == (others can disagree with me here)
Attached patch nits fixedSplinter Review
Nits addressed.
Attachment #477221 - Attachment is obsolete: true
Attachment #484113 - Flags: review?(21)
verified FIXED on builds:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101019 Namoroka/4.0b8pre Fennec/4.0b2pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101019 Namoroka/4.0b8pre Fennec/4.0b2pre
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Flags: in-litmus? → in-litmus?(mozaakash)
litmus testcase https://litmus.mozilla.org/show_test.cgi?id=13793 created to regression test this bug.
Flags: in-litmus?(mozaakash) → in-litmus+
You need to log in before you can comment on or make changes to this bug.