1524950 - Resolve backspace handling in browser_urlbarValueOnTabSwitch.js

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement, P2)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1524950 - Make UrlbarInput keep clickSelectsAll up to date, and enable browser_urlbarValueOnTabSwitch.js.

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.

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Dão Gottwald [:dao]]

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

Comment 4

[Mark Banner (:standard8)]

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

Comment 5

[Drew Willcoxon :adw]

Attachment: Bug 1524950 - Resolve backspace handling in browser_urlbarValueOnTabSwitch.js.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df9abfd88677
Resolve backspace handling in browser_urlbarValueOnTabSwitch.js. r=dao

Comment 7

[Razvan Maries]

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