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

VERIFIED FIXED in Firefox 41

Status

()

Firefox
Location Bar
P1
normal
Rank:
4
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Alice0775 White, Assigned: markh)

Tracking

({regression})

36 Branch
Firefox 41
regression
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox41 verified)

Details

(Whiteboard: [unifiedautocomplete][fxsearch])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
[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....
...
(Reporter)

Comment 1

3 years ago
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)
(Reporter)

Comment 2

3 years ago
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

Updated

3 years ago
Blocks: 995091
looks like the is a focus change, one delete removes text, while the next one cancels the popup...

Updated

3 years ago
Depends on: 1105456

Updated

3 years ago
Duplicate of this bug: 1105361
tracking-firefox36: ? → +
Alice, this happens only with unified complete right?
If so, there's no reason to track it.
(Reporter)

Comment 6

3 years ago
(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.
status-firefox35: unaffected → ---
status-firefox36: affected → ---
tracking-firefox36: + → ---
Blocks: 1157952
Whiteboard: [fxsearch][unifiedautocomplete]
Priority: -- → P1
Flags: firefox-backlog+
(Assignee)

Comment 7

2 years ago
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)
(Assignee)

Comment 8

2 years ago
Created attachment 8600236 [details] [diff] [review]
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.
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)
(Assignee)

Updated

2 years ago
Points: --- → 3
Flags: needinfo?(mhammond)
OS: Windows 7 → All
Hardware: x86_64 → All
(Assignee)

Comment 10

2 years ago
Created attachment 8600795 [details] [diff] [review]
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
Attachment #8600795 - Flags: review?(mak77)
(Assignee)

Updated

2 years ago
Attachment #8600236 - Attachment is obsolete: true
Attachment #8600236 - Flags: feedback?(mak77)
(Assignee)

Comment 11

2 years ago
Created attachment 8601306 [details] [diff] [review]
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
Attachment #8600795 - Attachment is obsolete: true
Attachment #8600795 - Flags: review?(mak77)
Attachment #8601306 - Flags: review?(mak77)

Updated

2 years ago
Rank: 4

Updated

2 years ago
Whiteboard: [fxsearch][unifiedautocomplete] → [unifiedautocomplete][fxsearch]

Updated

2 years ago
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)

Comment 13

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/f5cec2911e73
https://hg.mozilla.org/mozilla-central/rev/f5cec2911e73
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
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
status-firefox41: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.