Closed Bug 1105244 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox :: Address Bar, defect, P1)

36 Branch
defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 41
Iteration:
41.1 - May 25
Tracking Status
firefox41 --- verified

People

(Reporter: alice0775, Assigned: markh)

References

Details

(Keywords: regression, Whiteboard: [unifiedautocomplete][fxsearch])

Attachments

(1 file, 2 obsolete files)

[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....
...
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
Blocks: 1067903
Flags: needinfo?(bmcbride)
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
....
Summary: Deleting a character needs pressing [DELETE] key twice in Location Bar → Deleting a character needs pressing [DELETE] key more than twice in Location Bar
looks like the is a focus change, one delete removes text, while the next one cancels the popup...
Depends on: 1105456
Alice, this happens only with unified complete right?
If so, there's no reason to track it.
(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.
Blocks: 1157952
Whiteboard: [fxsearch][unifiedautocomplete]
Priority: -- → P1
Flags: firefox-backlog+
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]
Flags: needinfo?(bmcbride)
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.
Attachment #8600236 - Flags: feedback?(mak77)
Hi Mark, can you provide a point value.
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Flags: qe-verify?
Flags: needinfo?(mhammond)
Points: --- → 3
Flags: needinfo?(mhammond)
OS: Windows 7 → All
Hardware: x86_64 → All
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
Attachment #8600795 - Flags: review?(mak77)
Attachment #8600236 - Attachment is obsolete: true
Attachment #8600236 - Flags: feedback?(mak77)
Finally sorted out the test on OSX. Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4198f09ea7f7
Attachment #8600795 - Attachment is obsolete: true
Attachment #8600795 - Flags: review?(mak77)
Attachment #8601306 - Flags: review?(mak77)
Rank: 4
Whiteboard: [fxsearch][unifiedautocomplete] → [unifiedautocomplete][fxsearch]
Iteration: 40.3 - 11 May → 41.1 - May 25
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
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.
Attachment #8601306 - Flags: review?(mak77)
Attachment #8601306 - Flags: review+
Attachment #8601306 - Flags: feedback?(mak77)
https://hg.mozilla.org/mozilla-central/rev/f5cec2911e73
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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.
Attachment #8601306 - Flags: feedback?(mak77) → feedback+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: