Closed Bug 349884 Opened 18 years ago Closed 17 years ago

Fast Find must be disabled when viewing non-text content

Categories

(Toolkit :: UI Widgets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

()

Details

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060710 Firefox/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060710 Firefox/2.0b1

When viewing non-text content (for example, an image), the Find toolbar (Ctrl+F) is disabled (it's disabled in the Edit menu, and pressing Ctrl+F does nothing), but the Fast Find toolbar is not disabled (pressing / or ' invokes the Fast Find toolbar).

Reproducible: Always

Steps to Reproduce:
1. Go to <https://bugzilla.mozilla.org/attachment.cgi?id=224582> or any other link directly pointing to an image.
2. Hit / or '.
Actual Results:  
The Fast Find toolbar shows.

Expected Results:  
Nothing should have happened.

about:buildconfig

Build platform
target
i586-pc-msvc

Build tools
Compiler 	Version 	Compiler flags
$(CYGWIN_WRAPPER) cl 	12.00.8804 	-TC -nologo -W3 -Gy -Fd$(PDBFILE)
$(CYGWIN_WRAPPER) cl 	12.00.8804 	-TP -nologo -W3 -Gy -Fd$(PDBFILE)

Configure arguments
--enable-application=browser --enable-update-channel=beta --enable-official-branding --enable-optimize --disable-debug --disable-tests --enable-static --disable-shared --enable-svg --enable-canvas --enable-update-packaging
Version: unspecified → 2.0 Branch
I can reproduce this in "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060823 BonEcho/2.0b2".  Confirming..
Status: UNCONFIRMED → NEW
Ever confirmed: true
This is a bug in the findbar toolkit XUL widget.  I'm taking over this bug, and will post a patch shortly.
Assignee: nobody → ehsan.akhgari
Component: Find Toolbar / FastFind → XUL Widgets
OS: Windows XP → All
Product: Firefox → Toolkit
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha6
Version: 2.0 Branch → Trunk
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
Patch to fix this bug.  The code is based on the code used to disable the Find command in browser (see bug 294383, attachment 187494 [details] [diff] [review]).

Tested on trunk.  To do a test, you can build a toolkit.jar with the patch applied, and navigate to <about:logo> and try pressing ' or /.
Attachment #268777 - Flags: review?(gavin.sharp)
QA Contact: fast.find → xul.widgets
Could we get a review on this before alpha6 freeze?  Otherwise we have to
retarget it to the next milestone.
Retargetting to beta1.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Comment on attachment 268777 [details] [diff] [review]
Patch

Requesting review from mano...
Attachment #268777 - Flags: review?(gavin.sharp) → review?(mano)
Comment on attachment 268777 [details] [diff] [review]
Patch

We should keep this in sync with Edit->Find, right now it is only disabled in image documents AFACIT.
Attachment #268777 - Flags: review?(mano) → review-
(In reply to comment #7)
> (From update of attachment 268777 [details] [diff] [review])
> We should keep this in sync with Edit->Find, right now it is only disabled in
> image documents AFACIT.
> 

In fact, this is *exactly* what my patch does.  The cmd_find command (bound to the Edit->Find menu item) observes the isImage observer <http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/base/content/browser-sets.inc&rev=1.99&mark=80-82#80> which is enabled/disabled based on the result of the |mimeTypeIsTextBased| <http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/base/content/browser.js&rev=1.815&mark=3395-3399,3464-3468#3395> which is defined in <http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/base/content/browser.js&rev=1.815&mark=3269-3275#3269>.  The |mimeTypeIsTextBased| function defined in my patch is exactly based on the corresponding function defined in browser.js.  Therefore, I think that my patch achieves exactly what you mention: syncing the fast find feature with the Edit->Find menu item.
Comment on attachment 268777 [details] [diff] [review]
Patch

Requesting a review again, based on comment 8.
Attachment #268777 - Flags: review- → review?(mano)
Comment on attachment 268777 [details] [diff] [review]
Patch

Ugh, ok, the broadcaster naming is confusing.

Please move this check to the top of this function, it's cheaper than most of the other checks in there.

r=mano otherwise.
Attachment #268777 - Flags: review?(mano) → review+
Also, you don't need the |if (doc)| check.
Attached patch Revised patch for check-in (obsolete) — Splinter Review
The revised patch according to the comment 10 and 11.

This version needs to be checked in.
Keywords: checkin-needed
Whiteboard: [checkin needed]
Fwiw, Mano raised another point when we were discussing this patch a while ago:

<Mano> gavin what is mozilla.application/cached-xul?
<gavin|> I don't know
<Mano> and why is that considered text-based
<Mano> awesome
<gavin|> I guess it's the content type of cached xul files?
<gavin|> that function was just copied from navigator.js
<dmose> could .mfl / .mfasl fastload files?
<dmose> s/could/could be./
<Mano> gavin oh, hrm, i don't think we want that for findbar
<Mano> gavin makes sense for view source though
<gavin|> Mano: why not?
<Mano> gavin find in xul doesn't really work :p
<gavin|> oh
<Mano> gavin then we can remove the hard coded "about:config" check in there

Worth filing a followup bug?
Whiteboard: [checkin needed]
Gavin, this search gives curious results  :-)

<http://mxr.mozilla.org/mozilla/search?string=cached-xul>

I can submit an updated patch on this bug which removes the check for "mozilla.application/cached-xul" and mark it for checkin on this bug, or we can file a followup bug.  We also need another bug which removes it from browser.js as well.

Should I go ahead and file the bugs?
(In reply to comment #14)
> Gavin, this search gives curious results  :-)
> 
> <http://mxr.mozilla.org/mozilla/search?string=cached-xul>
> 
> I can submit an updated patch on this bug which removes the check for
> "mozilla.application/cached-xul" and mark it for checkin on this bug, or we can
> file a followup bug.  We also need another bug which removes it from browser.js
> as well.

OK, here is the revised patch which needs to be checked in.
Attachment #273364 - Attachment is obsolete: true
toolkit/content/widgets/findbar.xml 1.15
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Keywords: verifyme
Flags: in-litmus?
Keywords: verifyme
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: