Closed
Bug 596621
Opened 14 years ago
Closed 14 years ago
findbar - need to indicate when no matches found
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(fennec2.0+)
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0+ | --- |
People
(Reporter: madhava, Assigned: wesj)
Details
Attachments
(1 file, 3 obsolete files)
3.31 KB,
patch
|
vingtetun
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
blocking2.0: --- → ?
Comment 1•14 years ago
|
||
(In reply to comment #0) > background of the field. I like that
Updated•14 years ago
|
blocking2.0: ? → ---
tracking-fennec: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Version: 1.9.2 Branch → Trunk
Updated•14 years ago
|
Assignee: nobody → wjohnston
tracking-fennec: ? → 2.0+
Assignee | ||
Comment 2•14 years ago
|
||
Attachment #476861 -
Flags: review?
Assignee | ||
Comment 3•14 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 4•14 years ago
|
||
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-
Comment 5•14 years ago
|
||
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•14 years ago
|
||
Attachment #477175 -
Flags: review?(21)
Assignee | ||
Comment 7•14 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 8•14 years ago
|
||
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+
Comment 9•14 years ago
|
||
>+ set status(val) {
>+ if(val != this._status) {
>+ this._status = val;
if (
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #476861 -
Attachment is obsolete: true
Attachment #477175 -
Attachment is obsolete: true
Attachment #477221 -
Flags: review?(21)
Assignee | ||
Updated•14 years ago
|
Attachment #477221 -
Flags: review?(21)
Comment 11•14 years ago
|
||
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•14 years ago
|
||
Nits addressed.
Attachment #477221 -
Attachment is obsolete: true
Attachment #484113 -
Flags: review?(21)
Attachment #484113 -
Flags: review?(21) → review+
Comment 13•14 years ago
|
||
http://hg.mozilla.org/mobile-browser/rev/778ccd0db269
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 14•14 years ago
|
||
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?
Updated•14 years ago
|
Flags: in-litmus? → in-litmus?(mozaakash)
Comment 15•14 years ago
|
||
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.
Description
•