480262 - Removing/Deleting an auto-complete entry via shift+delete removes last character from the entered text inside a textbox

Status: VERIFIED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090225 Shiretoko/3.1b3pre ID:20090225020421

The text which is entered by users on different websites will be stored and shown as proposal when a word with the same beginning letters is entered. Over time these lists get very long and even unwanted entries appear. The way to delete one of those entries is to use the up/down arraw keys to navigate through the list of proposed words and hit Shift+Delete. The underlying entry gets removed.

Sadly, it's not the only part which is deleted. It looks like that even when the auto-complete popup has processed the event, it is still propagated to the textbox itself. As result the last character is getting removed too.

Steps:
1. Enable "Remember what I enter in forms and the search bar" inside the preferences
2. Open Google home page and search for "Mozilla"
3. Go back and clear the textbox
4. Enter "Moz" in the textbox
5. Press the down arrow key once to select Mozilla
6. Press Shift+Delete

With step 6 not only the auto-complete entry is deleted from the list but also the "z" from "Moz". That shouldn't happen.

Comment 1

[Nickolay_Ponomarev]

While the event propagation also happens on non-mac OS, the "last character is getting removed too." issue is only noticeable on mac, where 'Delete' acts like backspace.

Comment 2

[Ed Lee :Mardak]

gavin: The event isn't being canceled because autocomplete controller only says "cancel = true" if it automatically completes to the default index. Any idea why it doesn't just set _retval to true whenever it's able to RemoveValueAt ?

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#536

Comment 3

[Ed Lee :Mardak]

Attachment: 2 ideas 1 patch

Only one of the changes is needed.... silly ifdefs...

Comment 4

[Ed Lee :Mardak]

Actually.. I didn't look at where it chooses to not handle removing the item if we're actually deleting from the middle of the text and not the end...

Comment 5

[:Gavin Sharp [email: [e-mail]]]

I think we should just take the first fix (always cancel in handleDelete).

Comment 6

[Nickolay_Ponomarev]

Attachment: always cancel the event in nsAutoCompleteController::HandleDelete

This bug affects: form fill on web pages (via satchel - nsFormFillController) and XUL <textbox type="autocomplete/> - the Location bar and the Search bar in particular (via autocomplete.xml). So indeed fixing this in nsAutocompleteController is better than doing this just in autocomplete.xml.

> I didn't look at where it chooses to not handle removing the item if
> we're actually deleting from the middle of the text and not the end...

Being in the middle of the text means the autocomplete popup is not open, so nsAutoCompleteController::HandleDelete returns early right after GetPopupOpen check.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 397450 [details] [diff] [review]
always cancel the event in nsAutoCompleteController::HandleDelete

>diff --git a/toolkit/components/satchel/test/test_form_autocomplete.html b/toolkit/components/satchel/test/test_form_autocomplete.html

>+        if (osString == "Darwin")
>+          doKey("back_space", shiftModifier);
>+        else
>+          doKey("delete", shiftModifier);

This isn't an issue with your changes, but there's no need for the shiftModifier in the "delete" case, given bug 241774.

Comment 8

[Nickolay_Ponomarev]

Don't think so - the change from bug 241774 was undone in bug 325737.

Comment 9

[Nickolay_Ponomarev]

Er, you're right that shiftModifier isn't needed in the case you quoted. This is because this line only runs on Win/Linux, not because of bug 241774. If you don't mind, I won't touch this, since I'm not testing on non-Mac.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #9)
> because this line only runs on Win/Linux, not because of bug 241774

It's because of both :)

It's fine to just leave it as is.

Comment 11

[Nickolay_Ponomarev]

http://hg.mozilla.org/mozilla-central/rev/c1972e85e27f

Comment 12

[[e-mail]]

So this is not a problem when there are multiple entries?

Comment 13

[Nickolay_Ponomarev]

Probably no, but I have no idea what problem you have in mind.

Comment 14

[[e-mail]]

Comment on attachment 397450 [details] [diff] [review]
always cancel the event in nsAutoCompleteController::HandleDelete

Gavin explained to me on IRC that the bug's conditions are more common than I originally realised in which case I would appreciate a branch fix (for SeaMonkey) for what thus appears to be a medium reward low risk patch.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 397450 [details] [diff] [review]
always cancel the event in nsAutoCompleteController::HandleDelete

Then getting it fixed on 1.9.2 would be nice too.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090831 Minefield/3.7a1pre ID:20090831030956

Comment 17

[Nickolay_Ponomarev]

I'm not sure when I'll get to checking it into 1.9.2, so if anyone has time to do that, please do.

I didn't check the patch applies cleanly, but it's simple, so there shouldn't be any problems.

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3a749a6d2e6b

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on 1.9.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a2pre) Gecko/20090915 Namoroka/3.6a2pre ID:20090915041724