569342 - Find bar should not be enabled in about:addons

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[John P Baker]

1. Tools / Add-ons
2. '/' to find in page

findbar shows but any text gives "Phrase not found".

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100531 Minefield/3.7a5pre

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We haven't had this feature in the old add-ons manager because it was in its own window. Looks like this is a valid feature request now.

Comment 2

[John P Baker]

(In reply to comment #1)
> We haven't had this feature in the old add-ons manager because it was in its
> own window. Looks like this is a valid feature request now.

Either an enhancement or an error that the findbar shows - I intentionally skipped mentioning any expected results.

Comment 4

[Tiago Sá]

It should be noted that "find as you type" works perfectly in Firefox 3.6's download manager, when it's loaded in a tab:

chrome://mozapps/content/downloads/downloads.xul

The search bar in the download manager also works perfectly.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, we only restrict FAYT when the focus is in the search field. If we have focused other areas it would be fantastic to find elements you are looking for. 

Lets say you have opened the list of extensions and about 40 are installed. Just typing "noscript" should jump to the extension.

Comment 7

[Dave Townsend [:mossop]]

(In reply to comment #6)
> Dave, we only restrict FAYT when the focus is in the search field. If we have
> focused other areas it would be fantastic to find elements you are looking for. 
> 
> Lets say you have opened the list of extensions and about 40 are installed.
> Just typing "noscript" should jump to the extension.

Short of someone who's actually going to fix the find bar for this UI I intend to instead disable it completely and just hook up the keyboard events that would trigger the find bar to focus the search box.

Comment 8

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Adds an optional property disablefastfind that documents in about: or chrome: urls can use to disable the find bar for themselves. Adds it to about:config and about:addons and tests that it works.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 449289 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/content/widgets/findbar.xml b/toolkit/content/widgets/findbar.xml

>+          // disable FAYT in about:blank to prevent FAYT opening unexpectedly.
>+          // Fixes bugs 264562 and bug 269712

>+          // disable FAYT in documents that ask for it to be disabled. Fixed
>+          // bug 267150 and bug 569342

I'd just remove the bug # references entirely - getting them from blame is sufficient (there's nothing particularly insightful in the bugs, and the reasoning is pretty obvious).

>+          if ((url.schemeIs("about") || url.schemeIs("chrome")) &&
>+              (win.document.documentElement &&
>+               win.document.documentElement.getAttribute("disablefastfind") == "true"))
>+            return;

return false;

Comment 10

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/33760547ecf7

Filed bug 570760 to get the keyboard shortcuts hooked up to the search box.

Comment 11

[Dave Townsend [:mossop]]

Backed out as this causes a test failure.

Comment 12

[Dave Townsend [:mossop]]

We need the test fixes from bug 567306 before we can land the test for this.

Comment 15

[Dave Townsend [:mossop]]

Landed but renamed the test so it runs later and doesn't break browser_typeAheadFind.js. Would probably be good to rename that back afterwards.

http://hg.mozilla.org/mozilla-central/rev/e2e5b5e57ca4

Comment 16

[Nochum Sossonko [:Natch]]

Ctrl-f is supposed to still work?

Comment 17

[Dave Townsend [:mossop]]

Nope

Comment 18

[:Margaret Leibovic]

Attachment: follow-up patch (to disable find command)

I'm running into a problem with this patch, but I feel like it might be a separate issue. The "Find..." menuitem in the Firefox button menu is not correctly observing the disabled attribute, even though the keyboard shortcut and the menuitem in the menubar are. Other than that, this patch seems to do the job correctly.

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 504938 [details] [diff] [review]
follow-up patch (to disable find command)

I think you also want to disable the cmd_findAgain and cmd_findPrevious commands. I don't really know why the menu wouldn't be responding to this though. Maybe gavin knows?

Comment 20

[Dave Townsend [:mossop]]

Over to Margaret for the last mile

Comment 21

[:Margaret Leibovic]

Attachment: follow-up patch v2

The disabled appmenu menuitem is a separate issue, so I filed bug 627136. Gavin says we shouldn't worry about it here, and instead just fix the problem in bug 627136.

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 505174 [details] [diff] [review]
follow-up patch v2

Looks good to me, technically I'm not a browser peer so just check that gavin is ok with me reviewing it. This does want a test though.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 505174 [details] [diff] [review]
follow-up patch v2

-I think you need to handle aLocationURI being null
-nit: local var for content.document.documentElement?
-nit: put if condition in a boolean, have only one forEach with the if/else inside it?

Comment 24

[:Margaret Leibovic]

Attachment: updated patch v3 (with test)

Addressed Gavin's comments and added a test. I had to modify the test file to change the selected tab after the page loaded because there were some timing problems with the location change event. I think it should be more reliable this way.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 505303 [details] [diff] [review]
updated patch v3 (with test)

r+ with the nits discussed IRL

Comment 26

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/ffb56732bdbe

Comment 27

[Eric Shepherd [:sheppy]]

Documented the disablefastfind attribute here:

https://developer.mozilla.org/en/XUL/Attribute/disablefastfind

And it's linked to from:

https://developer.mozilla.org/en/XUL/window

And mentioned on Fx4 for developers.

Comment 28

[Andreea Pod]

Build identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b10) Gecko/20100101 Firefox/4.0b10

I can not open find bar on addons manager. Edit > Find is disabled and the shortcut keys don't work. Is this a new bug?

Comment 29

[Luke Iliffe (Harlequin99)]

The find bar is still available on Windows 7 from the firefox button. I assume it should be disabled like the ctrl+F and the Edit, Find menu bar entry.

Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110122 Firefox/4.0b10pre 20110122030336 7b396ca54953

Comment 30

[Dave Townsend [:mossop]]

(In reply to comment #28)
> I can not open find bar on addons manager. Edit > Find is disabled and the
> shortcut keys don't work. Is this a new bug?

The purpose of this bug was to disable those things, it is intentional.

(In reply to comment #29)
> The find bar is still available on Windows 7 from the firefox button. I assume
> it should be disabled like the ctrl+F and the Edit, Find menu bar entry.

As mentioned in comment 21 that is bug 627136

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110121 Firefox/4.0b10pre

Comment 32

[Andreea Pod]

(In reply to comment #30)
> (In reply to comment #28)
> > I can not open find bar on addons manager. Edit > Find is disabled and the
> > shortcut keys don't work. Is this a new bug?
> 
> The purpose of this bug was to disable those things, it is intentional.

Oh yes, sorry for the misunderstanding.

Comment 33

[Siddhartha Dugar [:sdrocking]]

This is silly
Open a new tab
Hit Ctrl+F
Open add-on manager from the Firefox Button

Result: Findbar is visible.

Comment 34

[:Margaret Leibovic]

(In reply to comment #33)
> This is silly
> Open a new tab
> Hit Ctrl+F
> Open add-on manager from the Firefox Button
> 
> Result: Findbar is visible.

That is a separate issue. Please see bug 627136.