1418033 - Make it possible to disable fast find

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P3)

Description

[Tom S. (please needinfo tschuster)]

I think a lot of people find "fast find" annoying and would rather completely disable it. At least the shortcuts `/` and `'` can get in the way of getting stuff done.

https://twitter.com/paddykontschak/status/931226720098758656

Comment 1

[Mike Conley (:mconley) (:⚙️)]

I tend to agree. I would support disabling this feature by default.

Comment 2

[:Gijs (he/him)]

Generally the preferences here are a bit messy (see bug 254592 and friends), and it's not clear to me if there's currently a way to do this, if there should be but it's somehow broken, or if there just isn't a "turn the whole thing off" pref right now. So this may be a dupe, but I'm not entirely sure. Mike's away until January, but in the meantime, I guess we would take a patch if it dealt appropriately with the tangle of existing prefs.

I'm not sure about disabling the feature by default, that'd probably want some UX feedback.

Comment 4

[Tom S. (please needinfo tschuster)]

Attachment: Make it possible to disable fast find. r?

Comment 5

[:Gijs (he/him)]

Comment on attachment 8975604 [details] [diff] [review]
Make it possible to disable fast find. r?

Mike's a better reviewer for this.

Comment 6

[Josh Triplett]

Thanks for writing a patch! This seems like a good start.

At some point in the future, perhaps based on telemetry data, we might consider whether to disable this by default. Having this preference available would then let us make that decision as easily as changing the default value. In the meantime, having a preference that people can disable will help those who regularly run into this problem.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8975604 [details] [diff] [review]
Make it possible to disable fast find. r?

Review of attachment 8975604 [details] [diff] [review]:
-----------------------------------------------------------------

Kudos for the positive pref name that doesn't talk about 'disabling' stuff, but rather about 'enabling' it.

This is only missing one piece: a (regression) unit test! Can you please add one for me to check out in this bug? Thanks!!

::: toolkit/modules/RemoteFinder.jsm
@@ +334,5 @@
>    "accessibility.typeaheadfind.linksonly");
>  XPCOMUtils.defineLazyPreferenceGetter(RemoteFinder, "_findAsYouType",
>    "accessibility.typeaheadfind");
> +XPCOMUtils.defineLazyPreferenceGetter(RemoteFinder, "_manualFAYT",
> +  "accessibility.typeaheadfind.manual");

Not that I care a whole lot about allowing this to be available as a runtime switch, but it does seem that this wouldn't do it, whilst you're trying hard to do so in findbar.xml...

I'm not sure it's worth fixing at this point - but since we're repeating this pattern in RemoteFinder.jsm, we might need a followup bug (not one you should feel obliged to take on, but I won't stop you ;-) ) to fix that up.

Comment 8

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> ::: toolkit/modules/RemoteFinder.jsm
> @@ +334,5 @@
> >    "accessibility.typeaheadfind.linksonly");
> >  XPCOMUtils.defineLazyPreferenceGetter(RemoteFinder, "_findAsYouType",
> >    "accessibility.typeaheadfind");
> > +XPCOMUtils.defineLazyPreferenceGetter(RemoteFinder, "_manualFAYT",
> > +  "accessibility.typeaheadfind.manual");
> 
> Not that I care a whole lot about allowing this to be available as a runtime
> switch, but it does seem that this wouldn't do it, whilst you're trying hard
> to do so in findbar.xml...

Why not? defineLazyPreferenceGetter updates the property if the pref changes, I think...?

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs (he/him) from comment #8)
> Why not? defineLazyPreferenceGetter updates the property if the pref
> changes, I think...?

Ah ok, I'm sorry for the confusion... it's getting a bit late here ;-)

Comment 10

[Tom S. (please needinfo tschuster)]

Any hints for testing this? I found some tests that Invoke browser.finder directly, but I am looking for something that uses the shortcuts and makes sure it doesn't open.

Comment 11

[Mike de Boer [:mikedeboer]]

Apology for the late reply, Tom! I think you'll find a good example test at https://searchfox.org/mozilla-central/source/toolkit/content/tests/browser/browser_findbar.js

Comment 12

[Tom S. (please needinfo tschuster)]

Attachment: Make it possible to disable fast find. r?

Comment 13

[Tom S. (please needinfo tschuster)]

Attachment: Test for disabled manual FAYT. r?

Comment 14

[Tom S. (please needinfo tschuster)]

Comment on attachment 8987299 [details] [diff] [review]
Make it possible to disable fast find. r?

Review of attachment 8987299 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/findbar.xml
@@ +325,5 @@
>              return;
>  
>            let prefsvc = this._self._prefsvc;
> +          if (!prefsvc)
> +            return;

For some reason the test I added throws after finishing the run with:

 0:03.47 PASS Findbar should still be hidden. - 
 0:03.48 INFO Leaving test bound test_hotkey_disabled
 0:03.51 FAIL uncaught exception - TypeError: prefsvc is undefined at observe@chrome://global/content/bindings/findbar.xml:334:15
_receiveMessageAPI@chrome://mochikit/content/tests/SimpleTest/SpecialPowersObserverAPI.js:348:20
ChromePowers.prototype._receiveMessage@chrome://mochikit/content/tests/SimpleTest/ChromePowers.js:83:14
ChromePowers.prototype._sendSyncMessage@chrome://mochikit/content/tests/SimpleTest/ChromePowers.js:40:11
_setPref@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1402:12
_applyPrefs@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1245:9
popPrefEnv/<@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1165:9
popPrefEnv@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1158:12
flushPrefEnv/<@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1178:7
flushPrefEnv@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1176:12
nextTest/<@chrome://mochikit/content/browser-test.js:768:36
nextTest@chrome://mochikit/content/browser-test.js:768:13
async*testScope/test_finish/<@chrome://mochikit/content/browser-test.js:1392:11
run@chrome://mochikit/content/browser-test.js:1329:9

Comment 15

[:Gijs (he/him)]

Comment on attachment 8987299 [details] [diff] [review]
Make it possible to disable fast find. r?

Review of attachment 8987299 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/findbar.xml
@@ +378,5 @@
>  
>          prefsvc.addObserver("accessibility.typeaheadfind",
>                              this._observer);
> +        prefsvc.addObserver("accessibility.typeaheadfind.manual",
> +                            this._observer);

You need a matching removeObserver call to make this not continue to be accessed once the binding is dead. There's some existing ones for the other prefs in this file already. Then you can remove the early return.

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8987299 [details] [diff] [review]
Make it possible to disable fast find. r?

Review of attachment 8987299 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with Gijs' suggestion implemented. Thanks, Tom!

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8987300 [details] [diff] [review]
Test for disabled manual FAYT. r?

Review of attachment 8987300 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I think the other tests make sure that the reverse pref setting continues to work. Thanks for this test, looks nice & simple.

::: toolkit/content/tests/browser/browser_findbar_disabled_manual.js
@@ +6,5 @@
> +    ["accessibility.typeaheadfind.manual", false],
> +  ]});
> +});
> +
> +// Makes sure that the findbar hotkeys (' and /) have no effect

nit: missing dot.

@@ +8,5 @@
> +});
> +
> +// Makes sure that the findbar hotkeys (' and /) have no effect
> +add_task(async function test_hotkey_disabled() {
> +  // Opening new tab

nit: missing dot.

Comment 18

[Tom S. (please needinfo tschuster)]

Thanks for review and the hint with removeObserver.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3559084af4a
Make it possible to disable fast find. r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e50b1960713
Test for disabled manual FAYT. r=mikedeboer

Comment 20

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/c3559084af4a
https://hg.mozilla.org/mozilla-central/rev/3e50b1960713

Comment 21

[jscher2000]

Is this change a good candidate for uplift to beta?

It does come up on support forums from time to time. While user scripts (and probably content scripts) can intercept and discard / and ' in many cases, there are no workarounds for some other pages, such as the calculator on the Google search results page.

Comment 22

[the_new_mr]

This is happening for me as of today.

It's a really really weird one. Doesn't always happen. And has a very strange combination of factors required for it to occur.

Could be related to a bug in Outlook for the web. But I feel like Firefox should handle the situation better.

In outlook for the web, when composing an email, attempting to type "you're" brings up quick search. But only if I've put three or more letters after Hi and before a comma on the first line. See GIF for demo.

I have:

Hi Ha,

Hope you

Typing an apostrophe here works.

But if I have:

Hi Har,

Hope you

and then type an apostrophe after the you, the quick search bar opens.

See GIF and screenshots of console and Quick Find in link below:
https://imgur.com/a/rkifEbA

Comment 23

[Ksenia Berezina [:ksenia]]

Thanks :the_new_mr, I've filed a separate bug for this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1764199