Resolve backspace handling in browser_urlbarValueOnTabSwitch.js

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P2
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: standard8, Assigned: adw)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 months ago

browser_urlbarValueOnTabSwitch.js looks like it should work with QuantumBar, however the test hangs when I try to run it with QuantumBar enabled.

STR

  1. In browser/components/urlbar/tests/legacy/browser.ini, change prefs=browser.urlbar.quantumbar=false to be true.
  2. Run the test (./mach mochitest browser/components/urlbar/tests/legacy/browser_urlbarValueOnTabSwitch.js)

In the preparePartialURLTab function, it attempts to press backspace 5 times to delete 5 characters from the url.

When run under QuantumBar, I'm seeing that the first backspace is correctly handled and the input event is received (https://searchfox.org/mozilla-central/rev/60c4067b1cbb0f94d7dc2d7cdfa27ed579817fee/browser/components/urlbar/tests/legacy/browser_urlbarValueOnTabSwitch.js#51-53).

However, for the second backspace we never get the event.

Running under the debugger I got the test to at least work partially. So there maybe some timing issue or something here - it made me wonder if this is where we need the event bufferer or something.

Reporter

Updated

4 months ago
Assignee

Updated

4 months ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
Assignee

Comment 1

4 months ago

UrlbarInput sets textbox.clickSelectsAll (from the clickSelectsAll pref) in its constructor, but the problem here is that it doesn't keep that property updated when the pref changes. That breaks this test because the test toggles the pref and expects it to change how selection behaves. It's not a problem with the awesomebar because it does in fact have a pref observer that keeps textbox.clickSelectsAll up to date.

So UrlbarInput needs a pref observer. Importantly, the observer either needs to be removed when the input goes away, or it needs to be weak, so that the input isn't held onto when its window closes. What's the best way to do that? We already have UrlbarPrefs, and it already adds itself as a prefs observer. So using UrlbarPrefs as the actual nsIObserver and hooking up a lightweight observer via UrlbarPrefs seems natural. But, UrlbarPrefs is a global singleton that never goes away, so we need to be careful that it doesn't hold on to UrlbarInput or its observer indefinitely. There's WeakSet, but unfortunately it doesn't support iteration, so UrlbarPrefs can't use it to weakly hold observers. There's nsIWeakReference, but that's XPCOM. So there needs to be a removeObserver method. When to call it? UrlbarInput doesn't currently have a notion of a destructor like the awesomebar does. So I added one and call it when the browser window closes.

That all seems like the most straightforward nice way to do this... An alternative would be to make UrlbarInput itself the nsIObserver and nsISupportsWeakReference and skip UrlbarPrefs entirely.

You actually had my same idea and thoughts (from bug 1523332) about UrlbarPrefs observer (even if I actually used a Map so registering multiple times the same observer is a no-op and we don't invoke lots of observers for nothing). I finally decided to remove it because there was only one consumer and there was risk of introducing windows leaks (in my case the observer was created in a window global object). But if you introduce an uninit() I may just go back using it.

(In reply to Drew Willcoxon :adw from comment #1)

That breaks this test because the test toggles the pref and expects it to change how selection behaves.

It looks like the test doesn't really need to depend on that. It could just explicitly call select() or move the caret to the end, respectively.

For end users we don't need to support live changes for this hidden pref, so that's why I didn't bother to implement that.

Reporter

Comment 4

4 months ago

(In reply to Dão Gottwald [::dao] from comment #3)

(In reply to Drew Willcoxon :adw from comment #1)

That breaks this test because the test toggles the pref and expects it to change how selection behaves.

It looks like the test doesn't really need to depend on that. It could just explicitly call select() or move the caret to the end, respectively.

Just checked the history and came up with bug 304198 comment 11, so I think we could indeed do one of the other options.

Attachment #9046940 - Attachment is obsolete: true

Comment 6

4 months ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df9abfd88677
Resolve backspace handling in browser_urlbarValueOnTabSwitch.js. r=dao

Comment 7

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.