Closed
Bug 463294
Opened 16 years ago
Closed 13 years ago
link only preference not handled correctly
Categories
(Toolkit :: Find Toolbar, defect)
Toolkit
Find Toolbar
Tracking
()
RESOLVED
FIXED
mozilla1.9.2
People
(Reporter: neil, Assigned: neil)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(2 files, 7 obsolete files)
5.79 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
8.86 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
(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.
Flags: wanted1.9.1?
Assignee | ||
Comment 2•16 years ago
|
||
Assignee | ||
Comment 3•16 years ago
|
||
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.
Attachment #365172 -
Attachment is obsolete: true
Attachment #365868 -
Flags: review?(gavin.sharp)
Attachment #365172 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 4•16 years ago
|
||
As I guess the other patch might be too bug for 1.9.1 :-(
Attachment #365869 -
Flags: review?(gavin.sharp)
Updated•16 years ago
|
Attachment #365869 -
Flags: review?(gavin.sharp) → review-
Comment 5•16 years ago
|
||
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?
Assignee | ||
Comment 6•16 years ago
|
||
Attachment #365869 -
Attachment is obsolete: true
Attachment #366187 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 7•16 years ago
|
||
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.
Attachment #365868 -
Attachment is obsolete: true
Attachment #367226 -
Flags: review?(gavin.sharp)
Attachment #365868 -
Flags: review?(gavin.sharp)
Comment 8•16 years ago
|
||
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?
Assignee | ||
Comment 9•16 years ago
|
||
Attachment #366187 -
Attachment is obsolete: true
Attachment #371120 -
Flags: review?(gavin.sharp)
Attachment #366187 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 10•16 years ago
|
||
I neatened the constructor slightly.
Attachment #371120 -
Attachment is obsolete: true
Attachment #371122 -
Flags: review?(gavin.sharp)
Attachment #371120 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 11•16 years ago
|
||
Including the selection fixes.
Attachment #367226 -
Attachment is obsolete: true
Attachment #371124 -
Flags: review?(gavin.sharp)
Attachment #367226 -
Flags: review?(gavin.sharp)
Updated•16 years ago
|
Attachment #371122 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 12•16 years ago
|
||
Comment on attachment 371122 [details] [diff] [review]
Excise mLinksOnly*
Pushed changeset 9bec56d42d2e to mozilla-central.
Assignee | ||
Comment 13•16 years ago
|
||
(i.e. without the changes in attachment 371122 [details] [diff] [review])
Attachment #371124 -
Attachment is obsolete: true
Attachment #371175 -
Flags: review?(gavin.sharp)
Attachment #371124 -
Flags: review?(gavin.sharp)
Comment 14•13 years ago
|
||
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.
Attachment #371175 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Summary: toolkit typeaheadfind isn't useful → link only preference not handled correctly
Target Milestone: --- → mozilla1.9.2
You need to log in
before you can comment on or make changes to this bug.
Description
•