findbar - need to indicate when no matches found

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: madhava, Assigned: wesj)

Tracking

Bug Flags:
in-litmus +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Updated

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

Comment 2

7 years ago
Created attachment 476861 [details] [diff] [review]
Color + text + disable
Attachment #476861 - Flags: review?
(Assignee)

Comment 3

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

Comment 6

7 years ago
Created attachment 477175 [details] [diff] [review]
Just color and disable
Attachment #477175 - Flags: review?(21)
(Assignee)

Comment 7

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

Comment 10

7 years ago
Created attachment 477221 [details] [diff] [review]
Even better
Attachment #476861 - Attachment is obsolete: true
Attachment #477175 - Attachment is obsolete: true
Attachment #477221 - Flags: review?(21)
(Assignee)

Updated

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

Comment 12

7 years ago
Created attachment 484113 [details] [diff] [review]
nits fixed

Nits addressed.
Attachment #477221 - Attachment is obsolete: true
Attachment #484113 - Flags: review?(21)
Attachment #484113 - Flags: review?(21) → review+
http://hg.mozilla.org/mobile-browser/rev/778ccd0db269
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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.