Closed
Bug 288254
(xblfindbar)
Opened 20 years ago
Closed 18 years ago
Findbar XBL Widget
Categories
(Toolkit :: Find Toolbar, defect, P2)
Toolkit
Find Toolbar
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: asaf, Assigned: asaf)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 17 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
asaf
:
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).
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → Future
Assignee | ||
Comment 1•20 years ago
|
||
Another resonable option is to use the browser binding, something like:
<browser ...findabr="true"/>
Assignee | ||
Comment 2•20 years ago
|
||
hmm, browser.xml already include findbar.dtd (that was *before* findbar.dtd was
cvs added).
Updated•19 years ago
|
QA Contact: fast.find
Assignee | ||
Updated•18 years ago
|
Priority: P3 → P2
Target Milestone: Future → Firefox 3 alpha2
Assignee | ||
Comment 3•18 years ago
|
||
Assignee | ||
Comment 4•18 years ago
|
||
Attachment #241519 -
Attachment is obsolete: true
Assignee | ||
Comment 5•18 years ago
|
||
Attachment #242490 -
Attachment is obsolete: true
Assignee | ||
Comment 6•18 years ago
|
||
Including winstripe, view-source and help viewer changes (and few fixes in the binding itself).
Assignee | ||
Updated•18 years ago
|
Attachment #242593 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Alias: xblfindbar
Summary: Findbar should be a toolkit xbl widget rather than a bogus .inc → Findbar XBL Widget
Assignee | ||
Comment 7•18 years ago
|
||
Attachment #242718 -
Attachment is obsolete: true
Assignee | ||
Comment 8•18 years ago
|
||
TODO: Thunderbird changes, copy pinstripe "hover" images to toolkit/themes.
Attachment #242782 -
Attachment is obsolete: true
Attachment #243270 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•18 years ago
|
Attachment #243270 -
Flags: review?(masayuki)
Assignee | ||
Comment 9•18 years ago
|
||
Also, need to set the preferences in all.js.
Assignee | ||
Comment 10•18 years ago
|
||
Attachment #243270 -
Attachment is obsolete: true
Attachment #243330 -
Flags: review?(gavin.sharp)
Attachment #243270 -
Flags: review?(masayuki)
Attachment #243270 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•18 years ago
|
Attachment #243330 -
Flags: review?(masayuki)
Comment 11•18 years ago
|
||
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.
Assignee | ||
Comment 12•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
(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...)
Assignee | ||
Comment 15•18 years ago
|
||
Attachment #243330 -
Attachment is obsolete: true
Attachment #244933 -
Flags: review?(gavin.sharp)
Attachment #243330 -
Flags: review?(masayuki)
Attachment #243330 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 16•18 years ago
|
||
Assignee | ||
Comment 17•18 years ago
|
||
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)
Assignee | ||
Comment 18•18 years ago
|
||
Attachment #244998 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 19•18 years ago
|
||
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)
Assignee | ||
Comment 20•18 years ago
|
||
*oops*
Assignee | ||
Updated•18 years ago
|
Attachment #244984 -
Attachment is obsolete: true
Comment 21•18 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+
Assignee | ||
Updated•18 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 22•18 years ago
|
||
Attachment #245499 -
Flags: review?(mscott)
Assignee | ||
Updated•18 years ago
|
Attachment #245499 -
Flags: review?(bienvenu)
Comment 23•18 years ago
|
||
Comment on attachment 245499 [details] [diff] [review]
mail/ changes
looks ok to me...
Attachment #245499 -
Flags: review?(bienvenu) → review+
Assignee | ||
Updated•18 years ago
|
Attachment #245499 -
Flags: review?(mscott)
Comment 24•18 years ago
|
||
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+
Comment 25•18 years ago
|
||
(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•18 years ago
|
||
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+
Assignee | ||
Comment 27•18 years ago
|
||
> >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?
Assignee | ||
Comment 28•18 years ago
|
||
>> >+ <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.
Assignee | ||
Comment 29•18 years ago
|
||
Attachment #244997 -
Attachment is obsolete: true
Comment 30•18 years ago
|
||
(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 :)
Assignee | ||
Comment 31•18 years ago
|
||
(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.
Assignee | ||
Comment 32•18 years ago
|
||
Attachment #246506 -
Attachment is obsolete: true
Assignee | ||
Comment 33•18 years ago
|
||
Attachment #246519 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Attachment #246543 -
Flags: review?(gavin.sharp)
Comment 34•18 years ago
|
||
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+
Assignee | ||
Updated•18 years ago
|
Assignee | ||
Comment 35•18 years ago
|
||
Attachment #245499 -
Attachment is obsolete: true
Attachment #246543 -
Attachment is obsolete: true
Assignee | ||
Comment 36•18 years ago
|
||
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
Assignee | ||
Comment 37•18 years ago
|
||
I will attach the fixed unit test later today.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 38•18 years ago
|
||
Attachment #244998 -
Attachment is obsolete: true
Comment 39•18 years ago
|
||
Why was this checked into the wrong place? You added it into content/widgets and it doesn't belong there.
Assignee | ||
Comment 40•18 years ago
|
||
How so? We don't have any xul widgets defined by xul.css outside of toolkit/content/widgets.
Comment 41•18 years ago
|
||
The findbar isn't a XUL widget, it's very browser specific UI.
Comment 42•18 years ago
|
||
(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.
Assignee | ||
Comment 43•18 years ago
|
||
...and is also supposed to be part of xulrunner (note thunderbird already uses it)
Comment 44•18 years ago
|
||
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 → ---
Updated•18 years ago
|
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 45•18 years ago
|
||
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•18 years ago
|
||
Updated•18 years ago
|
Attachment #255967 -
Flags: review?(mano)
Comment 47•18 years ago
|
||
Attachment #255967 -
Attachment is obsolete: true
Attachment #255971 -
Flags: review?(mano)
Attachment #255967 -
Flags: review?(mano)
Comment 48•18 years ago
|
||
Attachment #255971 -
Attachment is obsolete: true
Attachment #255973 -
Flags: review?(mano)
Attachment #255971 -
Flags: review?(mano)
Assignee | ||
Comment 49•18 years ago
|
||
Comment on attachment 255973 [details] [diff] [review]
indentation nit
thank you sir.
Attachment #255973 -
Flags: review?(mano) → review+
Assignee | ||
Comment 50•18 years ago
|
||
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+
Assignee | ||
Updated•17 years ago
|
Keywords: dev-doc-needed
Comment 51•17 years ago
|
||
Can someone familiar with this bug briefly summarize the documentation issues involved for me?
Updated•16 years ago
|
Product: Firefox → Toolkit
Comment 52•16 years ago
|
||
Still unclear what exactly requires dev doc work here.
Assignee | ||
Comment 53•16 years ago
|
||
The API of the findbar widget, see findbar.xml and the usage of <findbar> in our source trees.
Comment 54•16 years ago
|
||
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•16 years ago
|
||
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.
Description
•