963925 - Find Again is using a previous search query

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

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().

Comment 1

[Luís Miguel [:quicksaver] (not reading bugmail)]

Polite reminder, this hit release already.

Comment 2

[Luís Miguel [:quicksaver] (not reading bugmail)]

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.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: do not store searchString upon Highlight

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: add test for Highlight All mode

Comment 5

[Luís Miguel [:quicksaver] (not reading bugmail)]

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?)

Comment 6

[Mike de Boer [:mikedeboer]]

(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!

Comment 7

[Mike de Boer [:mikedeboer]]

Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/2261025fb968
remote:   https://hg.mozilla.org/integration/fx-team/rev/947db48790fa

Comment 8

[Luís Miguel [:quicksaver] (not reading bugmail)]

(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?

Comment 9

[Mike de Boer [:mikedeboer]]

(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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2261025fb968
https://hg.mozilla.org/mozilla-central/rev/947db48790fa

Comment 11

[Mike de Boer [:mikedeboer]]

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

Comment 12

[Mike de Boer [:mikedeboer]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/694327d3251e
https://hg.mozilla.org/releases/mozilla-aurora/rev/90e4b605fdb7

Comment 14

[Mike de Boer [:mikedeboer]]

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

Comment 15

[Mike de Boer [:mikedeboer]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Patch 1 has conflicts on beta that I'm not sure how to resolve.

Comment 17

[Mike de Boer [:mikedeboer]]

Oops, looks like I already pushed this, without adding a comment in this bug!

http://hg.mozilla.org/releases/mozilla-aurora/rev/694327d3251e

Comment 18

[Mike de Boer [:mikedeboer]]

o wait, this is beta. hrm.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Yeah, comment 13 covers aurora.

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1-beta: do not store searchString upon Highlight

Beta branch patch.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8383692 [details] [diff] [review]
Patch v1-beta: do not store searchString upon Highlight

Thanks!

https://hg.mozilla.org/releases/mozilla-beta/rev/a910892387e1
https://hg.mozilla.org/releases/mozilla-beta/rev/860e76ec9fe5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a910892387e1
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/860e76ec9fe5

Comment 23

[Luís Miguel [:quicksaver] (not reading bugmail)]

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.