Closed Bug 928619 Opened 6 years ago Closed 6 years ago

When find bar closes, current match selection is lost

Categories

(Firefox :: General, defect)

27 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
firefox26 --- unaffected
firefox27 + fixed
firefox28 --- verified
firefox29 --- verified

People

(Reporter: quicksaver, Assigned: evilpie)

References

(Depends on 1 open bug)

Details

(Keywords: regression, Whiteboard: [good first verify] [bugday-20140108])

Attachments

(1 file)

Steps to reproduce:
1) Enable FAYT.
2) Search for something; I searched for "event" in https://developer.mozilla.org/en-US/docs/Web/Reference/Events
3) Let the quick find bar close by itself, or close it by hitting Esc.
(all this can be done from the normal find bar as well, by clicking the x button)

The match selection will be lost. At this point, the first time I hit F3 to keep scrolling through the results, it will select the match I was at before; I would expect it to continue from the next one instead.

This is sort of a regression from bug 921308. Before (without Finder.jsm), the same happens but only when the current match is found in a link; the current match is kept if on normal text. I was just about to create a bug to suggest to change that behavior so that the match selection is kept even on links. Good thing I tried the same thing in Nightly, as that leads me to change my suggestion: it should always keep the match selection when closing the find bar.

Obviously, if the match is found on a link, it should still be given focus, but it should also keep the text selection so that when I hit F3, it doesn't start at that very same place I was at, but at the next match.

Is this possible? Maybe by just doing a findAgain() operation right after the focus is set, but I don't know if that'd be a bit too band-aid-hacky...
Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9a8916b16e6e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130913071629
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3734bebc9bfb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130913074304
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9a8916b16e6e&tochange=3734bebc9bfb

Regressed by:
3734bebc9bfb	Tom Schuster — Bug 666816 - Support findbar in e10s. r=mikedeboer
Blocks: 666816
No longer blocks: 921308
Status: UNCONFIRMED → NEW
Component: Find Toolbar → General
Ever confirmed: true
Keywords: regression
Product: Toolkit → Firefox
:gavin/:mike : can you help with how  we plan to handle this case ? Or was this intended as a result of changes in 666816 ?
Flags: needinfo?(mdeboer)
Flags: needinfo?(gavin.sharp)
(In reply to bhavana bajaj [:bajaj] from comment #2)
> :gavin/:mike : can you help with how  we plan to handle this case ? Or was
> this intended as a result of changes in 666816 ?

I'll check it out, I hope have things ready before next uplift.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
Flags: needinfo?(gavin.sharp)
Problem is the call to removeSelection in "blur". The previous code did:
>if (findbar._foundEditable)
>  fastFind.collapseSelection();
>else {
>  fastFind.setSelectionModeAndRepaint(findbar.nsISelectionController.SELECTION_ON);
>}
We should probably do enableSelection. We don't really have a function yet that calls collapse only for editables. I am not sure why that is done. Maybe to avoid accidental edits? I think we should maybe just go for the finder.enableSelection and have consistent behavior.
Assignee: mdeboer → evilpies
Attached patch v1Splinter Review
Attachment #822787 - Flags: review?(mdeboer)
Comment on attachment 822787 [details] [diff] [review]
v1

Review of attachment 822787 [details] [diff] [review]:
-----------------------------------------------------------------

Works! Thanks for working on this.

How hard would it be to write a regression test for this?
Attachment #822787 - Flags: review?(mdeboer) → review+
OS: Windows 7 → All
Hardware: x86_64 → All
I will look at writing a test. Sadly that usually takes longer then fixing the bug and in this case I even need to write clunky test for the whole findbar.

http://hg.mozilla.org/integration/mozilla-inbound/rev/f91d1ac2bc2c
https://hg.mozilla.org/mozilla-central/rev/f91d1ac2bc2c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Blocks: 933710
Do we want to uplift this?
Comment on attachment 822787 [details] [diff] [review]
v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  666816
User impact if declined: Might be annoying when the match selection is lost for no good reason.
Testing completed (on m-c, etc.): Is on m-c.
Risk to taking this patch (and alternatives if risky): Not risky reverts old behavior and code area is not critical anyway.
String or IDL/UUID changes made by this patch: none
Attachment #822787 - Flags: approval-mozilla-aurora?
Attachment #822787 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 922040
Whiteboard: [good first verify]
verified on x86_64 Win7 using the latest nightly. Found not fixed.
Following the steps described in the first post, it works as supposed for me in the latest Nightly, same OS.
Depends on: 952042
I test on nightly 29, linux x86_64. I think the bug is fixed.

[bugday-20140108]
Whiteboard: [good first verify] → [good first verify] [bugday-20140108]
You need to log in before you can comment on or make changes to this bug.