Find Again is using a previous search query

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: quicksaver, Assigned: mikedeboer)

Tracking

27 Branch
mozilla30
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox27 wontfix, firefox28 fixed, firefox29 fixed, firefox30 fixed, b2g-v1.3 fixed)

Details

Attachments

(3 attachments)

STR:

1) Clean profile, open about:home
2) Open find bar
3) Type "e" in the find field

4) In the browser console type:
> "gFindBar._findField.value+' '+gFindBar._browser.finder._fastFind.searchString+' '+gFindBar._browser.finder.searchString"

will return as expected:
> "e e e"

5) With your mouse select an "o" character anywhere in the page
6) Hit Ctrl+F, so the find field assumes the "o" value

7) In the browser console type:
> "gFindBar._findField.value+' '+gFindBar._browser.finder._fastFind.searchString+' '+gFindBar._browser.finder.searchString"

will return wrong fastFind. and finder. searchString (kind of expected, since ctrl+F doesn't actually run a search by itself):
> "o e e"

8) Click the button "Highlight All", all the "o"s are correctly highlighted

9) In the browser console type:
> "gFindBar._findField.value+' '+gFindBar._browser.finder._fastFind.searchString+' '+gFindBar._browser.finder.searchString"

will return wrong fastFind.searchString, this is especially bad because of the next step:
> "o e o"

10) Hit F3. It will find the next "e" and not "o" character! <---

11) In the browser console type:
> "gFindBar._findField.value+' '+gFindBar._browser.finder._fastFind.searchString+' '+gFindBar._browser.finder.searchString"

will return wrong fastFind. and finder. searchString, see what I meant when I said it was bad before:
> "o e e"


Mike, Tom, I'm sorry about CC'ing you both directly, but this is affecting beta and I have a feeling it should be fixed ASAP. Basically, because after step 4 there are no actual fastFind operations, its searchString is never updated, and because the finder.searchString value is actually correct in step 10 (because of the highlight), it is assuming fastFind's is as well, so it proceeds with a findAgain().
Polite reminder, this hit release already.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
I actually ran into another manifestation of this bug, only triggered through usage with my add-on however. This got me thinking that a good failsafe for this would be to, in gFindBar.onFindAgainCommand (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#1113), instead of checking findField.value against finder.searchString, it should check against finder._fastFind.searchString, since ultimately that's what it's being switched for anyway, and it would bypass any "wrong" finder.searchString situation.
I have a feeling this "might" conflict with/regress bug 926033. I'm in a bit of a rush so I can't study it completely to make sure (I can do it later though), but that bug made it so the highlight action also updates the finder's searchString. I just can't remember from reading it if that was purposefully done or just happened by chance because that happens inside _notify() and highlight() happened to call it.

Sorry for the lack of detail, I can check it out later today like I said, if you'd like.

(or is this the purpose of the test you uploaded after?)
(In reply to Luís Miguel [:Quicksaver] from comment #5)
> I have a feeling this "might" conflict with/regress bug 926033. I'm in a bit
> of a rush so I can't study it completely to make sure (I can do it later
> though), but that bug made it so the highlight action also updates the
> finder's searchString. I just can't remember from reading it if that was
> purposefully done or just happened by chance because that happens inside
> _notify() and highlight() happened to call it.
> 
> Sorry for the lack of detail, I can check it out later today like I said, if
> you'd like.
> 
> (or is this the purpose of the test you uploaded after?)

You can double-check if you like, but I made sure it doesn't regress that bug. Thanks for mentioning it!
Attachment #8377112 - Flags: review?(bmcbride)
Attachment #8377112 - Flags: review?(evilpies)
Attachment #8377112 - Flags: review?(bmcbride)
Attachment #8377112 - Flags: review+
Attachment #8377124 - Flags: review?(bmcbride) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #6)
> You can double-check if you like, but I made sure it doesn't regress that
> bug. Thanks for mentioning it!

I believe you of course, I don't think it regresses that ;)

Question: what's the plan, will this be uplifted anywhere, or will it only stay in Nightly and ride the 30 train?
(In reply to Luís Miguel [:Quicksaver] from comment #8)
> Question: what's the plan, will this be uplifted anywhere, or will it only
> stay in Nightly and ride the 30 train?

The plan is to uplift as much as possible. So, 29 for sure and possibly 28.
https://hg.mozilla.org/mozilla-central/rev/2261025fb968
https://hg.mozilla.org/mozilla-central/rev/947db48790fa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8377112 [details] [diff] [review]
Patch v1: do not store searchString upon Highlight

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 666816
User impact if declined: Invalid state of the findbar after using 'Highlight All'
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 #8377112 - Flags: approval-mozilla-aurora?
Comment on attachment 8377124 [details] [diff] [review]
Patch 2: add test for Highlight All mode

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 666816
User impact if declined: Unit test to ensure this feature does not regress again.
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 #8377124 - Flags: approval-mozilla-aurora?
Attachment #8377112 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8377124 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8377112 [details] [diff] [review]
Patch v1: do not store searchString upon Highlight

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 666816
User impact if declined: Invalid state of the findbar after using 'Highlight All'
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 #8377112 - Flags: approval-mozilla-beta?
Comment on attachment 8377124 [details] [diff] [review]
Patch 2: add test for Highlight All mode

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 666816
User impact if declined: Unit test to ensure this feature does not regress again.
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 #8377124 - Flags: approval-mozilla-beta?
Attachment #8377112 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8377124 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Patch 1 has conflicts on beta that I'm not sure how to resolve.
Flags: needinfo?(mdeboer)
Oops, looks like I already pushed this, without adding a comment in this bug!

http://hg.mozilla.org/releases/mozilla-aurora/rev/694327d3251e
Flags: needinfo?(mdeboer)
o wait, this is beta. hrm.
Beta branch patch.
Attachment #8383692 - Flags: checkin?(ryanvm)
Just to confirm that I quickly tried the STRs described in the first post and I can't reproduce the bug anymore in either Nightly or today's Beta.
You need to log in before you can comment on or make changes to this bug.