Closed Bug 1525547 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Address Bar, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: standard8, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

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
Summary: [QuantumBar] Figure out broken delete key handling on Windows/Linux → QuantumBar: Figure out broken delete key handling on Windows/Linux

(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.

Depends on: 1528806

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.

Assignee: nobody → mak77
Status: NEW → ASSIGNED
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/dbce5a153b63 Unify removing Quantum Bar entries to shift+DEL/BKSP. r=dao
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Depends on: 1598934
Blocks: 1598934
No longer depends on: 1598934
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: