288254 - (xblfindbar) Findbar XBL Widget

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P2)

Description

[Asaf Romano (gone)]

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).

Comment 1

[Asaf Romano (gone)]

Another resonable option is to use the browser binding, something like:

<browser ...findabr="true"/>

Comment 2

[Asaf Romano (gone)]

hmm, browser.xml already include findbar.dtd (that was *before* findbar.dtd was
cvs added).

Comment 3

[Asaf Romano (gone)]

Attachment: checkpoint

Comment 4

[Asaf Romano (gone)]

Attachment: more...

Comment 5

[Asaf Romano (gone)]

Attachment: more...

Comment 6

[Asaf Romano (gone)]

Attachment: yet another checkpoint

Including winstripe, view-source and help viewer changes (and few fixes in the binding itself).

Comment 7

[Asaf Romano (gone)]

Attachment: more...

Comment 8

[Asaf Romano (gone)]

Attachment: ready for first review

TODO: Thunderbird changes, copy pinstripe "hover" images to toolkit/themes.

Comment 9

[Asaf Romano (gone)]

Also, need to set the preferences in all.js.

Comment 10

[Asaf Romano (gone)]

Attachment: Some minor fixes

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 12

[Asaf Romano (gone)]

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.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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...)

Comment 15

[Asaf Romano (gone)]

Attachment: patch

Comment 16

[Asaf Romano (gone)]

Attachment: very first draft of a unit test

Comment 17

[Asaf Romano (gone)]

Attachment: patch

De-delay close(), I don't think this is still necessary.

Comment 18

[Asaf Romano (gone)]

Attachment: unit test v2

Comment 19

[Asaf Romano (gone)]

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>

Comment 20

[Asaf Romano (gone)]

*oops*

Comment 21

[Robert Sayre]

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.

Comment 22

[Asaf Romano (gone)]

Attachment: mail/ changes

Comment 23

[David :Bienvenu]

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

looks ok to me...

Comment 24

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

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.

Comment 25

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

(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 26

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

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

Comment 27

[Asaf Romano (gone)]

> >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?

Comment 28

[Asaf Romano (gone)]

>> >+      <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.

Comment 29

[Asaf Romano (gone)]

Attachment: patch

Comment 30

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

(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 :)

Comment 31

[Asaf Romano (gone)]

(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 32

[Asaf Romano (gone)]

Attachment: patch

Comment 33

[Asaf Romano (gone)]

Attachment: patch

Comment 34

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

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.

Comment 35

[Asaf Romano (gone)]

Attachment: for checkin

Comment 36

[Asaf Romano (gone)]

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

Comment 37

[Asaf Romano (gone)]

I will attach the fixed unit test later today.

Comment 38

[Asaf Romano (gone)]

Attachment: unit test v2.1

Comment 39

[Neil Deakin]

Why was this checked into the wrong place? You added it into content/widgets and it doesn't belong there.

Comment 40

[Asaf Romano (gone)]

How so? We don't have any xul widgets defined by xul.css outside of toolkit/content/widgets.

Comment 41

[Neil Deakin]

The findbar isn't a XUL widget, it's very browser specific UI.

Comment 42

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

(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.

Comment 43

[Asaf Romano (gone)]

...and is also supposed to be part of xulrunner (note thunderbird already uses it)

Comment 44

[Neil Deakin]

So the browser UI folks are OK with no longer being able to specify or alter the appearance or behaviour of the findbar?

Comment 45

[Asaf Romano (gone)]

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.

Comment 46

[Robert Sayre]

Attachment: add unit test to automated run

Comment 47

[Robert Sayre]

Attachment: run finish() in the settimeout callback

Comment 48

[Robert Sayre]

Attachment: indentation nit

Comment 49

[Asaf Romano (gone)]

Comment on attachment 255973 [details] [diff] [review]
indentation nit

thank you sir.

Comment 50

[Asaf Romano (gone)]

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

Comment 51

[Eric Shepherd [:sheppy]]

Can someone familiar with this bug briefly summarize the documentation issues involved for me?

Comment 52

[Eric Shepherd [:sheppy]]

Still unclear what exactly requires dev doc work here.

Comment 53

[Asaf Romano (gone)]

The API of the findbar widget, see findbar.xml and the usage of <findbar> in our source trees.

Comment 54

[Eric Shepherd [:sheppy]]

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

Comment 55

[Eric Shepherd [:sheppy]]

I've written the article here:

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

And the old article redirects to that.  Now documented fully.