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

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[(not reading, please use [e-mail] instead)]

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

Comment 1

[Martijn Wargers (dead)]

(Regressions that were caused by a fix for a particular bug should be blocking that particular bug)

Comment 2

[Tony Mechelynck [:tonymec]]

see also bug 406238

Comment 4

[u88484]

requesting blocking due to it being a regression

Comment 7

[Simon Bünzli]

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.

Comment 8

[(not reading, please use [e-mail] instead)]

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.

Comment 9

[Simon Bünzli]

Attachment: remove the offending code

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.

Comment 10

[Seth Spitzer]

Comment on attachment 298032 [details] [diff] [review]
remove the offending code

r=sspitzer

Comment 11

[Seth Spitzer]

thanks for taking care of this one, Simon.

Comment 12

[Reed Loden [:reed]]

Cool, thanks. I'll land this in just a sec.

Comment 13

[Reed Loden [:reed]]

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

Comment 15

[Ria Klaassen (not reading all bugmail)]

Verified fixed.

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=5145

in-litmus+