link only preference not handled correctly

RESOLVED FIXED in mozilla1.9.2

Status

()

Toolkit
Find Toolbar
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla1.9.2
regression
Points:
---
Bug Flags:
wanted1.9.1 ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

9 years ago
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

9 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

9 years ago
Created attachment 365172 [details] [diff] [review]
Excise broken code
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #365172 - Flags: review?(gavin.sharp)
(Assignee)

Comment 3

9 years ago
Created attachment 365868 [details] [diff] [review]
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.
Attachment #365172 - Attachment is obsolete: true
Attachment #365868 - Flags: review?(gavin.sharp)
Attachment #365172 - Flags: review?(gavin.sharp)
(Assignee)

Comment 4

9 years ago
Created attachment 365869 [details] [diff] [review]
Just the links only fix

As I guess the other patch might be too bug for 1.9.1 :-(
Attachment #365869 - Flags: review?(gavin.sharp)
Attachment #365869 - Flags: review?(gavin.sharp) → review-
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

9 years ago
Created attachment 366187 [details] [diff] [review]
Excise mLinksOnlyPref
Attachment #365869 - Attachment is obsolete: true
Attachment #366187 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

9 years ago
Created attachment 367226 [details] [diff] [review]
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.
Attachment #365868 - Attachment is obsolete: true
Attachment #367226 - Flags: review?(gavin.sharp)
Attachment #365868 - Flags: review?(gavin.sharp)
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

9 years ago
Created attachment 371120 [details] [diff] [review]
Excise mLinksOnly*
Attachment #366187 - Attachment is obsolete: true
Attachment #371120 - Flags: review?(gavin.sharp)
Attachment #366187 - Flags: review?(gavin.sharp)
(Assignee)

Comment 10

9 years ago
Created attachment 371122 [details] [diff] [review]
Excise mLinksOnly*

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

9 years ago
Created attachment 371124 [details] [diff] [review]
Combined patch

Including the selection fixes.
Attachment #367226 - Attachment is obsolete: true
Attachment #371124 - Flags: review?(gavin.sharp)
Attachment #367226 - Flags: review?(gavin.sharp)
Attachment #371122 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 12

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

Pushed changeset 9bec56d42d2e to mozilla-central.
(Assignee)

Comment 13

9 years ago
Created attachment 371175 [details] [diff] [review]
Fix selection collapsing only

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

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Summary: toolkit typeaheadfind isn't useful → link only preference not handled correctly
Target Milestone: --- → mozilla1.9.2
(Assignee)

Updated

6 years ago
Blocks: 719767
You need to log in before you can comment on or make changes to this bug.