1105244 - Deleting a character needs pressing [DELETE] key more than twice in Location Bar

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Alice0775 White]

[Tracking Requested - why for this release]:

Steps To Reproduce:
1. Start Nightly with newly created profile
2. Open https://www.mozilla.org/en-US/firefox/nightly/firstrun/
3. Focus Location Bar and Move caret to the left most (Press [HOME] key)
4. Attempt to delete https://

Actual Results:
Press [DELETE] makes ttps://www....
Press [DELETE] nothing delete
Press [DELETE] makes tps://www....
Press [DELETE] nothing delete
Press [DELETE] makes ps://www....
Press [DELETE] nothing delete
...

Expected Results:
Press [DELETE] makes ttps://www....
Press [DELETE] makes tps://www....
Press [DELETE] makes ps://www....
...

Comment 1

[Alice0775 White]

Regression window(fx)
Good:
https://hg.mozilla.org/integration/fx-team/rev/c294ed0b5502
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141123145909
Bad:
https://hg.mozilla.org/integration/fx-team/rev/1791d8198cc3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141123160211
Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=c294ed0b5502&tochange=1791d8198cc3

Regressed by: Bug 1067903

Comment 2

[Alice0775 White]

Steps To Reproduce:
0. Store enough history (such as google search)
1. Type www.google in Location Bar
2. Focus Location Bar and Move caret to the left most (Press [HOME] key)
3. Attempt to delete www.go

Actual Results:
Press [DELETE] makes ww.go....
Press [DELETE] nothing delete
Press [DELETE] nothing delete
Press [DELETE] nothing delete
....
You need press [DELETE] key number of autocomplete entries
Press [DELETE] makes w.go....
Press [DELETE] nothing delete
Press [DELETE] nothing delete
Press [DELETE] nothing delete
....
You need press [DELETE] key number of autocomplete entries
Press [DELETE] makes .go....
Press [DELETE] nothing delete
Press [DELETE] nothing delete
Press [DELETE] nothing delete
....
You need press [DELETE] key number of autocomplete entries
....

Comment 3

[Marco Bonardo [:mak]]

looks like the is a focus change, one delete removes text, while the next one cancels the popup...

Comment 5

[Marco Bonardo [:mak]]

Alice, this happens only with unified complete right?
If so, there's no reason to track it.

Comment 6

[Alice0775 White]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #5)
> Alice, this happens only with unified complete right?
> If so, there's no reason to track it.

yes.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

The problem is that the first delete re-shows the popup (which is expected). But bug 1067903 has arranged for there to always be a selected item in the popup. nsAutoCompleteController.cpp has the delete key remove the item from the list when an item is selected - hence we hit this case.

Note that at all times the textbox keeps focus and the caret keeps flashing in the urlbar, even as you select items in the list. So there's nothing obvious I can see to help solve this - eg, if the textbox lost focus as you selected list items it would be trivial.

Given bug 1067903 explicitly says the "first item is special", the best I can come up with is to treat the first selected item as "special" in this case - "delete" doesn't make sense for that special item anyway. This would probably involve a new method on nsIAutoCompletePopup, say "bool isItemSelected", and our implementation would return true iff the selected index is > 0, with the default implementation returning true iff selected index != -1.  nsAutoCompleteController.cpp would then change to use this new method instead of calling GetSelectedIndex().

[clearing ni for Blair - I think we've the info we need]

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-1105244-avoid-delete-key-removing-the-first-spec.patch

The solution I came up with is for a new handleDelete() method on the autocomplete binding, which just calls the controller's handleDelete() as happens now. The urlbar binding overrides this method and calls the controller's handleText() method if the first item is selected, thus having the delete key in that case always work as a normal delete rather than removing that first item from the popup.

There's a test, but I'm struggling to make it work correctly in both the unified and non-unified case, but I thought I'd get feedback now and can fix that later if it looks ok.

Comment 9

[Marco Mucci [:MarcoM]]

Hi Mark, can you provide a point value.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-1105244-avoid-delete-key-removing-the-first-spec.patch

Same patch with tests fixed (hopefully fixed - I had to change the keycode in the test for osx and I'm still waiting for that to go green on https://treeherder.mozilla.org/#/jobs?repo=try&revision=56f41220a77d

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-1105244-avoid-delete-key-removing-the-first-spec.patch

Finally sorted out the test on OSX. Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4198f09ea7f7

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8601306 [details] [diff] [review]
0002-Bug-1105244-avoid-delete-key-removing-the-first-spec.patch

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

This is blocking us enabling unified autocomplete and has been sitting waiting for review for too long. It looks straightforward enough for me so let's get it landed and get unified complete on and baking. Mak perhaps you can do a feedback pass when you get time and if there are problems we can correct them in another bug.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f5cec2911e73

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5cec2911e73

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8601306 [details] [diff] [review]
0002-Bug-1105244-avoid-delete-key-removing-the-first-spec.patch

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

Sorry, I'm a little bit overloaded due to various events.
The approach looks fine, especially because it's very self-contained.

Comment 16

[Andrei Vaida [:avaida]]

Reproduced the bug with Nightly 36.0a1 (2014-11-26).

This is verified fixed on Nightly 41.0a1 (2015-06-16), using Ubuntu 14.04 (x86), Windows 8.1 (x64) and Mac OS X 10.9.5.