Bug 288254 (xblfindbar)

Findbar XBL Widget

RESOLVED FIXED in mozilla1.9alpha1

Status

()

P2
normal
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9alpha1
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 17 obsolete attachments)

151.59 KB, patch
Details | Diff | Splinter Review
7.76 KB, patch
Details | Diff | Splinter Review
12.62 KB, patch
mano
: review+
Details | Diff | Splinter Review
We have all sort of extra hacks in browser.js to handle something which lives in
toolkit, having the findbar as a xbl widget will help (e.g. constructor instead
of Startup() hacks).
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → Future
Another resonable option is to use the browser binding, something like:

<browser ...findabr="true"/>
hmm, browser.xml already include findbar.dtd (that was *before* findbar.dtd was
cvs added).
QA Contact: fast.find
Priority: P3 → P2
Target Milestone: Future → Firefox 3 alpha2
Created attachment 242718 [details] [diff] [review]
yet another checkpoint

Including winstripe, view-source and help viewer changes (and few fixes in the binding itself).
Alias: xblfindbar
Summary: Findbar should be a toolkit xbl widget rather than a bogus .inc → Findbar XBL Widget
Created attachment 243270 [details] [diff] [review]
ready for first review

TODO: Thunderbird changes, copy pinstripe "hover" images to toolkit/themes.
Attachment #242782 - Attachment is obsolete: true
Attachment #243270 - Flags: review?(gavin.sharp)
Also, need to set the preferences in all.js.
Created attachment 243330 [details] [diff] [review]
Some minor fixes
Attachment #243270 - Attachment is obsolete: true
Attachment #243330 - Flags: review?(gavin.sharp)
Attachment #243270 - Flags: review?(masayuki)
Attachment #243270 - Flags: review?(gavin.sharp)
Looks great for me. But I cannot review in several days. Are you O.K.?

I found some nits:

> Index: toolkit/themes/pinstripe/global/findBar.css

> +findbar > toolbarbutton.findbar-find-next:not([disabled]):hover > .toolbarbutton-text, 
> +findbar > toolbarbutton.findbar-find-previous:not([disabled]):hover > .toolbarbutton-text,
> +findbar > toolbarbutton.findbar-highlight:not([disabled]):hover > .toolbarbutton-text {
> +  background: url("chrome://browser/skin/bookmark-hover-right.png") no-repeat right center;
> +  color: #fff;
>  }
>  
> +findbar > toolbarbutton.findbar-find-next:not([disabled]):hover,
> +findbar > toolbarbutton.findbar-find-previous:not([disabled]):hover,
> +findbar > toolbarbutton.findbar-highlight:not([disabled]):hover
> +{
> +  background:url("chrome://browser/skin/bookmark-hover-left.png") no-repeat left center;
> +}

In the second case, you added the '{' on the head of the line. But on other places, it's not so. You should use same style. I.e.,

> +findbar > toolbarbutton.findbar-highlight:not([disabled]):hover {



> Index: toolkit/content/jar.mn

>  *+ content/global/bindings/wizard.xml          (widgets/wizard.xml)
> +*+ content/global/bindings/findbar.xml          (widgets/findbar.xml)

Please cut a space.(wrong indent)

I'll review this in next week, sorry.
Next week is fine, thanks. Partial-reviews are fine as well, This is Fx3 stuff and I'm sure going to tweak a lot of this code once it lands.

Updated

12 years ago
Blocks: 250910
Comment on attachment 243330 [details] [diff] [review]
Some minor fixes

+      <method name="_dispatchKeypressEvent">

> +          event.initKeyEvent(aEvent.type, aEvent.canBubble, aEvent.cancelable,

This code has my mistake. Please bug 359660.

aEvent.canBubble -> aEvent.bubbles

+      <method name="_updateStatusUIBar">

+          if (!this._textToSubURIService) {
+            this._textToSubURIService =
+               Components.classes["@mozilla.org/intl/texttosuburi;1"]
+                         .getService(Components.interfaces.nsITextToSubURI);
+          }

wrong indent the line of "Components.....".

+      <method name="_shouldFastFind">

+          if (aEvent.ctrlKey || aEvent.altKey || aEvent.metaKey ||
+              aEvent.getPreventDefault())
+              return false;

wrong indent of the line of return statement.

+      <method name="_onBrowserKeypress">

+          var key = aEvent.charCode ? String.fromCharCode(aEvent.charCode) : null;
+          if (key && (key == "'" || key == "/" || (this._useTypeAheadFind && key != " "))) {

I don't like this code. We should use member variables for readable. But the names of the variables of old code is not good.
Note that I'll need to work for customizable the characters for other keyboard layout users.(e.g., in JIS keyboard layout, the shortcut keys are not useful keys.) Therefore, I hope that you use meaningful names for the variables. e.g., "_key_for_find_links".

+            if (this.open(mode)) {
+              this._setFindCloseTimeout();
+              this._findField.select();
+              this._findField.focus();
+              if (this._useTypeAheadFind && key != "'" && key != "/")
+                this._dispatchKeypressEvent(this._findField.inputField, aEvent);
+              else
+                this._updateStatusUI(this.nsITypeAheadFind.FIND_FOUND);
+              aEvent.preventDefault();
+            }
+            else {
+              this._findField.select();
+              this._findField.focus();
+              if (this._findMode != this.FIND_NORMAL) {
+                if (key != "'" && key != "/")
+                  this._dispatchKeypressEvent(this._findField.inputField, aEvent);
+                else
+                  this._updateStatusUI(this.nsITypeAheadFind.FIND_FOUND, false);
+                aEvent.preventDefault();
+              }
+            }

What's this code?
The difference is only the line of "this._setFindCloseTimeout();"?
If so, you can rewrite to more simple code...

Otherwise, looks good for me.
(In reply to comment #13)
> This code has my mistake. Please bug 359660.

Please fix bug 359660 too, sorry.

> +            if (this.open(mode)) {
> +              this._setFindCloseTimeout();
> +              this._findField.select();
> +              this._findField.focus();
> +              if (this._useTypeAheadFind && key != "'" && key != "/")
> +                this._dispatchKeypressEvent(this._findField.inputField,
> aEvent);
> +              else
> +                this._updateStatusUI(this.nsITypeAheadFind.FIND_FOUND);
> +              aEvent.preventDefault();
> +            }
> +            else {
> +              this._findField.select();
> +              this._findField.focus();
> +              if (this._findMode != this.FIND_NORMAL) {
> +                if (key != "'" && key != "/")
> +                  this._dispatchKeypressEvent(this._findField.inputField,
> aEvent);
> +                else
> +                  this._updateStatusUI(this.nsITypeAheadFind.FIND_FOUND,
> false);
> +                aEvent.preventDefault();
> +              }
> +            }
> 
> What's this code?
> The difference is only the line of "this._setFindCloseTimeout();"?
> If so, you can rewrite to more simple code...

Oh, sorry. I can find another difference lines. But I still think that the code should be more simple... (I feel sleepy. Therefore, I may be wrong...)
Created attachment 244933 [details] [diff] [review]
patch
Attachment #243330 - Attachment is obsolete: true
Attachment #244933 - Flags: review?(gavin.sharp)
Attachment #243330 - Flags: review?(masayuki)
Attachment #243330 - Flags: review?(gavin.sharp)
Created attachment 244984 [details]
very first draft of a unit test
Created attachment 244997 [details] [diff] [review]
patch

De-delay close(), I don't think this is still necessary.
Attachment #244933 - Attachment is obsolete: true
Attachment #244997 - Flags: review?(gavin.sharp)
Attachment #244933 - Flags: review?(gavin.sharp)
Created attachment 244998 [details]
unit test v2
Attachment #244998 - Flags: review?(gavin.sharp)
Comment on attachment 244998 [details]
unit test v2

><window id="FindbarTest" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" width="600" height="600" onload="onLoad();" title="findbar test"><script type="application/javascript"/><commandset><command id="cmd_find" oncommand="document.getElementById('FindToolbar').onFindCommand();"/></commandset><browser type="content-primary" flex="1" id="content" src="about:blank"/><findbar id="FindToolbar" browserid="content"/></window>
Attachment #244998 - Flags: review?(sayrer)

Comment 21

12 years ago
Comment on attachment 244998 [details]
unit test v2

yeah, this is pretty much exactly what you would need to write. we'll get you a way to be a chrome URL.
Attachment #244998 - Flags: review?(sayrer) → review+

Comment 23

12 years ago
Comment on attachment 245499 [details] [diff] [review]
mail/ changes

looks ok to me...
Attachment #245499 - Flags: review?(bienvenu) → review+
Comment on attachment 244997 [details] [diff] [review]
patch

nit: lots of trailing spaces in findbar.xml.

>Index: toolkit/components/help/content/help.xul

>@@ -69,22 +66,19 @@
> #else
>         persist="width height screenX screenY"
> #endif
>         onload="init();"
>         onunload="gFindBar.uninitFindBar(); window.XULBrowserWindow.destroy();">

Forgot to remove this?

>Index: toolkit/content/jar.mn

> *+ content/global/bindings/wizard.xml          (widgets/wizard.xml)
>+*+ content/global/bindings/findbar.xml          (widgets/findbar.xml)

nit: spacing

>Index: toolkit/content/widgets/findbar.xml

>+      <field name="FIND_NORMAL">0</field>
>+      <field name="FIND_TYPEAHEAD">1</field>
>+      <field name="FIND_LINKS">2</field>
>+
>+      <field name="TAF_LINKS_KEY">"'"</field>
>+      <field name="TAF_TEXT_KEY">"/"</field>

Set readonly on these?

>+      <field name="_findMode">0</field>
>+      <field name="_tmpOutline">null</field>
>+      <field name="_tmpOutlineOffset">"0"</field>
>+      <field name="_drawOutline">false</field>
>+
>+      <field name="_flashFindBar">0</field>
>+      <field name="_flashFindBarCount">6</field>

There doesn't seem to be a field for _foundLink, _foundEditable, _currentWindow, etc, which means they're sometimes undefined. I guess that doesn't really matter, but for consistency it'd probably be best to add some?

>+      <field name="_observer"><![CDATA[({
>+        _self: this,
>+
>+        QueryInterface: function(iid) {
>+          if (iid.equals(Components.interfaces.nsIObserver) ||
>+              iid.equals(Components.interfaces.nsISupportsWeakReference) ||
>+              iid.equals(Components.interfaces.nsISupports))
>+             return this;
>+
>+           throw Components.results.NS_ERROR_NO_INTERFACE;

nit: fix indent

>+        observe: function(aSubject, aTopic, aPrefName) {
>+          if (aTopic != "nsPref:changed")
>+            return;
>+
>+          var prefsvc =
>+            Components.classes["@mozilla.org/preferences-service;1"]
>+                      .getService(Components.interfaces.nsIPrefBranch2);

You can just use aSubject QIed to nsIPrefBranch2 here.

>+      <constructor><![CDATA[

>+        // Convenience
>+        this.nsITypeAheadFind = Components.interfaces.nsITypeAheadFind;
>+        this.nsISelectionController = Components.interfaces.nsISelectionController;

nit: newline here to seperate the two unrelated blocks

>+        var _delayedInit = function(aSelf) {
>+          // The browser element might be bound after the findbar
>+          if (aSelf.hasAttribute("browserid")) {
>+            aSelf.browser =
>+              document.getElementById(aSelf.getAttribute("browserid"));
>+          }
>+        };
>+        setTimeout(_delayedInit, 0, this);
>+      ]]></constructor>

>+      <method name="_highlightDoc">

>+            var finder = Components.classes["@mozilla.org/embedcomp/rangefind;1"]
>+                                   .createInstance()
>+                                   .QueryInterface(Components.interfaces.nsIFind);

Make that:
+            var finder = Components.classes["@mozilla.org/embedcomp/rangefind;1"]
+                                   .createInstance(Components.interfaces.nsIFind);

>+          var baseNode = doc.createElementNS("http://www.w3.org/1999/xhtml", "span");
>+          baseNode.style.backgroundColor = aHighBackColor;
>+          baseNode.style.color = aHighTextColor;
>+          baseNode.style.display = "inline";
>+          baseNode.style.fontSize = "inherit";

You lost a padding = 0 here, does it matter?

>+      <method name="_highlightText">

>+          while((retRange = finder.Find(aWord, this._searchRange,
>+                                  this._startPt, this._endPt))) {

nit: fix indent

>+      <!--
>+        - Sets the findbar case-sensitivity mode
>+        - @param aCaseSensitive (boolean)
>+        -        Whether or not case-sesitivity should be turned on.
>+        -->

typo: sensitivity, and trailing whitespace.

>+      <method name="_setCaseSensitivity">
>+        <parameter name="aCaseSensitive"/>
>+        <body><![CDATA[
>+          var prefsvc =
>+            Components.classes["@mozilla.org/preferences-service;1"]
>+                      .getService(Components.interfaces.nsIPrefBranch2);
>+
>+          // Just set the pref; our observer will change the find bar behavior
>+          prefsvc.setIntPref("accessibility.typeaheadfind.casesensitive",
>+                             aCaseSensitive);

aCaseSensitive ? 1 : 0 is clearer, I think.

>+      <method name="_setFoundLink">

>+            aFoundLink.style.outline = "1px dotted invert";
>+            aFoundLink.style.outlineOffset = "0";

Make these easy-to-change fields?

>+      <method name="_finishFAYT">
>+        <parameter name="aKeypressEvent"/>
>+        <body><![CDATA[
>+          try {
>+            if (this._foundLink)
>+              this._foundLink.focus();
>+            else if (this._foundEditable) {
>+              this._foundEditable.focus();
>+              var fastFind = this._browser.fastFind;
>+              fastFind.collapseSelection();
>+            }
>+            else if (this._currentWindow)
>+              this._currentWindow.focus();

Although it isn't documented in nsITypeAheadFind.idl, I think its impossible for both _foundLink and _foundEditable to be null while _currentWindow isn't, so I think the |else if (this._currentWindow)| case can be removed here (and in close()). Can do that in a new bug, if you want (I know this was here before).

>+      <method name="_onBrowserKeypress">
>+        <parameter name="aEvent"/>
>+        <body><![CDATA[
>+          const TAF_LINKS_KEY = "'";
>+          const TAF_TEXT_KEY = "/";

this.TAF_TEXT_KEY/TAF_LINKS_KEY ?

>+      <method name="_flash">
>+        <body><![CDATA[
>+          if (this._flashFindBarCount-- == 0) {
>+            clearInterval(this._flashFindBarTimeout);
>+            this.removeAttribute("flash");
>+            this._flashFindBarCount = 6;
>+            return;
>+          }

There should probably be a field for the magic "6" value.

>+      <method name="onFindAgainCommand">

>+          var res = this._findAgain(aFindPrevious);
>+          if (res == this.nsITypeAheadFind.FIND_NOTFOUND) {
>+            if (this.open()) {
>+              this._findField.focus();
>+              this._findField.focus();
>+              this._updateStatusUI(res, aFindPrevious);
>+            }
>+          }

You don't call _setFindCloseTimeout here, while the old find bar code did. Is this intentional?

>+    <handlers>
>+      <handler event="keypress" keycode="VK_ESCAPE" phase="capturing" action="this.close();"/>
>+    </handlers>
>+  </binding>

This means that Esc while other findbar chrome (e.g. match case checkbox) is focused closes it, which is a change in behavior. Is this intentional? I guess it makes sense.

>Index: toolkit/themes/pinstripe/global/findBar.css

>+findbar > toolbarbutton.findbar-find-next > .toolbarbutton-text,
>+findbar > toolbarbutton.findbar-find-previous > .toolbarbutton-text,
>+findbar > toolbarbutton.findbar-highlight > .toolbarbutton-text {
>+  -moz-margin-start: 0px
>+}

Missing semi-colon.

>Index: browser/themes/pinstripe/browser/browser.css

Can you get rid of http://lxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/browser.css#1188 too?


r=me with all those addressed.
Attachment #244997 - Flags: review?(gavin.sharp) → review+
(In reply to comment #24)
> >+          baseNode.style.backgroundColor = aHighBackColor;
> >+          baseNode.style.color = aHighTextColor;
> >+          baseNode.style.display = "inline";
> >+          baseNode.style.fontSize = "inherit";
> 
> You lost a padding = 0 here, does it matter?

Ah, it probably regresses some part of bug 256990, so I guess it does.
Comment on attachment 244998 [details]
unit test v2

>    var gStausText;

typo

>    function onPageShow() {
>      testNormalFind();
>      gFindBar.close();
>      testQuickFindText();
>      gFindBar.close();
>      testQuickFindLink();
>      testStatusText();
>      testQuickFindClose();
>    }

Probably would be a good idea to assert that it's hidden after each close() call (or at the beginning of each of the following tests).

>      var matchCaseCheckbox = gFindBar.getElement("find-case-sensitive");
>      if (!matchCaseCheckbox.hidden & matchCaseCheckbox.checked)

&&

>        ASSERT(gBrowser.contentWindow.getSelection() != searchStr,
>               "testNormalFind: Case-sensitivy is broken '" + searchStr + "'");

sensitivity
Attachment #244998 - Flags: review?(gavin.sharp) → review+
> >Index: toolkit/content/widgets/findbar.xml
> 
> >+      <field name="FIND_NORMAL">0</field>
> >+      <field name="FIND_TYPEAHEAD">1</field>
> >+      <field name="FIND_LINKS">2</field>
> >+
> >+      <field name="TAF_LINKS_KEY">"'"</field>
> >+      <field name="TAF_TEXT_KEY">"/"</field>
> 
> Set readonly on these?

fields cannot be set as readonly AFAICT.

> >+      <field name="_findMode">0</field>
> >+      <field name="_tmpOutline">null</field>
> >+      <field name="_tmpOutlineOffset">"0"</field>
> >+      <field name="_drawOutline">false</field>
> >+
> >+      <field name="_flashFindBar">0</field>
> >+      <field name="_flashFindBarCount">6</field>
> 
> There doesn't seem to be a field for _foundLink, _foundEditable,
> _currentWindow, etc, which means they're sometimes undefined. I guess that
> doesn't really matter, but for consistency it'd probably be best to add some?
> 

Most of them were removed for performance reasons.

> >+          var baseNode = doc.createElementNS("http://www.w3.org/1999/xhtml", "span");
> >+          baseNode.style.backgroundColor = aHighBackColor;
> >+          baseNode.style.color = aHighTextColor;
> >+          baseNode.style.display = "inline";
> >+          baseNode.style.fontSize = "inherit";
> 
> You lost a padding = 0 here, does it matter?
> 

added |baseNode.style.padding = "0";|.

> >+      <method name="_setFoundLink">
> 
> >+            aFoundLink.style.outline = "1px dotted invert";
> >+            aFoundLink.style.outlineOffset = "0";
> 
> Make these easy-to-change fields?
> 

Not fixed, pretty much because itg wouldn't allow changing other style rules.

> >+      <method name="_finishFAYT">
> >+        <parameter name="aKeypressEvent"/>
> >+        <body><![CDATA[
> >+          try {
> >+            if (this._foundLink)
> >+              this._foundLink.focus();
> >+            else if (this._foundEditable) {
> >+              this._foundEditable.focus();
> >+              var fastFind = this._browser.fastFind;
> >+              fastFind.collapseSelection();
> >+            }
> >+            else if (this._currentWindow)
> >+              this._currentWindow.focus();
> 
> Although it isn't documented in nsITypeAheadFind.idl, I think its impossible
> for both _foundLink and _foundEditable to be null while _currentWindow isn't,
> so I think the |else if (this._currentWindow)| case can be removed here (and in
> close()). Can do that in a new bug, if you want (I know this was here before).
>

I will file a separate bug.

> >+      <method name="_onBrowserKeypress">
> >+        <parameter name="aEvent"/>
> >+        <body><![CDATA[
> >+          const TAF_LINKS_KEY = "'";
> >+          const TAF_TEXT_KEY = "/";
> 
> this.TAF_TEXT_KEY/TAF_LINKS_KEY ?
> 

As noted above, I was trying to avoid fields as much as possible.

> >+      <method name="_flash">
> >+        <body><![CDATA[
> >+          if (this._flashFindBarCount-- == 0) {
> >+            clearInterval(this._flashFindBarTimeout);
> >+            this.removeAttribute("flash");
> >+            this._flashFindBarCount = 6;
> >+            return;
> >+          }
> 
> There should probably be a field for the magic "6" value.
> 

Not yet fixed.

> >+      <method name="onFindAgainCommand">
> 
> >+          var res = this._findAgain(aFindPrevious);
> >+          if (res == this.nsITypeAheadFind.FIND_NOTFOUND) {
> >+            if (this.open()) {
> >+              this._findField.focus();
> >+              this._findField.focus();
> >+              this._updateStatusUI(res, aFindPrevious);
> >+            }
> >+          }
> 
> You don't call _setFindCloseTimeout here, while the old find bar code did. Is
> this intentional?

That is done in _findAgain.

> >+    <handlers>
> >+      <handler event="keypress" keycode="VK_ESCAPE" phase="capturing" action="this.close();"/>
> >+    </handlers>
> >+  </binding>
> 
> This means that Esc while other findbar chrome (e.g. match case checkbox) is
> focused closes it, which is a change in behavior. Is this intentional? I guess
> it makes sense.

Yes, see bug 355123.

> Can you get rid of
> http://lxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/browser.css#1188
> too?

Get rid or move to findbar.css?
>> >+      <method name="_flash">
>> >+        <body><![CDATA[
>> >+          if (this._flashFindBarCount-- == 0) {
>> >+            clearInterval(this._flashFindBarTimeout);
>> >+            this.removeAttribute("flash");
>> >+            this._flashFindBarCount = 6;
>> >+            return;
>> >+          }
>> 
>> There should probably be a field for the magic "6" value.
>> 

> Not yet fixed.

Fixed, actually.
(In reply to comment #27)
> > >Index: toolkit/content/widgets/findbar.xml

> > >+      <field name="TAF_LINKS_KEY">"'"</field>
> > >+      <field name="TAF_TEXT_KEY">"/"</field>
...
> > >+      <method name="_onBrowserKeypress">
> > >+        <parameter name="aEvent"/>
> > >+        <body><![CDATA[
> > >+          const TAF_LINKS_KEY = "'";
> > >+          const TAF_TEXT_KEY = "/";
> > 
> > this.TAF_TEXT_KEY/TAF_LINKS_KEY ?
> 
> As noted above, I was trying to avoid fields as much as possible.

You should remove the fields, then :)

> > >+      <method name="onFindAgainCommand">
> > 
> > >+          var res = this._findAgain(aFindPrevious);
> > >+          if (res == this.nsITypeAheadFind.FIND_NOTFOUND) {
> > >+            if (this.open()) {
> > >+              this._findField.focus();
> > >+              this._findField.focus();
> > >+              this._updateStatusUI(res, aFindPrevious);
> > >+            }
> > >+          }
> > 
> > You don't call _setFindCloseTimeout here, while the old find bar code did. Is
> > this intentional?
> 
> That is done in _findAgain.

For some reason that doesn't work in my testing. The following STR show a change in behavior from your patch:
1) Trigger search with "'" or "/"
2) Enter text not found on the page, see "not found styling"
3) After the findbar auto-closes, press F3 to find again

With your patch the findbar never closes again. With a current 2.0 branch build, it closes after the normal timeout.

> > Can you get rid of
> > http://lxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/browser.css#1188
> > too?
> 
> Get rid or move to findbar.css?

Either one :)
(In reply to comment #30)
> (In reply to comment #27)
> > > >Index: toolkit/content/widgets/findbar.xml
> 
> > > >+      <field name="TAF_LINKS_KEY">"'"</field>
> > > >+      <field name="TAF_TEXT_KEY">"/"</field>
> ...
> > > >+      <method name="_onBrowserKeypress">
> > > >+        <parameter name="aEvent"/>
> > > >+        <body><![CDATA[
> > > >+          const TAF_LINKS_KEY = "'";
> > > >+          const TAF_TEXT_KEY = "/";
> > > 
> > > this.TAF_TEXT_KEY/TAF_LINKS_KEY ?
> > 
> > As noted above, I was trying to avoid fields as much as possible.
> 
> You should remove the fields, then :)

Oh, ugh, fixed :)

> > > >+      <method name="onFindAgainCommand">
> > > 
> > > >+          var res = this._findAgain(aFindPrevious);
> > > >+          if (res == this.nsITypeAheadFind.FIND_NOTFOUND) {
> > > >+            if (this.open()) {
> > > >+              this._findField.focus();
> > > >+              this._findField.focus();
> > > >+              this._updateStatusUI(res, aFindPrevious);
> > > >+            }
> > > >+          }
> > > 
> > > You don't call _setFindCloseTimeout here, while the old find bar code did. Is
> > > this intentional?
> > 
> > That is done in _findAgain.
> 
> For some reason that doesn't work in my testing. The following STR show a
> change in behavior from your patch:
> 1) Trigger search with "'" or "/"
> 2) Enter text not found on the page, see "not found styling"
> 3) After the findbar auto-closes, press F3 to find again
> 
> With your patch the findbar never closes again. With a current 2.0 branch
> build, it closes after the normal timeout.

OK, fixed.
 
> > > Can you get rid of
> > > http://lxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/browser.css#1188
> > > too?
> > 
> > Get rid or move to findbar.css?
> 
> Either one :) 

OK, apparently this rule does not apply to anything so I just removed it.

Thanks for reviews.
Comment on attachment 246543 [details] [diff] [review]
patch

>Index: toolkit/content/widgets/findbar.xml

>+            var tmpLink = this._findbar._foundLink;
>+            if (tmpLink && this.findbar._finishFAYT(aEvent))
>+              this.findbar._dispatchKeypressEvent(tmpLink, aEvent);

I think it'd be good to add a small comment saying you need to keep a reference to _foundLink because _finishFAYT resets it.

>+      <handler event="keypress" keycode="VK_ESCAPE" phase="capturing" action="this.close();"/>

I just noticed that you're missing a 'preventdefault="true"' here, which makes pressing escape in the view source window close both the findbar and the window.

r=me with those addressed.
Attachment #246543 - Flags: review?(gavin.sharp) → review+
Created attachment 246633 [details] [diff] [review]
for checkin
Attachment #245499 - Attachment is obsolete: true
Attachment #246543 - Attachment is obsolete: true
mozilla/browser/base/content/browser-doctype.inc 1.11
mozilla/browser/base/content/browser-sets.inc 1.85
mozilla/browser/base/content/browser.js 1.741
mozilla/browser/base/content/browser.xul 1.329
mozilla/browser/base/content/global-scripts.inc 1.11
mozilla/browser/themes/pinstripe/browser/bookmark-hover-left.png delete
mozilla/browser/themes/pinstripe/browser/bookmark-hover-mid.png delete
mozilla/browser/themes/pinstripe/browser/bookmark-hover-right.png delete
mozilla/browser/themes/pinstripe/browser/browser.css 1.36
mozilla/browser/themes/pinstripe/browser/jar.mn 1.34
mozilla/browser/themes/winstripe/browser/browser.css 1.51
mozilla/mail/base/content/mailWindowOverlay.js 1.144
mozilla/mail/base/content/mailWindowOverlay.xul 1.182
mozilla/mail/base/content/messageWindow.js 1.47
mozilla/mail/base/content/messageWindow.xul 1.29
mozilla/mail/base/content/messenger.xul 1.65
mozilla/mail/base/content/msgMail3PaneWindow.js 1.108
mozilla/toolkit/components/help/content/help.js 1.44
mozilla/toolkit/components/help/content/help.xul 1.34
mozilla/toolkit/components/typeaheadfind/jar.mn 1.3
mozilla/toolkit/components/typeaheadfind/content/findBar.inc delete
mozilla/toolkit/components/typeaheadfind/content/findBar.js delete
mozilla/toolkit/components/viewsource/content/viewPartialSource.js 1.13
mozilla/toolkit/components/viewsource/content/viewPartialSource.xul 1.26
mozilla/toolkit/components/viewsource/content/viewSource.js 1.18
mozilla/toolkit/components/viewsource/content/viewSource.xul 1.33
mozilla/toolkit/content/jar.mn 1.27
mozilla/toolkit/content/xul.css 1.90
mozilla/toolkit/content/widgets/findbar.xml initial revision: 1.1
mozilla/toolkit/themes/pinstripe/global/findBar.css 1.5
mozilla/toolkit/themes/pinstripe/global/jar.mn 1.24
mozilla/toolkit/themes/pinstripe/global/toolbar/toolbarbutton-customhover-left.png initial revision: 1.1
mozilla/toolkit/themes/pinstripe/global/toolbar/toolbarbutton-customhover-mid.png initial revision: 1.1
mozilla/toolkit/themes/pinstripe/global/toolbar/toolbarbutton-customhover-right.png initial revision: 1.1
mozilla/toolkit/themes/winstripe/global/findBar.css 1.4
Target Milestone: Firefox 3 alpha2 → Firefox 3 alpha1
I will attach the fixed unit test later today.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Why was this checked into the wrong place? You added it into content/widgets and it doesn't belong there.
How so? We don't have any xul widgets defined by xul.css outside of toolkit/content/widgets.
The findbar isn't a XUL widget, it's very browser specific UI.
(In reply to comment #41)
> The findbar isn't a XUL widget, it's very browser specific UI.

It's not "browser" (as in Firefox) specific at all. It's used in the toolkit help viewer, for example, and can be used anywhere else a <browser> is used.
...and is also supposed to be part of xulrunner (note thunderbird already uses it)
So the browser UI folks are OK with no longer being able to specify or alter the appearance or behaviour of the findbar?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
You're probably making a false assumption here; the old findbar code lived in toolkit/components/typeaheadfind (used in both help viewer and view source), not in browser/.

Do note it's now easier to modify the appearance or behavior of the findbar for a particular application, either by extending the binding (for behavior or non-trivial appearance changes) or by overriding findbar.css.
Depends on: 367070

Comment 46

12 years ago
Created attachment 255967 [details] [diff] [review]
add unit test to automated run

Updated

12 years ago
Attachment #255967 - Flags: review?(mano)

Comment 47

12 years ago
Created attachment 255971 [details] [diff] [review]
run finish() in the settimeout callback
Attachment #255967 - Attachment is obsolete: true
Attachment #255971 - Flags: review?(mano)
Attachment #255967 - Flags: review?(mano)

Comment 48

12 years ago
Created attachment 255973 [details] [diff] [review]
indentation nit
Attachment #255971 - Attachment is obsolete: true
Attachment #255973 - Flags: review?(mano)
Attachment #255971 - Flags: review?(mano)
Comment on attachment 255973 [details] [diff] [review]
indentation nit

thank you sir.
Attachment #255973 - Flags: review?(mano) → review+
mozilla/toolkit/content/tests/Makefile.in 1.2
mozilla/toolkit/content/tests/chrome/Makefile.in initial revision: 1.1
mozilla/toolkit/content/tests/chrome/bug288254_window.xul initial revision: 1.1
mozilla/toolkit/content/tests/chrome/test_bug288254.xuL initial revision: 1.1
Flags: in-testsuite? → in-testsuite+
Keywords: dev-doc-needed
Can someone familiar with this bug briefly summarize the documentation issues involved for me?
Product: Firefox → Toolkit
Still unclear what exactly requires dev doc work here.
The API of the findbar widget, see findbar.xml and the usage of <findbar> in our source trees.
For my own reference, there's an initial outline of a doc here, which I need to update and finish: https://developer.mozilla.org/En/Findbar_API
I've written the article here:

https://developer.mozilla.org/En/XUL/Findbar

And the old article redirects to that.  Now documented fully.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.