254592 - Find as you type = off pref, should respect typeaheadfind and not allow autostart

Status: NEW

(Toolkit :: Find Toolbar, defect, P2)

Description

[Peter van der Woude [:Peter6]]

If Use Find as you type in options is off only
userpref accessibility.typeaheadfind is set to false
userpref accessibility.typeaheadfind.autostart is left default (=true)

For people using chats and fora it's extremely annoying if you use ' or / and
the autostart starts the find toolbar and moves focus to that input.

expected:
if 
accessibility.typeaheadfind is set false
accessibility.typeaheadfind.autostart should respect that and regarless of its
value not autostart (respect accessibility.typeaheadfind false).

Comment 1

[Michael Clackler]

This bug is fixed with the patch in bug 251073

Comment 2

[Peter van der Woude [:Peter6]]

*** Bug 264814 has been marked as a duplicate of this bug. ***

Comment 3

[Peter van der Woude [:Peter6]]

(In reply to comment #1)
> This bug is fixed with the patch in bug 251073

so typing a / or a ' on a page (not in an input) causing the findbar to popup is
designed behaviour ?
I though this would be disabled, only leaving you the option to press F3 to
activate it ?

Comment 4

[Michael Clackler]

(In reply to comment #3)
> (In reply to comment #1)
> > This bug is fixed with the patch in bug 251073
> 
> so typing a / or a ' on a page (not in an input) causing the findbar to popup is
> designed behaviour ?
> I though this would be disabled, only leaving you the option to press F3 to
> activate it ?

The final patch to fic bug 251073 was not that patch that included the fix for
this bug.  I am working on a new patch to fix the remainder of the preference
issues including this one.

Comment 5

[Michael Clackler]

this bug is fixed with the patch in bug 265915

Comment 6

[Peter van der Woude [:Peter6]]

*** Bug 290625 has been marked as a duplicate of this bug. ***

Comment 7

[Matt]

Possibly related to bug 220900?

Comment 8

[Amir Caspi]

In v1.06 on MacOS X (10.3.9), the preference "Begin finding as you begin typing"
controls the *autostart* mechanism of typeaheadfind.  However, the checkbox will
modify accessibility.typeaheadfind, *NOT* accessibility.typeaheadfind.autostart.

In addition, it appears that accessibility.typeaheadfind.autostart is completely
ignored (true/false make no difference in behavior), and
accessibility.typeaheadfind controls only the autostart mechanism.  There
appears to be no way to totally disable typeaheadfind because of this.

Comment 9

[Adam Guthrie]

*** Bug 321418 has been marked as a duplicate of this bug. ***

Comment 10

[Amir Caspi]

In v1.5.0.1 for MacOS X, the bug I described in comment #8 is still present... the two typeaheadfind preferences are not behaving properly, as outlined in that comment. 

Comment 11

[Mikel Ward]

The same problem occurs in Firefox 2.0 beta 1 (20060710) on Windows

Comment 12

[Mikel Ward]

Sorry, the problem in the latest Windows beta is that any typing outside a text field will activate type-ahead find, even if accessibility.typeaheadfind.autostart is false.  The MozillaZine knowledge base links to this bug for that problem. :-(

Comment 13

[Adam Guthrie]

*** Bug 348187 has been marked as a duplicate of this bug. ***

Comment 16

[Mike de Boer [:mikedeboer]]

The history of the 'autostart' pref wasn't very clear to me, until I dived a little deeper into CVS and bugzilla history.
So this was implemented in bug 176296, which was back when _any_ keystroke would initiate a find-in-page and "'" or "/" were used to switch between text-mode and links-only-mode.

Today these keys are used as activators for Quick Find (showing a minimally marked up findbar), and the 'autostart' pref is not used anymore.
Additionally, the 'accessibility.typeaheadfind.autostart' and 'accessibility.typeaheadfind.enabletimeout' are not used anymore.

Therefore it seems prudent to ensure that
 1. 'accessibility.typeaheadfind.autostart = false' disables Quick Find for those who wish it,
 2. 'accessibility.typeaheadfind.enabletimeout' is removed from all.js.

Upping priority, since this bug is now actionable.

Comment 18

[spillner]

Attachment: Bug 254592 - Updated "Find As You Type" (FAYT) to resolve https://bugzilla.mozilla.org/show_bug.cgi?id=254592, specifically: r=mikedeboer

- The accessibility.typeaheadfind preference turns off ALL FAYT functions when set to 'false'
  - The keystroke-interpretation code in FindBarChild.jsm and findbar.xml now respects both the accessibility.typeaheadfind.manual and typeaheadfind.autostart preferences
  - Documents of any type can manually inhibit FAYT with the "disablefastfind" document element attribute (previously restricted to the chrome: and about: protocols)
  - Corrected & extended the pertinent unit tests (browser_fastfind* test modules)

Comment 19

[spillner]

Uploaded patch at https://phabricator.services.mozilla.com/D3404 to simplify/correct this to what I think is more intuitive logic:
   * Setting accessibility.typeaheadfind to 'false' inhibits ALL fast-find triggers
   * When accessibility.typeaheadfind and accessibility.typeaheadfind.manual are both true, typing a / or ' will trigger the fast-find dialog
   * When accessibility.typeaheadfind and accessibility.typeaheadfind.autostart are both true, typing any character except a space will trigger the fast-find dialog
   * Note that when all three preference variables are true, the manual trigger characters (/ or ') will trigger but not populate the dialog, i.e. you would have to type ' or / twice to begin a search string with those characters.  When autostart is true but the manual trigger is not, ' and / are treated exactly like any other keys.  This was poorly defined before and this convention seems preferable (less confusing to those used to the manual shortcuts).

If accepted, http://kb.mozillazine.org/Accessibility.typeaheadfind should be updated to reflect the new behavior.

Even if the patch is not accepted, someone should correct the relevant variable names in the new toolkit/actors/FindBarChild.jsm to match toolkit/content/widgets/findbar.xml (leading underscores in _findAsYouType, _manualFAYT)--- right now this typo breaks most FAYT behavior.

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 9000194 [details]
Bug 254592 - Updated "Find As You Type" (FAYT) to resolve https://bugzilla.mozilla.org/show_bug.cgi?id=254592, specifically: r=mikedeboer

Kris Maglione [:kmag] has approved the revision.

Comment 21

[Daniel Varga [:dvarga]]

Hi! When trying to land this on autoland trough phabricator(lando) I received the following error:

Error: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmpjkVhcf\npatching file toolkit/content/widgets/findbar.xml\nHunk #2 FAILED at 327\n1 out of 16 hunks FAILED -- saving rejects to file toolkit/content/widgets/findbar.xml.rej\nabort: patch failed to apply', '')

Comment 22

[Eliza Balazs [:ebalazs_]]

Please see Comment 21. Thanks!

Comment 23

[Dorel Luca [:dluca]]

Hi spillner

The commit message for the patch needs fixing.

You can use this as guideline: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities

Comment 24

[spillner]

Reset to original commit message.  I've also submitted the following text to the MozillaZine knowledge base thread as a correction for http://kb.mozillazine.org/Accessibility.typeaheadfind:

"When accessibility.typeaheadfind is set to false, all Find As You Type functionality is disabled.  When both accessibility.typeaheadfind and accessibility.typeaheadfind.manual are set to true, typing a ' or / will open the Find As You Type window and move the cursor focus there.  If accessibility.typeaheadfind and accessibility.typeaheadfind.autostart are both true, typing anything other than a space will open the Find As You Type window.  If accessibility.typeaheadfind.timeout is set to a value greater than zero, the Find As You Type window will disappear that many milliseconds after the last keypress."

Comment 25

[Ryan VanderMeulen [:RyanVM]]

The issue with the commit message is the first line - you're basically referencing the bug number twice and not clearly summarizing what the patch is actually doing. I would expect it to be something more along the lines of:
Bug 254592 - Update "Find As You Type" (FAYT) to respect the accessibility typeaheadfind preferences. r=kmag

Or something to that effect. Anyway, that's more in line with our usual conventions for commit messages.

Comment 26

[spillner]

The patch fixes the bug.  The next three lines of the commit message describe the new behavior.  The tests provide some level of confidence that the browser satisfies that specification.  I don't see much value in anything either shorter or longer.  Feel free to edit it yourself if I'm missing something.

Comment 27

[Calixte Denizet (:calixte)]

:mikedeboer, could you fix the issue with the commit message ?

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:mcheang, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 29

[Mandy Cheang [:mcheang][she/her]]

After reading through the comments, I see that a patch was submitted to fix this and it was approved.
Previously, it failed to land because there was an issue with the commit message.

Since this patch was put up 4 years ago and was already approved. I'd recommend re-basing it to central, testing it manually again, and running the test against the patch on TRY. And lastly, fix the format of the commit message.

Since we're doing the hand over of triage owner today. I'll ask if you can do the above steps recommended, Neil, when you are able to. Thank you.

Comment 30

[spillner]

I'm still here and happy to do the rebase; I didn't realize anyone still cared about this bug based on its age (18 years!) and the difficulty finding anyone to land it when the patch was reviewed & approved in 2018.

Comment 31

[Mandy Cheang [:mcheang][she/her]]

(In reply to spillner from comment #30)

I'm still here and happy to do the rebase

Thanks for sticking around after so long :)!

If you could do the rebase and fix the commit message. It could help Neil with landing this bug. Of course, we would have to check that the patch still works. But the first and easy step seems to be the commit message.

Comment 32

[:Gijs (he/him)]

(In reply to spillner from comment #30)

I'm still here and happy to do the rebase; I didn't realize anyone still cared about this bug based on its age (18 years!) and the difficulty finding anyone to land it when the patch was reviewed & approved in 2018.

Yeah, this doesn't seem to have gone well - apologies for that.

How did you get on trying to rebase the patch? Do let us know if you get stuck - it'd be a shame if this bug got lost for another 4 years. :-)

Comment 33

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:enndeakin, since the bug has high priority and recent activity, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 34

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 6 duplicates and 11 votes.
:enndeakin, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 35

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.