I think having an integrated Find in page panel could be a nice feature. Why? - it would get rid of a pop-up (see bug 7930) that masks randomly the text (especially the matched part, as implied by the Murphy's Law) and that can appear randomly in the desktop (at least on linux 2.2) - consistency with the simple search panel in mail (bug 63573) and addressbook (bug 83023). - not having Find pop-up we don't know the parent's window. - ergonomy: o few buttons o preferences rarely changed are hidden o easy implementation of many enhancement features with an advanced button. - easier implementation of a search history droplist - enhanced usability This is an example of what could appear/disappear toggling with CTRL-F: -+-----------------------------------------------------------------+ | Find in page for: |____|v| (<) (>) (Clear) (Advanced) | +-------------------------^-^-------------------------------------+ | | : : Clicking on advanced search would show (when all features implemented): -+-------------------------------------------------+ | Find in page for: |____|v| (<)(>) (Clear)(Hide) | +-------------------------^-^---------------------+ S| . Search as typing | (bug 30088) I| x Search always from the beginning of the page | (bug 87037) D| x Search cycling throught the window | (see bug 91520) E| . Match exact case | (see bug 49331) B| . Match exact diacritic characters | (from bug 7930) A| . Match exact exact alef hamza | (from bug 7930) R| . Match * and ? wildchars | (bug 32641) | . Match regular expressions | (bug 37941) | . Match whole words only | (bug 14871) +-------------------------^-^---------------------+ | | : Browser Lay-out : **WARNING**: I presented the settings in one column. They should be in two columns to save space. Notes: - (<) search again backwards Ctrl-Shift-G (see bug 77704) - (>) search again forwards, Ctrl-G - |v| button to show the search history (bug 70771) as in the location bar. - ^-^ in lines when clicked on close the advanced panel or the search panel. - I proposed an UI for features that don't exist yet. I know it's bad. I did it so that the advanced panel would not appear useless with only warp and exact case. - I prefer 'Cycle throught the window' instead of 'wrap' this word is not much understood by non-native anglisch spikkers even with the Merriam Webster's dictionnary. - and sth like 'Search as typing' (don't know if it is grammatically correct) instead of 'Incremental...' - If no match: (I hate pop-ups) a red label 'Not Found!' appears: +------------------------------------------------------------------+ | Find in page for: |Bla__|v| (<)(>) Not found! (Clear) (Advanced) | +------------------------------------------------------------------+ - Mozilla window is dynamical. We shouldn't think as ns4.x-developpers but as xul-developpers. - For accessibility keys/focus, I have some ideas that could be discussed in time.

Confirmed plus "me too".

There should be also alerts indicating - Beginning of the page! - End of the page! if the wrap around option is unselected and if there is at least one match. From bug 7930, timeless suggested a Find All facility that would highlight all the matches and that could be useful. (is it a filed bug?) Could be put in the advanced panel as | x Search for all occurences at once. |

What will happen when I hit tab during a search? Will it go to the content area because focus is in a toolbar, or will it go to the next link/form element after the selection (found text) like in IE? (See also bug 66597, "tab should start from insertion point or beginning of selection".)

uid is being phased out.

I'd like to comment that a big problem with dialog box searches is that when browsing with multiple windows, it's easy to get in a situation in which you have 4 browser windows and 3 search boxes open, and no way to tell which search box is associated with which browser window. A toolbar search would be soooo helpful!

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

See bug 162204, IMO the way find should be done and I hope this toolbar will incorporate it. Today ctrl-G to repeat, respects last established search direction, and I like it. Do you propose ctrl-G will change to alias your button (>): always forward (not last established direction)?

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

Reassigning obsolete bugs to their respective Seamonkey owners (i.e. nobody). If you want this fixed for Firefox, change the Product and Component accordingly and reassign back to me.

I fixed bug 55751 and bug 113187. Therefore, we cannot use IME for FAYT on SeaMonkey(Win32 Only). We MUST need to fix this before the release of SeaMonkey with Gecko1.9. However, currently FAYT of Firefox is very buggy. I'm working for the bugs now. If I will finish these working, I will try to fix this. But if you want to work for this, please take this :-)

Some UI for FAYT would be nice definately, and integrating this with non-popup "find in page" UI would be a good idea as well, I'm very opposed to do the same as Firefox though, because the bar at the bottom of the page is very unintuitive and contrary to all other UI concepts we use that have the important UI at the top (toolbars etc., also search in mailnews) or left (sidebar), where the user looks first. Also people should be able to easily pref off the UI for the FAYT case.

> contrary to all other UI concepts we use that have the > important UI at the top (toolbars etc., also search in mailnews) If we will position to top of the content, the content is shaked by the toolbar visible changed. See Asa's comment http://weblogs.mozillazine.org/asa/archives/2005/09/berkun_switches_to_firefox.html > His first criticism is of the Find toolbar's location, at the bottom of the app rather than the top. We tried both configurations and the bottom was the solution that didn't cause the content area to shift down a couple of lines. This seemed much less jarring. We haven't done any serious usability testing on this but we've been following the feedback quite closely and Scott's not alone in this concern.

> Also people should be able to easily pref off the UI for the FAYT case. We cannot do this. See my comment in bug 250309 comment 30 and below.

(In reply to comment #12) > > contrary to all other UI concepts we use that have the > > important UI at the top (toolbars etc., also search in mailnews) > > If we will position to top of the content, the content is shaked by the toolbar > visible changed. I know, and I don't care. We are NOT Firefox, BTW. And: See Asa's comment "We haven't done any serious usability testing on this" So basically he says "we just wanted it that way". It's wrong in UI style though. Additionally - see below. (In reply to comment #13) > > Also people should be able to easily pref off the UI for the FAYT case. > > We cannot do this. See my comment in bug 250309 comment 30 and below. We can do this. The code is not that hard to do (defaulting to "FAYT toolbar on"), and people who don't need IME input might just not want to care about that toolbar popping up. (And remember again, we are NOT Firefox.)

> and people who don't need IME input might just not want to care about > that toolbar popping up. (And remember again, we are NOT Firefox.) I cannot accept it. I think that all feature MUST be internationalized. IME users will say, "Why cannot use IME at Find Toolbar hidden mode? It's a bug, the developers should fix it!".

(In reply to comment #15) > > and people who don't need IME input might just not want to care about > > that toolbar popping up. (And remember again, we are NOT Firefox.) > > I cannot accept it. I think that all feature MUST be internationalized. > IME users will say, "Why cannot use IME at Find Toolbar hidden mode? It's a > bug, the developers should fix it!". Just because there's a bug for IME users in this mode doesn't mean we can't implement it for everyone else. If we would work like this, we'd need to remove FAYT completely for the moment. Would you like us to do that?

> Would you like us to do that? I don't so, therefore, I refacted the FAYT of Firefox after 1.0. (On Firefox 1.0, we couldn't use IME on FAYT) But we should not implement if internationalizable is not in prospect.

Well it's as easy as defaulting it to showing the toolbar (on top) and adding a pref into the FAYT panel of advanced > keyboard naviagtion that states "hide find toolbar (does not work with IME input)". If people are warned about that, this should be enough, IMHO. As I said, the possibilty that it might not work with some input methods is no reason for adding a non-default feature that many of us would like to use. We should explain that possibilty to users who turn it off though.

Additional info. If we implement with Javascript and without editor, central European cannot input accents. See bug 270936.

I'm resetting bugs which are assigned to me but I'm not working on them and I don't have plan for fixing them in near future.

This patch makes the findbar come up as a toolbar above the tabbrowser when invoking find in page instead of the dialog. It also adds a preference so that the findbar can be hidden (and use the current FAYT code) for auto-started find as you type; however, there is no support in the findbar binding right now to allow it to be hidden when manually invoking find as you type. Also, the current code allows a browser.findbar.enabled pref to be set to false to switch back to using the dialog; however, there is no preference UI for that as I am not sure where to put it. Also, while the findbar looks good in modern, it could use some theme work in Mac OS classic, which I assume should be a separate bug. (I'm not setup to build Seamonkey on Windows and Linux right now.) Does this seem like a good approach? Is it still a desired feature?

Comment on attachment 408318 [details] [diff] [review] Findbar Patch v.1 To fix the issue of not being able to disable the findbar completely, we could write an extension to the findbar binding, like we did for the notification, preferences and toolbar bindings. >+pref("browser.findbar.enabled", true); ... >+pref("accessibility.typeaheadfind.seamonkeyautostart", true); >+pref("accessibility.typeaheadfind.usefindbar", true); What's the difference between these preferences? >+ <field name="_fastFind">null</field> There were 7 new whitespace errors in your patch. This is just one of them. [There were also 4 old errors; it would be nice if you could fix them too.] >+ document.getElementById("FindToolbar").onFindCommand(); Not all windows have a FindToolbar element... >+ else > { >+ // is the dialog up already? >+ if ("findDialog" in window && window.findDialog) Nit: could write "else if"

(In reply to comment #23) > Does this seem like a good approach? Is it still a desired feature? It's surely still desired, I'll look at how it looks once 2.0 is safely out the door ;-)

(In reply to comment #23) > Created an attachment (id=408318) [details] > Findbar Patch v.1 > > This patch makes the findbar come up as a toolbar above the tabbrowser when > invoking find in page instead of the dialog. Even though I don't know if it will work I'm curious to know if you have considered doing it like Safari (below the tabs and search field at the right)? > Also, while the findbar looks good in modern, it > could use some theme work in Mac OS classic, which I assume should be a > separate bug. Yeah, definitely a separate bug. But it doesn't look too bad, I think. Might need to re-work the tabstrip, though (but you shouldn't worry about that).

(In reply to comment #24) > (From update of attachment 408318 [details] [diff] [review]) > To fix the issue of not being able to disable the findbar completely, we could > write an extension to the findbar binding, like we did for the notification, > preferences and toolbar bindings. Done. > >+pref("browser.findbar.enabled", true); > ... > >+pref("accessibility.typeaheadfind.seamonkeyautostart", true); > >+pref("accessibility.typeaheadfind.usefindbar", true); > What's the difference between these preferences? Browser.findbar.enabled enables the findbar instead of the dialog for searches started from the edit menu, while accessibility.typeaheadfind.usefindbar enables the findbar for type ahead find searches instead of the status bar type ahead find. The accessibility.typeaheadfind.seamonkeyautostart preference was used to keep the preference to automatically start find as you type separate from accessibility.typeaheadfind which the find bar checked, but since the I have now overridden the findbar binding, we can just use accessibility.typeaheadfind.seamonkeyautostart and have both the findbar and the status bar type ahead find check the value of accessibility.typeaheadfind.usefindbar. > >+ <field name="_fastFind">null</field> > There were 7 new whitespace errors in your patch. This is just one of them. > [There were also 4 old errors; it would be nice if you could fix them too.] I should have fixed the white space errors in my patch, and anything else I could find. > >+ document.getElementById("FindToolbar").onFindCommand(); > Not all windows have a FindToolbar element... Now, if there is not a FindToolbar element, we will revert back to the dialog. > >+ else > > { > >+ // is the dialog up already? > >+ if ("findDialog" in window && window.findDialog) > Nit: could write "else if" Done. > Even though I don't know if it will work I'm curious to know if you have > considered doing it like Safari (below the tabs and search field at the right)? I'd be willing to implement that if it's preferred. Also, I added the findbar to the top of the view source window as well. Finally, while adding the new preference, I noticed that the disabling of the "Links only" and "Any text in the page" radio buttons didn't work when use find as you type was unchecked (even in 2.0). It is fixed in this patch, but I could move the small fix to a separate patch if desired.

This patch is the same as the previous patch, except that it actually contains the white space cleanup.

(In reply to comment #27) > Finally, while adding the new preference, I noticed that the disabling of the > "Links only" and "Any text in the page" radio buttons didn't work when use find > as you type was unchecked (even in 2.0). It is fixed in this patch, but I > could move the small fix to a separate patch if desired. Yes, and file a new bug please, so we can fix it in 2.0.1 too.

(In reply to comment #27) > > Even though I don't know if it will work I'm curious to know if you have > > considered doing it like Safari (below the tabs and search field at the right)? > > I'd be willing to implement that if it's preferred. That would be extremely cool imo, but you might want to get the idea confirmed by your reviewers before you start doing any work. Also, since the content area will decrease in height you'll need to compensate for that in some way (otherwise the page will jump downwards).

This patch changes initPrefs() to initFindPrefs() as Chatzilla already uses initPrefs(), resulting in Chatzilla working again. I removed the patch for 524886 from my patch, and I removed the code to disable the "Show find toolbar during find as you type" checkbox since that option still applies to the manually started find as you type. Also, I want to clarify that accessibility.typeaheadfind.autostart is being modified instead of accessibility.typeaheadfind.seamonkeyautostart by the preference pane now and used by both the overridden findbar and Seamonkey's status bar type ahead find.

Comment on attachment 409432 [details] [diff] [review] Findbar Patch v.3 From the UI POV, I like this as a first step - it's much better than the popup window and it allows people with an IME to use find as you type by flipping that pref. I still think it would be nice to get it into the content area, ideally along with some way to not shift down a page that has scollbars enabled, but we can do that in a followup patch.

Neil, can we move this patch forward?

Comment on attachment 409432 [details] [diff] [review] Findbar Patch v.3 One problem I noticed with this was that the menuitems for typeaheadfind did not invoke the findbar. (Obviously this should use the usefindbar pref.) >-pref("browser.startup.homepage", "chrome://navigator-region/locale/region.properties"); >+pref("browser.startup.homepage", "chrome://navigator-region/locale/region.properties"); Nit: some miscellaneous whitespace changes crept in to the patch. Rather annoying given that the patch contains 12 new whitespace inconsistencies. > <vbox id="appcontent" flex="1" > ondragdrop="nsDragAndDrop.drop(event, contentAreaDNDObserver);"> > >+ <findbar id="FindToolbar" browserid="content"/> Nit: not sure what that blank line was doing there before, but I think it would be slightly nicer to put the findbar before it. >+ default: >+ // Don't start if typeahead find is disabled or >+ // if findbar is taking care of type ahead find >+ if (!this.mPrefs.getBoolPref("autostart") || >+ this.mPrefs.getBoolPref("usefindbar")) I'm not sure this is necessary, the findbar should call preventDefault() already if it is active. >+ <field name="_fastFind">null</field> Nit: incorrect indentation >+ <property name="fastFind" readonly="true"> There's no obvious place to add these, but between goHome and homePage looks wrong; try making it either the first thing in the implementation or the third last i.e. just before the constructor. >+ Seamonkey Flexibile Findbar Flexible >+ <binding id="findbar" >+ extends="chrome://global/content/bindings/findbar.xml#findbar"> >+ <implementation> >+ <constructor><![CDATA[ You should find that you don't need most of this since the extended findbar constructor will run anyway. >+ prefsvc.addObserver("accessibility.typeaheadfind.usefindbar", >+ this._observer, false); You never remove this observer. >+ <field name="_observer"><![CDATA[({ I'm not sure whether it will help, but if you give your observer a different name then the toolkit findbar's observer will do the prefs that it knows about and your observer only has to know about the usefindbar pref. >+ if (gFindPrefs.getBoolPref("enabled") && findToolbar) Nit: check findToolbar first before the enabled pref. (Possibly even change initPrefs into isFindToolbarEnabled?) >+ return true; >+ else Nit: don't need else after return. > <groupbox align="start"> > <caption label="&findAsYouTypeBehavior.label;"/> > <checkbox id="findAsYouTypeEnableAuto" > label="&findAsYouTypeEnableAuto.label;" > accesskey="&findAsYouTypeEnableAuto.accesskey;" > preference="accessibility.typeaheadfind.autostart"/> >+ <checkbox id="findAsYouTypeFindbarEnable" >+ class="indent" >+ label="&findAsYouTypeFindbarEnable.label;" >+ accesskey="&findAsYouTypeFindbarEnable.accesskey;" >+ preference="accessibility.typeaheadfind.usefindbar"/> >+ <description>&findAsYouTypeFindbarEnableTip.label;</description> This checkbox makes no sense here. The sense of the flow is as follows: >Find automatically when typing within a web page: Any text in the page / Links only. I would say that it probably belongs after the sound and timeout checkboxes. (Move the <preference> element too of course.) >+<!ENTITY findAsYouTypeFindbarEnable.label "Show find toolbar during find as you type"> >+<!ENTITY findAsYouTypeFindbarEnable.accesskey "S"> >+<!ENTITY findAsYouTypeFindbarEnableTip.label "Find as you type without showing the findbar does not allow international text entry."> Nit: This should say "Note: Find as you type without ...

Benjamin: Can you update that patch for the review comments?

Sorry about the delay. I have been busy with school. This patch should address all of Neil's comments. I'm uncomfortable changing utilityOverlay.xul to get the find as you type menu items to work, but I figured it was better than changing nsTypeAheadFind.js or making the findbar able to respond to goDoCommand. I'm open to suggestions though. Thanks.

Comment on attachment 438579 [details] [diff] [review] Findbar Patch v.4 >+ return (document.getElementById("FindToolbar") >+ && gFindPrefs.getBoolPref("enabled")); Nit: && belongs at end of previous line >+function findLinksAsType() >+function findTextAsType() I'm not enthralled by these names, but then again the best suggestion I have is to insert "You" between "As" and "Type". >+ var findbar = document.getElementById("FindToolbar"); >+ if (isFindbarEnabled()) >+ findbar.open(findbar.FIND_LINKS); findbar.open doesn't quite do all the things you need. See _onBrowserKeypress > <command id="cmd_findTypeText" >- oncommand="goDoCommand('cmd_findTypeText')"/> >+ oncommand="findTextAsType()"/> > <command id="cmd_findTypeLinks" >- oncommand="goDoCommand('cmd_findTypeLinks')"/> >+ oncommand="findLinksAsType()"/> Nit: include a ; in the statement

Comment on attachment 438579 [details] [diff] [review] Findbar Patch v.4 >+ this._useTypeAheadFind = >+ prefsvc.getBoolPref("accessibility.typeaheadfind.autostart"); This doesn't quite work because the base binding is trying to update the same value, but for the accessibility.typeaheadfind preference. I guess we have to hope that nobody will actually change it. Or maybe removing the toolkit's observer for the accessibility.typeaheadfind preference will work. >+ } >+ } Nit: indentation is one space too few. >+var gFindPrefs; We can now use Services.prefs.getBoolPref directly, simplifying this code. In fact, it might be worth eliminating isFindbarEnabled altogether, like this: function findInPage(findInstData) { var findbar = document.getElementById("FindToolbar"); if (findbar && Services.prefs.getBoolPref("browser.findbar.enabled")) findbar.onFindCommand(); else ... >+function findLinksAsType() >+{ >+ var findbar = document.getElementById("FindToolbar"); >+ if (isFindbarEnabled()) This is wrong since isFindbarEnabled checks the pref for the find dialog but you want to check the pref for the find as you type. >+ findbar.open(findbar.FIND_LINKS); Following on from my point about this not being enough to start a new type ahead find, it might be an idea to write a findbar.startNewFind method to avoid duplicating the code for links and text.

Note that bug 411754 now has removed our our view source and uses the toolkit one (with the findbar displayed at the top though) so that part/line of this patch can be dropped.

Ben, any chance we can have an updated patch this week? It looks like we're freezing for Alpha 2 on the 22nd, and I'd love to see this in the code until then...

Comment on attachment 451474 [details] [diff] [review] Findbar Patch v.5 This should address all of Neil's issues, except it brings up a problem/bug and a few things to discuss. Thanks for waiting. Hopefully, we can get something in by code freeze. Problem (Bug): If the user invokes FAYT on either about:blank or about:config using the keyboard with the findbar enabled, the component based FAYT is activated as findbar's FAYT will not start on either URL via the keyboard, but the component FAYT will. I attached my attempted patch to disable component FAYT on about:config or about:blank. I tried to get it working during my free time these past few days and I haven't figured out why the comparison is failing. Any advice? For Discussion: 1) Whether the findbar as implemented now should be in Alpha 2 as it: - a) is located above the tab bar - b) is decent looking but doesn't blend in with the other toolbars in any theme, especially on Mac OS X Classic - c) problems may be introduced with mozilla-central fixes for the lazy loading findbar in Firefox, where it is currently not lazy loading in this patch - d) nsTypeAheadFind.js will have to be updated to not use XPCOMComponentUtils - e) see issue above (separate patch? file another bug? include about:addons?) 2) Future Findbar directions (File a meta-bug and dependent bug for each? Should any be a part two of this bug? Good plan?): - a) Fix 1.e - b) Fix the toolbar appearance - c) Help out with lazy loading findbar.xml fixes, and then lazy load findbar in SeaMonkey too - d) Fix 1.a. by porting bug 347930 to SeaMonkey (perhaps switch to extending Firefox's tabbrowser.xul with the previews and customizable close button?) - e) Make sure nsTypeAheadFind.js keeps working In my opinion, even with the above issues, it is an improvement that should go in after review+, but perhaps some of these should be put in the release notes if they aren't fixed by code freeze. Of course, I'm open though. I will see what can do in getting to the future steps before then, but any help would appreciated.

> Problem (Bug): > If the user invokes FAYT on either about:blank or about:config using the > keyboard with the findbar enabled, the component based FAYT is activated as > findbar's FAYT will not start on either URL via the keyboard, but the component > FAYT will. I attached my attempted patch to disable component FAYT on > about:config or about:blank. I tried to get it working during my free time > these past few days and I haven't figured out why the comparison is failing. > Any advice? Related bugs? Bug 569342 - Find bar should not be enabled in about:addons Bug 570760 - Make ctrl-f and / focus the search box in the add-ons manager. Bug 567306 - Find command(ctrl+F) does not start looking for it with a selected text on the actual page.

Comment on attachment 451474 [details] [diff] [review] Findbar Patch v.5 Mostly OK, but I did seem to find some new nits that I'd maybe missed before... >+ // disable FAYT in about:config and about:blank to prevent FAYT >+ // opening unexpectedly - to fix bugs 264562, 267150, 269712 FAYT should be pretty hard to accidentally trigger in about:config since it only contains a textbox and a tree, and they both eat keypresses. IMHO about:blank is actually a bug, since a document could easily contain a blank frame and inject nodes into it. So maybe we should ignore them. >+ Seamonkey Flexible Findbar Nit: SeaMonkey >+ xmlns:xbl="http://www.mozilla.org/xbl" >+ xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> Nit: these two attributes are only used in conjunction with <content>. >+ prefsvc.removeObserver("accessibility.typeaheadfind", >+ this._observer); Nit: incorrect alignment ^ >+ <field name="_smObserver"><![CDATA[({ Nit: Would you mind calling this _suiteObserver? >+ prefsvc.removeObserver("accessibility.typeaheadfind", >+ this._observer); Nit: didn't you remove this in the constructor? >+ if (elt instanceof HTMLInputElement) { >+ // block FAYT when an <input> textfield element is focused >+ var inputType = elt.type; >+ switch (inputType) { >+ case "text": >+ case "password": >+ case "file": >+ return false; >+ } Nit: toolkit code changed here to work with alternate input types. >+ // disable FAYT in about:config and about:blank to prevent FAYT >+ // opening unexpectedly - to fix bugs 264562, 267150, 269712 See above. >+ <method name="smMenuFastFind"> Nit: This isn't very suite-specific so you could just call it startFastFind, or maybe suiteFastFind or suiteStartFastFind. >+ >+ Nit: doubled blank line >+ <checkbox id="findAsYouTypeFindbarEnable" >+ label="&findAsYouTypeFindbarEnable.label;" >+ accesskey="&findAsYouTypeFindbarEnable.accesskey;" >+ preference="accessibility.typeaheadfind.usefindbar"/> > </vbox> >+ <description>&findAsYouTypeFindbarEnableTip.label;</description> I need to check against my Windows build, but on my Linux build this overflows the pref pane :-( One workaround is to make the radiogroup horizontal. >+<!ENTITY findAsYouTypeFindbarEnable.label "Show find toolbar during find as you type"> Nit: Show the find toolbar

(In reply to comment #42) > For Discussion: > 1) Whether the findbar as implemented now should be in Alpha 2 as it: > - a) is located above the tab bar We should look into tab bar issues anyhow, let's file a followup bug for this issue. > - b) is decent looking but doesn't blend in with the other toolbars in any > theme, especially on Mac OS X Classic That's another thing that shouldn't block us but go into a followup bug. > - c) problems may be introduced with mozilla-central fixes for the lazy loading > findbar in Firefox, where it is currently not lazy loading in this patch We should try to have this patch working at the point in time where it's landing in the tree, anything else can be addressed later. > - d) nsTypeAheadFind.js will have to be updated to not use XPCOMComponentUtils Why that? > In my opinion, even with the above issues, it is an improvement that should go > in after review+ I agree. We are still in alpha state anyhow.

(In reply to comment #44) >(From update of attachment 451474 [details] [diff] [review]) >>+ <checkbox id="findAsYouTypeFindbarEnable" >>+ label="&findAsYouTypeFindbarEnable.label;" >>+ accesskey="&findAsYouTypeFindbarEnable.accesskey;" >>+ preference="accessibility.typeaheadfind.usefindbar"/> >> </vbox> >>+ <description>&findAsYouTypeFindbarEnableTip.label;</description> >I need to check against my Windows build, but on my Linux build this overflows >the pref pane :-( This looks fine on Windows, so maybe it's an issue with my Linux build. Anyone else care to comment?

Comment on attachment 451474 [details] [diff] [review] Findbar Patch v.5 r=me if you remove the about:blank/config checks and fix the other minor nits (don't worry about the pref pane for now).

(In reply to comment #46) > >I need to check against my Windows build, but on my Linux build this overflows > >the pref pane :-( > This looks fine on Windows, so maybe it's an issue with my Linux build. Anyone > else care to comment? I cannot see an issue there here on Kubuntu Lucid Lynx (pretty much with default settings, after all this is just my newly setup Linux test VM). Maybe it'd help if you added a screenshot. Pro Tip: Request feedback on an attachment from specific persons to actually get feedback. ;-)

(In reply to comment #48) > Pro Tip: Request feedback on an attachment from specific persons to actually > get feedback. ;-) Erm, the patch has review. The only thing needed here is an updated patch for the latest comments. Unfortunately, this has missed Alpha 2 just because that updated patch didn't come around, but I hope we get that fast so we can drive Benjamin's cool work here into the tree!

Thanks for the reviews, and sorry about the delay. This should address all of Neil's comments, and should hopefully be ready for check-in. I renamed smMenuFastFind to startFastFind. The preferences dialog looks fine on Mac OS X 10.6 and Ubuntu 10.04 with the default settings for me.

> +findbar[xpfe="false"] { > + -moz-binding: url("chrome://communicator/content/bindings/findbar.xml#findbar"); > +} > +findbar { > + -moz-binding: url("chrome://communicator/content/bindings/findbar.xml#findbar"); > +} Hmm? Typo? In Firefox toggleAffectedChrome() opens/closes the findbar. Don't we have to do the same? In Firefox there is this comment: /** * Returns true if |aMimeType| is text-based, false otherwise. * * @param aMimeType * The MIME type to check. * * If adding types to this function, please also check the similar * function in findbar.xml */ function mimeTypeIsTextBased(aMimeType) Perhaps we should add a similar comment to our implementation of mimeTypeIsTextBased()

(In reply to comment #51) > > +findbar[xpfe="false"] { > > + -moz-binding: url("chrome://communicator/content/bindings/findbar.xml#findbar"); > > +} > > +findbar { > > + -moz-binding: url("chrome://communicator/content/bindings/findbar.xml#findbar"); > > +} > Hmm? Typo? > > In Firefox toggleAffectedChrome() opens/closes the findbar. Don't we have to do > the same? Looks like it. > In Firefox there is this comment: > > /** > * Returns true if |aMimeType| is text-based, false otherwise. > * > * @param aMimeType > * The MIME type to check. > * > * If adding types to this function, please also check the similar > * function in findbar.xml > */ > function mimeTypeIsTextBased(aMimeType) > > Perhaps we should add a similar comment to our implementation of > mimeTypeIsTextBased() We could do but that's beyond the scope of this bug.

Comment on attachment 456771 [details] [diff] [review] Findbar Patch v.6 >+findbar[xpfe="false"] { >+ -moz-binding: url("chrome://communicator/content/bindings/findbar.xml#findbar"); >+} r=me with this fixed. It would be nice if you could fix toggleAffectedChrome too but if you prefer you can do it in a follow up patch.

This should address Neil's and Philip's comments. I created bug 580520 for the comment fix.

(In reply to comment #54) > Created attachment 458910 [details] [diff] [review] > Findbar Patch v.7 Wrong fix for comment #53 - the problem was that the [xpfe="false"] binding had the wrong URL which needed to be corrected, not removed ;-) toggleAffectedChrome changes look fine though.

Ah, I took a look at communicator.css again, and I now understand what xpfe="false" should do. This patch should have the proper URL and apply again to the trunk. Hopefully, it's finally ready for check-in! (Fingers crossed.)

@Ben: Since this affects UI, please also request sr from Neil before setting checkin-needed. Should be quick.

Comment on attachment 459774 [details] [diff] [review] Findbar Patch v.8 I was going to check it in later today anyway...

Pushed changeset fbc3a0154606 to comm-central.

<!ENTITY findAsYouTypeFindbarEnableTip.label "Note: Find as you type without showing the findbar does not allow international text entry."> IMHO, "international text" is very confusing wording. What exactly does that mean? Does it only affect complex input methods, or anything that doesn't fall under ASCII? Plus, I guess a Japanese person would hardly clasify solely Japanese text as international...

(In reply to comment #61) > <!ENTITY findAsYouTypeFindbarEnableTip.label "Note: Find as you type without > showing the findbar does not allow international text entry."> > > IMHO, "international text" is very confusing wording. You're right, the wording is very confusing. Feel free to improve it. > Does it only affect complex input methods, or anything that doesn't fall > under ASCII? I'm not really sure. It might affect dead keys, or it might not.

I'm actually not sure if that note there is worth having at all.

The Japanese localizers would probably traslate this into something understandable. Also looking at the dependency tree, moving to a findbar probably fixes a problem with IME not working with the statusbar FYAT. So I'd guess the Japanese text would probably say something like "Enable the findbar if you want to search using Japanese input method."

And what do I say for Lithuanian users?

1) We should handle this in a followup, 2) We might want to rephrase this to talk about either "...if you want to use an IME for searching" or "...if you want to input characters not on a normal keyboard (via IME)" - or remove that message altogether and describe this in help only.

Comment on attachment 459774 [details] [diff] [review] Findbar Patch v.8 > content/communicator/bindings/general.xml (bindings/general.xml) >+ content/communicator/bindings/findbar.xml (bindings/findbar.xml) I failed to notice that these were in the wrong order, so I pushed changeset d29fbbf9a3dc to comm-central to fix that and one other mismerge that isn't apparent from the patch because it doesn't have enough context.

This patch: * Reuses findbar where it wasn't being; * Replace missing isFindbarEnabled with relevant code.

That reminded me of the following problem: 1. Turn off the "Show the find toolbar during find as you type" in Preferences 2. Start a type ahead find (in-page) 3. Try to find next In this case, you would expect find next to find next in-page rather than open the find bar. This patch fixes that. I had to roll your patch in to the fix.

Comment on attachment 470425 [details] [diff] [review] Fix for find again >diff -r b0c0a81d13cb suite/common/findUtils.js >--- a/suite/common/findUtils.js Sun Aug 29 12:40:20 2010 +0200 >+++ b/suite/common/findUtils.js Mon Aug 30 14:52:45 2010 +0100 >@@ -100,7 +100,15 @@ function findAgainInPage(findInstData, r > { > var findbar = document.getElementById("FindToolbar"); > if (findbar && Services.prefs.getBoolPref("browser.findbar.enabled")) >- document.getElementById("FindToolbar").onFindAgainCommand(reverse); >+ { >+ // first, look to see whether XPFE typeaheadfind wants to find next >+ var sip = Components.classes['@mozilla.org/supports-interface-pointer;1'] Nit: use " instead of '

Pushed changeset 57162e47434c to comm-central.

Just be happy I wasn't around this weekend and today. PLEASE FILE FOLLOWUPS for followup fixes and don't pointlessly reopen bugs that have been fixed, spamming ALL dependencies unneededly with bugspam about reopening and re-fixing? Pretty please? Thanks.