Closed Bug 406179 Opened 12 years ago Closed 12 years ago

delete (or shift/delete) of a row in the rich url bar results does not select the next item after delete

Categories

(Firefox :: Address Bar, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta3

People

(Reporter: moco, Assigned: zeniko)

References

Details

(Keywords: regression)

Attachments

(1 file)

delete (or shift/delete on the mac) of a row in the rich url bar results does not select the next item after delete.
(Regressions that were caused by a fix for a particular bug should be blocking that particular bug)
Blocks: 399664
No longer depends on: 399664
Duplicate of this bug: 406238
requesting blocking due to it being a regression
Flags: blocking-firefox3?
Summary: delete (or shift/delete on the mac) of a row in the rich url bar results does not select the next item after delete → delete (or shift/delete) of a row in the rich url bar results does not select the next item after delete
Duplicate of this bug: 406370
Assignee: nobody → sspitzer
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Target Milestone: --- → Firefox 3 M11
Component: Places → Location Bar and Autocomplete
QA Contact: places → location.bar
Duplicate of this bug: 409868
Seth: What's the following bit for: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/autocomplete.xml&rev=1.103&mark=926-937#914 ?

That's where the already correctly selected next item loses the selection because the richlistitem of the deleted entry is still hanging around (until _appendCurrentResult overrides it just further down - though a tad too late).

Removing it fixes this bug. In case that would cause a different regression, please add a comment to the code itself to make things clearer.
simon, thanks for the detective work.

that change (see https://bugzilla.mozilla.org/attachment.cgi?oldid=290322&action=interdiff&newid=290496&headers=1) was made to address the problem of selection persisting between searches.

(see https://bugzilla.mozilla.org/show_bug.cgi?id=399664#c99)

but, the fix for bug #400671 may have addressed that problem, as we now call this.selectedIndex = -1 when on popuphiding.

if so, we might be able to remove the code you've highlighed and fix this bug without regressing anything.

but I also need to make sure the problem of additional autocomplete results coming in (and calling invalidate()) while the results are open does not clear the selected item.
Seth: Are you still doing reviews?

I've been using this patch for since my comment #7 and haven't noted any regression. Furthermore I've compared the code paths of the two autocomplete popups and I don't see anything similar in the classic implementation.

(In reply to comment #8)
> but I also need to make sure the problem of additional autocomplete results
> coming in (and calling invalidate()) while the results are open does not clear
> the selected item.

That's not what this code would have prevented anyway (can't have an item selected which hasn't been added yet). OTOH the autocomplete controller will itself adjust the index when it inserts new items in between, so nothing we'll have to care about ourselves.
Attachment #298032 - Flags: review?(seth)
Assignee: moco → zeniko
Attachment #298032 - Flags: review?(enndeakin)
Comment on attachment 298032 [details] [diff] [review]
remove the offending code

r=sspitzer
Attachment #298032 - Flags: review?(seth) → review+
thanks for taking care of this one, Simon.
Attachment #298032 - Flags: review?(enndeakin)
Cool, thanks. I'll land this in just a sec.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Flags: in-litmus?
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.107; previous revision: 1.106
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Duplicate of this bug: 411936
Verified fixed.
Status: RESOLVED → VERIFIED
https://litmus.mozilla.org/show_test.cgi?id=5145

in-litmus+
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.