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)
Toolkit
UI Widgets
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha8
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
()
Details
Attachments
(2 files, 1 obsolete file)
3.10 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
3.70 KB,
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•18 years ago
|
Version: unspecified → 2.0 Branch
Comment 1•18 years ago
|
||
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
Assignee | ||
Comment 2•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
URL: about:logo
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•17 years ago
|
||
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)
Updated•17 years ago
|
QA Contact: fast.find → xul.widgets
Assignee | ||
Comment 4•17 years ago
|
||
Could we get a review on this before alpha6 freeze? Otherwise we have to retarget it to the next milestone.
Assignee | ||
Comment 5•17 years ago
|
||
Retargetting to beta1.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Assignee | ||
Comment 6•17 years ago
|
||
Comment on attachment 268777 [details] [diff] [review] Patch Requesting review from mano...
Attachment #268777 -
Flags: review?(gavin.sharp) → review?(mano)
Comment 7•17 years ago
|
||
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-
Assignee | ||
Comment 8•17 years ago
|
||
(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.
Assignee | ||
Comment 9•17 years ago
|
||
Comment on attachment 268777 [details] [diff] [review] Patch Requesting a review again, based on comment 8.
Attachment #268777 -
Flags: review- → review?(mano)
Comment 10•17 years ago
|
||
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+
Comment 11•17 years ago
|
||
Also, you don't need the |if (doc)| check.
Assignee | ||
Comment 12•17 years ago
|
||
The revised patch according to the comment 10 and 11. This version needs to be checked in.
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•17 years ago
|
Whiteboard: [checkin needed]
Comment 13•17 years ago
|
||
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?
Updated•17 years ago
|
Whiteboard: [checkin needed]
Assignee | ||
Comment 14•17 years ago
|
||
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?
Assignee | ||
Comment 15•17 years ago
|
||
(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
Comment 16•17 years ago
|
||
toolkit/content/widgets/findbar.xml 1.15
Assignee | ||
Updated•11 years ago
|
Flags: in-litmus?
You need to log in
before you can comment on or make changes to this bug.
Description
•