Closed Bug 967982 Opened 7 years ago Closed 5 years ago

Findbar not appearing when Find Again done on non-matching text

Categories

(Toolkit :: Find Toolbar, defect)

27 Branch
x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox27 --- wontfix
firefox28 + fixed
firefox29 + fixed
firefox30 + fixed
firefox45 --- verified
firefox-esr24 --- unaffected
b2g-v1.3 --- fixed

People

(Reporter: starcas25, Assigned: mbrubeck)

References

Details

(Keywords: regression, verifyme, Whiteboard: [e10s])

Attachments

(2 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140127194636

Steps to reproduce:

. Create a new profile
. Go to (eg)
  http://forums.mozillazine.org/viewtopic.php?f=48&t=72994
. Type "/" to open quick findbar
. Type "12345z" (in the quick findbar textbox)
  . The textbox should turn red indicating no match was found on the page
. Hit Esc to close quick findbar
. Hit F3 to "find again"


Actual results:

Fx27: When you hit F3:
. The quick findbar does -not- pop up as it should (nothing happens)
  . But it seems further keystrokes are still directed to it
. This makes it appear that the keyboard has locked up
  . Especially if you have your keys mapped to other functions (with keyconfig)
. eg, Hitting "/" or "'" again does nothing (should open quick findbar/links-only-bar)
. Esc does not help
. Though Ctrl+ and Alt+ key combos and nav keys still work

. To escape the problem, you have to hit Ctrl+F to bring up the regular findbar
  . Here you will see all your hidden keystrokes in the findbar textbox
  . Now everything is back to normal



Expected results:

Fx26: When you hit F3:
. The quick findbar should pop up again with a red textbox indicating no match
Status: UNCONFIRMED → NEW
Component: Untriaged → Find Toolbar
Ever confirmed: true
Keywords: regression
Product: Firefox → Toolkit
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/4a3509325a0f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130913081307
Bad:
http://hg.mozilla.org/mozilla-central/rev/7dbdc0fbda87
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130913130804
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4a3509325a0f&tochange=7dbdc0fbda87


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/32b0c06568e9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130913084305
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/295578d99074
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130913085315
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=32b0c06568e9&tochange=295578d99074

Regressed by:
295578d99074	Tom Schuster — Bug 666816 - Refactor findbar to use a result listener and move most of the logic into a JSM. r=mikedeboer


And backed out bug 666816 Aurora26.0a2 by Bug 916536
So, This bug persist Firefox27 and later.
Blocks: 666816
Whiteboard: [e10s]
Tom, is this something you can prioritize (or find another assignee for) in the next few weeks this is on Beta?
Flags: needinfo?(evilpies)
I think this might be a duplicate. I would prefer to commit to fix this right now. Mike, I am kinda busy, can you take a look?
Flags: needinfo?(evilpies) → needinfo?(mdeboer)
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
Attachment #8377105 - Flags: review?(bmcbride)
Comment on attachment 8377105 [details] [diff] [review]
Patch v1: un-break _setFindCloseTimeout()

Review of attachment 8377105 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but it's not clear to me, which part actually fixes the problem. Removing those !hidden checks before the call?
Attachment #8377105 - Flags: review?(evilpies) → review+
(In reply to Tom Schuster [:evilpie] from comment #5)
> This looks good, but it's not clear to me, which part actually fixes the
> problem. Removing those !hidden checks before the call?

No, it's something different, I should've added my debug & fix trace which is the largest part of the solution.
The problem is triggered by the F3 (find again) action, which the following pseudo-stacktrace:
```
findAgain()::finder.xml
findAgain()::Finder.jsm
_notify()::Finder.jsm
onFindResult()::findbar.xml
_setFindCloseTimeout()::findbar.xml
global.setTimeout()::findbar.xml
```

The callback passed to `setTimeout()` never clears the `findbar._quickFindTimeout` value, which is always set to a number > 0, thus always truthy.
Now, any consecutive keypress is short-circuited by http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#748, because
1. the find mode is not 'normal' and
2. `findbar._quickFindTimeout` is never cleared, thus always truthy.

This is when I decided to conservatively clean up `findbar._setFindCloseTimeout()`, as to prevent similar calamities.
Attachment #8377105 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/de286a964490
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8377105 [details] [diff] [review]
Patch v1: un-break _setFindCloseTimeout()

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 666816
User impact if declined: See bug summary.
Testing completed (on m-c, etc.): baked on m-c for a day.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a
Attachment #8377105 - Flags: approval-mozilla-aurora?
I'm probably missing something, but can I confirm this patch will re-open the findbar on F3? I tried it and while subsequent keypresses were ok, F3 itself did not re-open the findbar (on non-matching text).
Comment on attachment 8377105 [details] [diff] [review]
Patch v1: un-break _setFindCloseTimeout()

Are you considering an uplift to beta too?
Attachment #8377105 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(mdeboer)
Comment on attachment 8377105 [details] [diff] [review]
Patch v1: un-break _setFindCloseTimeout()

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 666816
User impact if declined: See bug summary.
Testing completed (on m-c, etc.): baked on m-a for a while.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a
Attachment #8377105 - Flags: approval-mozilla-beta?
Flags: needinfo?(mdeboer)
Attachment #8377105 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Mike, isn't this fix supposed to reopen the findbar on F3 on a failed match? I'm not seeing this on firefox 29 (aurora).
Flags: needinfo?(mdeboer)
Rick, I'll double-check asap... might need a follow-up patch.
Keywords: verifyme
I can still reproduce this issue with Firefox 28 beta 9 (build ID: 20140306171728) on Win XP x86: while following the STR from comment 0, when I get to the last step (Hit F3 to "find again"), the quick findbar still doesn't pop up as it should (nothing happens).
(In reply to Rick from comment #15)
> Mike, isn't this fix supposed to reopen the findbar on F3 on a failed match?
> I'm not seeing this on firefox 29 (aurora).

(In reply to Manuela Muntean [:Manuela] [QA] from comment #18)
> I get to the last step (Hit F3 to "find again"), the quick findbar still
> doesn't pop up as it should (nothing happens).
Confirmed, this is not fixed - 30.0a1 (2014-03-10), win 7 x64.
Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
There used to be explicit logic in `findbar.onFindAgainCommand` to call `this.open()` if the result was FIND_NOTFOUND.  This logic was deleted in bug 666816 and has not been replaced:

https://hg.mozilla.org/mozilla-central/rev/295578d99074#l6.1513
Attached patch patch (obsolete) — Splinter Review
This patch restores the previous behavior of opening the findbar if findAgain returns FIND_NOTFOUND.

There's one special case in onFindAgainCommand: When there was no previous search for the findbar's current value, it starts a new search by calling _find() instead of _findAgain().  For example, this happens if you do a search in one tab, then open a new tab and immediately press F3.

When this happens, the "findAgain" flag in onFindResult isn't set, since the result wasn't actually from a findAgain call.  Instead, this patch unconditionally opens the findbar when a Find Again command results in a new _find() call.

This is a slight behavior change, which in my opinion is an improvement.  But if we don't want this change, we could instead thread the "findAgain" flag through the "fastFind" methods/messages.

I still need to write tests for this fix.
Assignee: mdeboer → mbrubeck
Attachment #8689201 - Flags: feedback?(mdeboer)
Attached patch follow-up patch v2 (obsolete) — Splinter Review
Removed an extraneous line.
Attachment #8689201 - Attachment is obsolete: true
Attachment #8689201 - Flags: feedback?(mdeboer)
Attachment #8689203 - Flags: feedback?(mdeboer)
(In reply to Matt Brubeck (:mbrubeck) from comment #21)
> There's one special case in onFindAgainCommand: When there was no previous
> search for the findbar's current value, it starts a new search by calling
> _find() instead of _findAgain().  For example, this happens if you do a
> search in one tab, then open a new tab and immediately press F3.
> 
> When this happens, the "findAgain" flag in onFindResult isn't set, since the
> result wasn't actually from a findAgain call.  Instead, this patch
> unconditionally opens the findbar when a Find Again command results in a new
> _find() call.

I disagree with this behavioral change. Hitting F3 should always be considered "finding again", it is an explicit command to re-use the last used search term, whether it comes from that tab, or from another tab in case it's the first time the find bar is used in the current tab.

The fact that there is a _find() call in there is a technicality for the code to cope with the scenario of the user wanting to "find again" when that tab can't; basically it pretends to find again, while backstage it actually starts a search anew.

> 1161 	if (aData.findAgain && aData.result == this.nsITypeAheadFind.FIND_NOTFOUND) {

As per above, this would no longer work as it wouldn't show the findbar if it didn't _findAgain() but did _find() instead.

In this case, I think it can just use the message's .storeResult property as a flag (so the onFindResult doesn't react unnecessarily to highlights). It should work while avoiding the need for a whole new flag to be passed along, and without all the extra routine in onFindAgainCommand.
Food for thought and discussion: open the quick find bar on failed find again, instead of the normal/last-used-mode find bar. For the immediacy of this use-case: "Does this page have x word? if not I'm not going to keep typing in it as that's all I care about."

(I do precisely this in FindBar Tweak, no complaints so far.)
Attached patch follow-up patch v3 (obsolete) — Splinter Review
This uses the simpler approach suggested by quicksaver in comment 23, which shouldn't introduce any new behavior.  It also adds an automated test.
Attachment #8689203 - Attachment is obsolete: true
Attachment #8689203 - Flags: feedback?(mdeboer)
Attachment #8689267 - Flags: review?(mdeboer)
Comment on attachment 8689267 [details] [diff] [review]
follow-up patch v3

Review of attachment 8689267 [details] [diff] [review]:
-----------------------------------------------------------------

Testing the behavior, I have to say that I like it! Thanks Matt!

Could you post a rev 2 with the comments below addressed?

::: toolkit/content/tests/chrome/findbar_window.xul
@@ +563,5 @@
> +      ok(gFindBar.hidden, "Successful Find Again leaves the find bar closed.");
> +    }
> +
> +    function findAndWaitForResult(aText) {
> +      let deferred = Promise.defer();

`Promise.defer` is deprecated in favor of `return new Promise(resolve => { /* do stuff and resolve() */ })`

::: toolkit/content/widgets/findbar.xml
@@ +1151,5 @@
>          -->
>        <method name="onFindResult">
>          <parameter name="aData"/>
>          <body><![CDATA[
> +          if (aData.storeResult && aData.result == this.nsITypeAheadFind.FIND_NOTFOUND) {

Is there a reason to order this clause at the top and not below by extending the existing aData.result == xxx clause?
If possible, I'd prefer that :)
Attachment #8689267 - Flags: review?(mdeboer) → review-
Considering that this patch quite harmless, I'd consider uplifting this as far as we can.
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #26)
> Is there a reason to order this clause at the top and not below by extending
> the existing aData.result == xxx clause?
> If possible, I'd prefer that :)

.open() calls _updateStatusUI(FIND_FOUND). Doing it in there you'd have to call _updateStatusUI one extra time after it was opened to show the correct NOTFOUND status. It's probably simpler to keep this patch at the top of onFindResult because of that, unless I missed something.
(In reply to Luís Miguel [:quicksaver] from comment #28)
> .open() calls _updateStatusUI(FIND_FOUND). Doing it in there you'd have to
> call _updateStatusUI one extra time after it was opened to show the correct
> NOTFOUND status.

Yes, this was my reasoning.  We could move the whole `if` block above the _updateStatusUI call, though.
Addresses review comments.
Attachment #8689267 - Attachment is obsolete: true
Attachment #8689586 - Flags: review?(mdeboer)
Comment on attachment 8689586 [details] [diff] [review]
follow-up patch v4

Review of attachment 8689586 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Thanks for spending time on this, Matt - super appreciated!

::: toolkit/content/widgets/findbar.xml
@@ +1151,5 @@
>          -->
>        <method name="onFindResult">
>          <parameter name="aData"/>
>          <body><![CDATA[
> +          if (aData.result == this.nsITypeAheadFind.FIND_NOTFOUND) {

I was gonna say: r=me with the if-block moved up. But you already did that :-D
Attachment #8689586 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/integration/fx-team/rev/0920dee3c3036861bfe0f32342cdced4cabfb8df
Bug 967982 - Show findbar after unsuccessful Find Again [r=mikedeboer]
https://hg.mozilla.org/mozilla-central/rev/0920dee3c303
Status: REOPENED → RESOLVED
Closed: 7 years ago5 years ago
Resolution: --- → FIXED
I was able to reproduce this issue on Firefox 30.0a1 (2014-02-04) under Windows XP 64-bit.

Verified on Firefox 45.0a1 (2015-12-14) under Windows XP 64-bit and Windows 10 64-bit and I noticed the following:
 - introducing an invalid key (e.g.”12345z”): The search bar is displayed after hitting F3 button 
 - introducing a valid key(e.g “removed”): The search bar is *not* displayed and the results are highlighted one at a time when hitting F3 button  


Matt, could you please confirm that this is the intended behaviour?
Flags: needinfo?(mbrubeck)
Yes, the behavior in comment 35 is the intended behavior.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mbrubeck)
Thanks Matt!
Marking Firefox 45 as Verified Fixed.
Depends on: 1233727
You need to log in before you can comment on or make changes to this bug.