596621 - findbar - need to indicate when no matches found

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

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.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #0)
> background of the field.

I like that

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Color + text + disable

Comment 3

[Wesley Johnston (:wesj)]

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.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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)

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Just color and disable

Comment 7

[Wesley Johnston (:wesj)]

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 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

>+  set status(val) {
>+    if(val != this._status) {
>+      this._status = val;

if (

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Even better

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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)

Comment 12

[Wesley Johnston (:wesj)]

Attachment: nits fixed

Nits addressed.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/778ccd0db269

Comment 14

[Aakash Desai [:aakashd]]

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

Comment 15

[Aakash Desai [:aakashd]]

litmus testcase https://litmus.mozilla.org/show_test.cgi?id=13793 created to regression test this bug.