463294 - link only preference not handled correctly

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[[e-mail]]

OK, so I'm having trouble thinking of a good summary.

Obviously the idea here is to try to replace Suite's typeaheadfind, which uses internal linkage. Unfortunately I soon discovered that toolkit's typeaheadfind clears the selection even on a failed search, which is not the behaviour we're used to. Additionally, on Windows, typeaheadfind cancels the sound when you do another find, (except when using the system beep, but again that's not default for Suite) so I can't even restore the selection that way.

So, there are two options which would resolve the problem for me:
1. Don't unconditionally clear the selection. There is already code to clear the selection on a successful match in a frame. The caller (find bar) could always call CollapseSelection should you deem it necessary.
2. Don't release the sound instance. This doesn't hurt Firefox because it beeps by default.

Comment 1

[[e-mail]]

(In reply to comment #0)
> 1. Don't unconditionally clear the selection. There is already code to clear
> the selection on a successful match in a frame. The caller (find bar) could
> always call CollapseSelection should you deem it necessary.
I've since noticed that there is a test for a collapsed selection. Unfortunately the test is done after the selection is collapsed, so it always returns true...

I've now also noticed another new bogus behaviour: the parameter for whether to find links is ignored on the very first character; if the preference is set to find links then you always find links whether you like it or not.

It would be really nice if we could get these fixed for 1.9.1 for SeaMonkey.

Comment 2

[[e-mail]]

Attachment: Excise broken code

Comment 3

[[e-mail]]

Attachment: Real selection fix

OK, so it turns out that the bogus Collapse is to work around a bug in GetSearchContainers; the XPFE version collapses the selection to the start except when a) doing a find again forwards or b) to support #ifdef-ed out code that tries to repeat the find for a character when it is repeated (e.g. finding "ff" finds the second "f" rather than the first "ff"). The toolkit version unfortunately only collapses the selection when doing a find again backwards, which is why the Collapse was added to take care of the non-repeating case.

Comment 4

[[e-mail]]

Attachment: Just the links only fix

As I guess the other patch might be too bug for 1.9.1 :-(

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 365869 [details] [diff] [review]
Just the links only fix

This is the only use of mLinksOnlyPref. Since find/findAgain callers can already control this behavior, seems like we should remove all traces of this pref in typeaheadfind code and leave it up to consumers (e.g. findbar.xml), no?

Comment 6

[[e-mail]]

Attachment: Excise mLinksOnlyPref

Comment 7

[[e-mail]]

Attachment: Fixed selection fix

Find again didn't wrap forwards with the previous patch :-(

The bogus CollapseToStart must have been added because it was too difficult to work out what was really going on, so I added a couple more comments.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 366187 [details] [diff] [review]
Excise mLinksOnlyPref

With this it now appears that mLinksOnly is unnecessary... you could just replace it with  aLinksOnly in both cases right?

Comment 9

[[e-mail]]

Attachment: Excise mLinksOnly*

Comment 10

[[e-mail]]

Attachment: Excise mLinksOnly*

I neatened the constructor slightly.

Comment 11

[[e-mail]]

Attachment: Combined patch

Including the selection fixes.

Comment 12

[[e-mail]]

Comment on attachment 371122 [details] [diff] [review]
Excise mLinksOnly*

Pushed changeset 9bec56d42d2e to mozilla-central.

Comment 13

[[e-mail]]

Attachment: Fix selection collapsing only

(i.e. without the changes in attachment 371122 [details] [diff] [review])

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 371175 [details] [diff] [review]
Fix selection collapsing only

I'm sorry I've let this linger for a long time. Do you still need these changes? If you still need them re-request review and I'll find someone to do it. It might be best to do that in a new bug, since this one has been used to track a different fix already.