Closed
Bug 967982
Opened 11 years ago
Closed 9 years ago
Findbar not appearing when Find Again done on non-matching text
Categories
(Toolkit :: Find Toolbar, defect)
Tracking
()
VERIFIED
FIXED
mozilla30
People
(Reporter: starcas25, Assigned: mbrubeck)
References
Details
(Keywords: regression, verifyme, Whiteboard: [e10s])
Attachments
(2 files, 3 obsolete files)
3.00 KB,
patch
|
evilpies
:
review+
Unfocused
:
review+
Sylvestre
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
4.84 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
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
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
status-firefox27:
--- → affected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox-esr24:
--- → unaffected
tracking-firefox28:
--- → ?
tracking-firefox29:
--- → ?
tracking-firefox30:
--- → ?
Component: Untriaged → Find Toolbar
Ever confirmed: true
Keywords: regression
Product: Firefox → Toolkit
Comment 1•11 years ago
|
||
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
Updated•11 years ago
|
Whiteboard: [e10s]
Comment 2•11 years ago
|
||
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)
Updated•11 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
Comment 4•11 years ago
|
||
Attachment #8377105 -
Flags: review?(evilpies)
Updated•11 years ago
|
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+
Comment 6•11 years ago
|
||
(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.
Updated•11 years ago
|
Attachment #8377105 -
Flags: review?(bmcbride) → review+
Comment 7•11 years ago
|
||
Comment 8•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment 9•11 years ago
|
||
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?
Reporter | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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+
Updated•11 years ago
|
Flags: needinfo?(mdeboer)
Comment 12•11 years ago
|
||
Comment 13•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8377105 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 14•11 years ago
|
||
Reporter | ||
Comment 15•11 years ago
|
||
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)
Comment 16•11 years ago
|
||
Rick, I'll double-check asap... might need a follow-up patch.
Comment 17•11 years ago
|
||
Updated•11 years ago
|
status-b2g-v1.3:
--- → fixed
Comment 18•11 years ago
|
||
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).
Comment 19•11 years ago
|
||
(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 → ---
Assignee | ||
Comment 20•9 years ago
|
||
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
Assignee | ||
Comment 21•9 years ago
|
||
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)
Assignee | ||
Comment 22•9 years ago
|
||
Removed an extraneous line.
Attachment #8689201 -
Attachment is obsolete: true
Attachment #8689201 -
Flags: feedback?(mdeboer)
Attachment #8689203 -
Flags: feedback?(mdeboer)
Comment 23•9 years ago
|
||
(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.
Comment 24•9 years ago
|
||
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.)
Assignee | ||
Comment 25•9 years ago
|
||
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 26•9 years ago
|
||
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-
Comment 27•9 years ago
|
||
Considering that this patch quite harmless, I'd consider uplifting this as far as we can.
Flags: needinfo?(mdeboer)
Comment 28•9 years ago
|
||
(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.
Assignee | ||
Comment 29•9 years ago
|
||
(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.
Assignee | ||
Comment 30•9 years ago
|
||
Addresses review comments.
Attachment #8689267 -
Attachment is obsolete: true
Attachment #8689586 -
Flags: review?(mdeboer)
Comment 31•9 years ago
|
||
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+
Assignee | ||
Comment 32•9 years ago
|
||
Assignee | ||
Comment 33•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0920dee3c3036861bfe0f32342cdced4cabfb8df
Bug 967982 - Show findbar after unsuccessful Find Again [r=mikedeboer]
Comment 34•9 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 11 years ago → 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Comment 35•9 years ago
|
||
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)
Assignee | ||
Comment 36•9 years ago
|
||
Yes, the behavior in comment 35 is the intended behavior.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mbrubeck)
Comment 37•9 years ago
|
||
Thanks Matt!
Marking Firefox 45 as Verified Fixed.
You need to log in
before you can comment on or make changes to this bug.
Description
•