1525547 - QuantumBar: Figure out broken delete key handling on Windows/Linux

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Mark Banner (:standard8)]

In running a ported browser_urlbarDelete.js test on try, I noticed that it is currently failing on Linux and Windows.

The basic issue appears to be:

• Start entering something that completes
• Press the Home button to get to the start of text
• Press the Delete button

At this stage, we're attempting to delete the autofill entry which fails, because it sees the selectedIndex as 0 and therefore fails the incorrect !index check.

https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/browser/components/urlbar/UrlbarController.jsm#322

I'm not sure this should be attempting to delete the history entry, I think it should just let it go and let the text be deleted.

I don't know how the old address bar handles this - it implies the selected index might be -1 to begin with, though I'm not quite sure.

In any case, we should fix this, bug 1525439 has ported the test but disabled it on non-Mac for now.

TEST-PASS | browser/components/urlbar/tests/browser/browser_urlbarDelete.js | "ug1105244.example.com/" == "ug1105244.example.com/" - 
Buffered messages finished
TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser/browser_urlbarDelete.js | "ug1105244.example.com/" == "g1105244.example.com/" - JS frame :: chrome://mochitests/content/browser/browser/components/urlbar/tests/browser/browser_urlbarDelete.js :: testDelete :: line 47
Stack trace:
chrome://mochitests/content/browser/browser/components/urlbar/tests/browser/browser_urlbarDelete.js:testDelete:47
resource://testing-common/BrowserTestUtils.jsm:withNewTab:111
chrome://mochitests/content/browser/browser/components/urlbar/tests/browser/browser_urlbarDelete.js:null:19
chrome://mochikit/content/browser-test.js:Tester_execTest/<:1106
chrome://mochikit/content/browser-test.js:Tester_execTest:1097
chrome://mochikit/content/browser-test.js:nextTest/<:995
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
Console message: [JavaScript Error: "Failed to find the selected result in the results" {file: "resource:///modules/UrlbarController.jsm" line: 323}]
_handleDeleteEntry@resource:///modules/UrlbarController.jsm:323:7
handleKeyNavigation@resource:///modules/UrlbarController.jsm:238:13
_on_keydown@resource:///modules/UrlbarInput.jsm:1003:5
handleEvent@resource:///modules/UrlbarInput.jsm:189:23

Comment 1

[Dão Gottwald [:dao]]

(In reply to Mark Banner (:standard8) from comment #0)

• Start entering something that completes
• Press the Home button to get to the start of text

[...]

I don't know how the old address bar handles this - it implies the selected index might be -1 to begin with, though I'm not quite sure.

That would be bug 1528806.

Comment 2

[Marco Bonardo [:mak]]

My opinion, considered bug 1528806 is unwanted per UX request, is that we only support SHIFT+DEL (and/or shift+backspace?) to remove entries.
It will be a small change that we can relnote (and we can use the behavioral differences document to create a relnote with all the changes).
Regardless, removing entries is already not really discoverable and the fact it differs across platforms makes it even more complex. The fact it uses a keypress that is usually related to modifying the text in the text field was also a bad choice from the beginning.

In the future we'll surely evaluate either a contextual menu or the options button for each entry, like Verdi suggested.

Comment 4

[Marco Bonardo [:mak]]

Attachment: Bug 1525547 - Unify removing Quantum Bar entries to shift+DEL/BKSP. r=dao!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbce5a153b63
Unify removing Quantum Bar entries to shift+DEL/BKSP. r=dao

Comment 6

[Cristina Coroiu [:ccoroiu]]

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

Comment 7

[Andreea Pavel [:apavel]]

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