Closed Bug 399664 Opened 17 years ago Closed 17 years ago

In location bar auto-complete, indicate which part of the result matches the query

Categories

(Firefox :: Address Bar, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 3 beta2

People

(Reporter: faaborg, Assigned: moco)

References

Details

Attachments

(9 files, 47 obsolete files)

29.01 KB, image/png
Details
299.39 KB, image/jpeg
Details
47.66 KB, image/png
Details
103.31 KB, image/png
Details
56.92 KB, image/png
Details
28.18 KB, image/png
Details
66.35 KB, patch
moco
: review+
Details | Diff | Splinter Review
1.54 KB, patch
moco
: review+
Details | Diff | Splinter Review
48.34 KB, image/png
Details
In location bar auto-complete, we need to indicate with part of the result matches the query.  Let's start by using bold, and iterate from there.  Beltzner would like this to block M9.
Flags: blocking-firefox3?
beltzner was just asking about this bug.

While we can apply styles (like bold or underline) to a given cell of text in a tree, I don't think it is currently possible to style a part of text in a cell.

(See http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp)

Even to just bold the matched text, I think we'll need to move to a richlistbox.

Neil, do you agree?
Blocking, need to consider the milestone. I'm pretty sure I'm gonna be a bear about getting this into M9, but mconnor and I need to arm-wrestle a bit. Seth, could be helpful to have you arbitrate.
Flags: blocking-firefox3? → blocking-firefox3+
Summary: In location bar auto-complete, indicate with part of the result matches the query → In location bar auto-complete, indicate which part of the result matches the query
setting milestone to M9 to keep on the radar. need to re-assess pending the determination of the M9 freeze date.
Target Milestone: --- → Firefox 3 M9
(In reply to comment #1)
> Even to just bold the matched text, I think we'll need to move to a
> richlistbox.
> 
> Neil, do you agree?
> 

That would require rewritting the autocomplete widget as well as much of nsAutoCompleteController.cpp. Also, it would be slower to update and scroll through the list.

neil, the ultimate goal for the url bar autocomplete (leaving other autocomplete, like search suggest alone), here is http://people.mozilla.com/~faaborg/files/granParadisoUI/places_UnifyingSearch_i2.png

we'd limit the search results (to 7), and there would be no scroll bar (but we would have an additional item to launch into the places organizer to the full results)
currently investigating the following approach:

1)  for the autocomplete-result-popup, using a richlistbox instead of a tree
2)  this means, in nsAutoCompleteController.cpp, we won't have a tree (which we already handle gracefully).  but we will still call invalidate() on our nsIAutoCompletePopup
3)  on invalidate(), back in autocomplete.xml, I handle building / updating the richlist box, using the nsIAutoCompleteController interface.
Assignee: nobody → sspitzer
Attached file work in progress (obsolete) —
Attached image screen shot (obsolete) —
Attached patch updated patch (obsolete) — Splinter Review
Attachment #286051 - Attachment is obsolete: true
beltzner points out that I broke browser.urlbar.autoFill
> beltzner points out that I broke browser.urlbar.autoFill

correction, I didn't break autoFill, but if you enable autofill you'll see a problem with my patch.

Other feedback from beltzner:

 - typing very quickly can lead to a pretty big wait time, it might be that I'm hesitating just enough between some of the keypresses to make the algorithm start calculating

 - the first time I use it in a session, as I arrow-down through the list, the entire selected entry becomes bold and the boldface is lost on the matching portions throughout the rest of the drop-down

 - sometimes no match appears because the portion of the title being matched is out of range; in those cases, I think we might want to bold the "..." for now, but a better solution would be to always ensure that the matching portion of the title was in range.

new patch coming later today.
Status: NEW → ASSIGNED
Attached patch updated patch (obsolete) — Splinter Review
fixes the problems associated with "bolding" the text in the url bar, and not the search text.
Attachment #286112 - Attachment is obsolete: true
Attached patch updated patch, left ellipsis (obsolete) — Splinter Review
Attachment #286239 - Attachment is obsolete: true
Whiteboard: [has wip patch]
a partial todo list:

1) figure out how to determine the row height (currently hardcoded in
autocomplete.xml)
2) figure out how to determine the amount of padding we need to prevent the
tag/star icon on the right from appearing under the scrollbar.  (currently
hardcoded in autocomplete.xml)

Setting your display font size properties to be extra large will reveal the
problems with hard coding.

need to test:

1) RTL (http://www.ynet.co.il).  need to make sure that when in RTL mode, we do
the right thing with ellipsis and with the emphasized and non-emphasized parts
of the title (or url).
Comment on attachment 286638 [details] [diff] [review]
performance improvements, ui polish, moving css rules out of the autocomplete binding (and into browser.css)

hoping for a review from gavin of this wip patch while I do some final cleanup and address the two todo items from comment #20
Attachment #286638 - Flags: review?(gavin.sharp)
I took a quick glance through this too, although I only looked for stylistic things.

>+      <!-- what is this thing? -->
>       <tabbrowser id="content" disablehistory="true"

It's the tabbrowser. Seriously though, what is this comment for?

>Index: toolkit/content/xul.css
>===================================================================
...
>+panel[type="rich-autocomplete"] {
>+  -moz-binding: url("chrome://global/content/autocomplete.xml#autocomplete-rich-result-popup");
>+}
>+

I'd rather use type="autocomplete-richlistbox" here, and in the second usage. But if the api isn't changing, a class would be suitable too. You use a class below for .autocomplete-richlistbox for instance.

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

>+      <field name="mInput">null</field>
>+      <field name="mPopupOpen">false</field>
>+      <field name="mShowCommentCol">false</field>
>+      <field name="mShowImageCol">false</field>
>+      <field name="mCurrentIndex">0</field>
>+      <field name="mMaxResults">100</field>

Private members should start with an underscore. I know the existing autocomplete has this inconsistency too, but we might as well fix up any new code added.

>+        this.setAttribute("ignorekeys", "true");

We can just use <content ignorekeys="true"> instead.

>+          var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                      .getService(Components.interfaces.nsIPrefBranch);      
>+          this.mMaxResults = prefService.getIntPref("browser.urlbar.maxResults");

This preference is only available for firefox. Since you override this binding in urlbarBindings.xml, you can just move the constructor there.

>+          this.richlistbox.selectedIndex = val;
>+          this.richlistbox.ensureSelectedElementIsVisible();
>+          // Fire select event on xul:richlistbox so that accessibility API
>+          // support layer can fire appropriate accessibility events.
>+          var event = document.createEvent('Events');
>+          event.initEvent("select", true, true);
>+          this.richlistbox.dispatchEvent(event);

Changing richlistbox.selectedIndex should already be firing the select event.

>+
>+      <method name="closePopup">
>+        <body><![CDATA[
>+          if (this.mPopupOpen) {
>+            this.hidePopup();
>+            document.popupNode = null;
>+//            this.setAttribute("hidden", "true");
>+            this.removeAttribute("width");
>+          }

Just remove the commented out line, and from the current autocomplete.

>+
>+          if (height == 0)
>+            this.richlistbox.setAttribute("collapsed", "true");
>+          else {
>+            if (this.richlistbox.hasAttribute("collapsed"))
>+              this.richlistbox.removeAttribute("collapsed");
>+
>+            this.richlistbox.setAttribute("height", height);
>+          }

this.richlistbox.collapsed = (height == 0);
this.richlistbox.height = height;

>+
>+          // make sure that any existing richlistitems 
>+          // that we aren't going to are collapsed

aren't going to what?

>+              item = this.richlistbox.childNodes[this.mCurrentIndex];
>+              item.setAttribute("ac_image", controller.getImageAt(this.mCurrentIndex));
>+              item.setAttribute("ac_url", controller.getValueAt(this.mCurrentIndex));
>+              item.setAttribute("ac_title", controller.getCommentAt(this.mCurrentIndex));
>+              item.setAttribute("ac_type", controller.getStyleAt(this.mCurrentIndex));
>+              item.setAttribute("ac_text", trimmedSearchString);

What's up with these ac_* attributes? Why not just 'image', 'url', etc...

>+              if (item.hasAttribute("collapsed"))
>+                item.removeAttribute("collapsed");

item.collapsed = false;

There's a number of other places where collapsed/class/width/height can just be set with properties.

>+          <!-- =================== PUBLIC MEMBERS =================== -->
>+

Are they public? The existing autocomplete used this line to separate the nsIAutoCompletePopup implementations. The appendCurrentResult method doesn't fit this I think.

Also, there's a number of methods in this binding and in the autocomplete-richlistitem binding which look that they were intended to be private (adjustAcItem, adjustWidth, etc.) They should have names preceded by an underscore. This makes it easier to create api documentation.


>+          <field name="treecols">
>+            document.getAnonymousElementByAttribute(this, "anonid", "treecols");
>+          </field>

Leftover stuff? Some of the other methods have tree related code too (the *Column methods defined later).

>+
>+          <property name="view" 
>+                    onget="return this.mInput.controller;">
>+            <setter><![CDATA[
>+            // XXX not being called for the richlist implementation, make this property readonly?
>+            return val;

No, returning val is correct.

>+          if (height == 0)
>+            this.tree.setAttribute("collapsed", "true");
>+          else {
>+            if (this.tree.hasAttribute("collapsed"))
>+              this.tree.removeAttribute("collapsed");
>+
>+            this.tree.setAttribute("height", height);
>+          }

Similar simplification can be made as earlier.

>+        <handlers>
>+          <handler event="popupshowing"><![CDATA[
>+        // If normalMaxRows wasn't already set by the input, then set it here
>+        // so that we restore the correct number when the popup is hidden.
>+        if (this._normalMaxRows < 0)
>+          this._normalMaxRows = this.mInput.maxRows;
>+
>+        this.mPopupOpen = true;
>+      ]]></handler>

The popup won't necessarily be opened, for instance if another listener called preventDefault. You should just get the popup's open state directly when needed via its 'state' property.

>+      <binding id="autocomplete-richlistitem" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>+        <content>
>+          <xul:image anonid="ac-favicon-image" class="ac-site-icon"/>
>+          <xul:label anonid="pre-url-overflow-ellipsis" class="ac-ellipsis-before"/>

favicon? boo. Just ac-image is ok here. Or, better still, just 'image'.
It could also just inherit the src attribute rather than set it manually later.

>+    </constructor>
>+    <method name="adjustAcItem">

This function seems to rely on classes that are only used in browser.css. Was that intentional?
Comment on attachment 286638 [details] [diff] [review]
performance improvements, ui polish, moving css rules out of the autocomplete binding (and into browser.css)

(Didn't see Neil's comments until I came to post my review, some may be duplicates)

>Index: browser/base/content/browser.xul

>+    <!-- for search -->
>     <panel type="autocomplete" chromedir="&locale.dir;" id="PopupAutoComplete" noautofocus="true"/>
> 
>+    <!-- for url bar autocomplete -->
>+    <panel type="autocomplete" chromedir="&locale.dir;" id="PopupAutoCompleteRichResult" noautofocus="true"/>

Note: using separate <panel>s for each of the different kinds of autocomplete has been attempted before, but had to be backed out because it caused a Ts/Txul hit. See bug 391385. Not sure there's much we can do about that in this case, but we need to be aware of it.

>     <splitter id="sidebar-splitter" class="chromeclass-extrachrome" hidden="true"/>
>     <vbox id="appcontent" flex="1">
>+      <!-- what is this thing? -->
>       <tabbrowser id="content" disablehistory="true"
>                   flex="1" contenttooltip="aHTMLTooltip"
>                   contentcontextmenu="contentAreaContextMenu"
>                   onnewtab="BrowserOpenTab();"
>                   autocompletepopup="PopupAutoComplete"

Assuming your question refers to the autocompletepopup attribute on the tabbrowser, it specifies which popup to use for content autocomplete (e.g. form history, password manager). It's inherited by the initial browser, and set by addTab on any additional child browsers.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/tabbrowser.xml&rev=1.247#130
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/tabbrowser.xml&rev=1.247#1152
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/browser.xml&rev=1.108#374

>Index: browser/base/content/urlbarBindings.xml

>+  <binding id="urlbar-rich-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup">
>+    <implementation>
>+      <method name="onPopupClick">

This only differs from the existing "urlbar-result-popup" because it uses a different way to get the selectedIndex, right? Seems like we should be able to continue to use one "popup" binding for all autocomplete popups, and just use this.selectedIndex, which maps to this.tree.currentIndex or this.richlistbox.selectedIndex as needed. The existing binding uses this.tree.view.selection.currentIndex where it could be using this.selectedIndex, I should have caught that when I did that review.

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

>@@ -913,16 +913,18 @@ statusbarpanel#statusbar-display {

>+/* XXX can we remove some of these style rules? */

Certainly seems like we should be able to remove the browser-specific treecolAutoCompleteImage rules.

> /* Popup Bounding Box */
> #identity-popup-container {
>     background-image: none;
>     background-color: white;
>     min-width: 280px;
>     padding: 10px;
> }
>+
>+/* Popup Bounding Box */
>+#identity-popup-container {
>+    background-image: none;
>+    background-color: white;
>+    min-width: 280px;
>+    padding: 10px;
>+}

Bad merge?

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

>+/* XXX TODO handle RTL / LTR for rich results */

I think what we actually want is to keep the URL itself LTR, but have the surrounding chrome and such RTL/LTR as needed. That might mean the existing rules aren't quite right either. But I might also be wrong, I don't really know what the correct RTL behavior is. The people involved in bug 350611 would be the ones to ask.

>Index: toolkit/components/autocomplete/public/nsIAutoCompleteController.idl

>@@ -36,17 +36,17 @@

>   /*
>-   * Set the current search string, but don't start searching
>+   * Get / set the current search string.  Note, setting will not start searching
>    */
>-  void setSearchString(in AString aSearchString);
>+  attribute AString searchString;
> };

>Index: toolkit/content/xul.css

>@@ -719,46 +719,70 @@ textbox[type="number"] {

>+panel[type="rich-autocomplete"] {
>+  -moz-binding: url("chrome://global/content/autocomplete.xml#autocomplete-rich-result-popup");
>+}
>+

>+.autocomplete-richlistbox {
>+  -moz-binding: url("chrome://global/content/autocomplete.xml#autocomplete-richlistbox");
>+}

>+.autocomplete-richlistitem {
>+  -moz-binding: url("chrome://global/content/autocomplete.xml#autocomplete-richlistitem");
>+}

I'm not sure we should really worry about adding these in the %ifdef AUTOCOMPLETE_OLD_STYLE block... they're not going to work there without more work anyways, right?

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

>+  <binding id="autocomplete-rich-result-popup" extends="chrome://global/content/bindings/popup.xml#popup">

It looks like this binding duplicates a lot of methods from the existing autocomplete-result-popup. Seems like it would make sense to have a base binding that they both inherit from, or just have this autocomplete-rich-result-popup binding extend autocomplete-result-popup and override it's content+certain methods?

>+      <constructor><![CDATA[
>+        this.setAttribute("ignorekeys", "true");
>+  
>+        try {
>+          var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                      .getService(Components.interfaces.nsIPrefBranch);      
>+          this.mMaxResults = prefService.getIntPref("browser.urlbar.maxResults");
>+        }
>+        catch (ex) {
>+        }
>+      ]]></constructor>

Having a toolkit widget depend on a browser preference is wrong. There may be other existing cases of this, but we should try to reduce them, not add to them.

>+      <method name="openAutocompletePopup">

>+            this.popupBoxObject.setConsumeRollupEvent(this.mInput.consumeRollupEvent);

This doesn't seem right, but it's a separate issue unrelated to your patch. I filed bug 401712.

>+      <binding id="autocomplete-richlistitem" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">

I kinda wish there was a cleaner way to do this without so many anonymous children, but I can't think of any off-hand.

>+    <method name="adjustAcItem">
>+      <body><![CDATA[

>+        var ensureBoldIsVisible = true;
>+        var showPreEllipsis = true;

Are these just here for testing?

>+        if (ensureBoldIsVisible) {
>+          var url_sbo = this._urlScrollbox.boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject);
>+          var title_sbo = this._titleScrollbox.boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject);
>+        }

Do you really need those QIs? Shouldn't be necessary.

>+        this._acTypeImage.setAttribute("class", result_type ? ("ac-result-type-" + result_type) : "");

.className (this happens in other places throughout the patch as well).

I also noticed quite a few indentation problems that I'm guessing you'll fix as part of the de-WIPing.

Posting these now so that you have a chance to address them as you continue working on them.
thanks to both neil and gavin for reviews.

here are responses to neil's comments (and then I'll attach a new patch)

1)

>+      <!-- what is this thing? -->
>       <tabbrowser id="content" disablehistory="true"

> Seriously though, what is this comment for?

It was note to myself about the autocompletepopup attribute, which is set to "PopupAutoComplete".

(Thanks to gavin for answering the question in his review comments that I had not yet figured out.)

I've removed the comment.

2)

>Index: toolkit/content/xul.css
>===================================================================
...
>+panel[type="rich-autocomplete"] {
>+  -moz-binding: url("chrome://global/content/autocomplete.xml#autocomplete-rich-result-popup");
>+}
>+

> I'd rather use type="autocomplete-richlistbox" here, and in the second usage.

fixed both usages, thanks.

I also fixed:

- <panel type="autocomplete" chromedir="&locale.dir;" id="PopupAutoCompleteRichResult" noautofocus="true">
+ <panel type="autocomplete-richlistbox" chromedir="&locale.dir;" id="PopupAutoCompleteRichResult" noautofocus="true"/>

And added:

+panel[type="autocomplete-richlistbox"],

to autocomplete.css for both winstripe and pinstripe.

3)

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

>+      <field name="mInput">null</field>
>+      <field name="mPopupOpen">false</field>
>+      <field name="mShowCommentCol">false</field>
>+      <field name="mShowImageCol">false</field>
>+      <field name="mCurrentIndex">0</field>
>+      <field name="mMaxResults">100</field>

> Private members should start with an underscore. I know the existing
> autocomplete has this inconsistency too, but we might as well fix up any new
> code added.

Partially fixed.  mShowCommentCol and mShowImageCol have been removed and I'm now doing _currentIndex and _maxResults.

I've left mInput and mPopupOpen alone, as they are used by existing autocomplete.xml code.

4)

>+        this.setAttribute("ignorekeys", "true");

> We can just use <content ignorekeys="true"> instead.

fixed, thanks.

5)

>+          var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                      .getService(Components.interfaces.nsIPrefBranch);      
>+          this.mMaxResults = prefService.getIntPref("browser.urlbar.maxResults");

> This preference is only available for firefox. Since you override this binding
> in urlbarBindings.xml, you can just move the constructor there.

Moved to the constructor to urlbarBindings.xml, thanks.

6)

>+          this.richlistbox.selectedIndex = val;
>+          this.richlistbox.ensureSelectedElementIsVisible();
>+          // Fire select event on xul:richlistbox so that accessibility API
>+          // support layer can fire appropriate accessibility events.
>+          var event = document.createEvent('Events');
>+          event.initEvent("select", true, true);
>+          this.richlistbox.dispatchEvent(event);

> Changing richlistbox.selectedIndex should already be firing the select event.

Fixed, thanks. 

(the selectedIndex setter calls selectItem() which will call _fireOnSelect() which will fire the select event.)

7)

>+
>+      <method name="closePopup">
>+        <body><![CDATA[
>+          if (this.mPopupOpen) {
>+            this.hidePopup();
>+            document.popupNode = null;
>+//            this.setAttribute("hidden", "true");
>+            this.removeAttribute("width");
>+          }

> Just remove the commented out line, and from the current autocomplete.

removed the commented out line from the new code and the current autocomplete code, thanks.

8)

>+
>+          if (height == 0)
>+            this.richlistbox.setAttribute("collapsed", "true");
>+          else {
>+            if (this.richlistbox.hasAttribute("collapsed"))
>+              this.richlistbox.removeAttribute("collapsed");
>+
>+            this.richlistbox.setAttribute("height", height);
>+          }

> this.richlistbox.collapsed = (height == 0);
> this.richlistbox.height = height;

fixed, thanks.

9)

>+
>+          // make sure that any existing richlistitems 
>+          // that we aren't going to are collapsed

> aren't going to what?

I've changed the comment to:

          // make sure to collapse any existing richlistitems
          // that we aren't going to be used

10)

>+              item = this.richlistbox.childNodes[this.mCurrentIndex];
>+              item.setAttribute("ac_image", controller.getImageAt(this.mCurrentIndex));
>+              item.setAttribute("ac_url", controller.getValueAt(this.mCurrentIndex));
>+              item.setAttribute("ac_title", controller.getCommentAt(this.mCurrentIndex));
>+              item.setAttribute("ac_type", controller.getStyleAt(this.mCurrentIndex));
>+              item.setAttribute("ac_text", trimmedSearchString);

> What's up with these ac_* attributes? Why not just 'image', 'url', etc...

fixed, thanks.  (Note, I've also renamed "ac_width" to be "current_width").

11)


>+              if (item.hasAttribute("collapsed"))
>+                item.removeAttribute("collapsed");

> item.collapsed = false;

fixed, thanks.

> There's a number of other places where collapsed/class/width/height can just be
> set with properties.

fixed, thanks.

12)

>+          <!-- =================== PUBLIC MEMBERS =================== -->
>+

> Are they public? The existing autocomplete used this line to separate the
> nsIAutoCompletePopup implementations. The appendCurrentResult method doesn't
> fit this I think.

appendCurrentResult() is not meant to be public.  (I've renamed that to be _appendCurrentResult())

I've removed the copy-and-pasted comment.

> Also, there's a number of methods in this binding and in the
> autocomplete-richlistitem binding which look that they were intended to be
> private (adjustAcItem, adjustWidth, etc.) They should have names preceded by an
> underscore. This makes it easier to create api documentation.

I've renamed the private methods to be _adjustAcItem() and _adjustWidth()

13)

>+          <field name="treecols">
>+            document.getAnonymousElementByAttribute(this, "anonid", "treecols");
>+          </field>

> Leftover stuff? Some of the other methods have tree related code too (the *Column methods defined later).

Yes, leftover stuff.  Thanks for catching it.  (I swore I removed all that tree cruft at somepoint.)

I've removed all the tree related stuff, treecols, showCommentColumn, showImageColumn, addColumn, removeColumn, adjustHeight

Note, I added the showImageColumn stuff for bug #387718, but it (and the style rules) are now unused.

(See the comment in my patch about "+/* XXX can we remove some of these style rules? */")

I'll either back that out as part of this patch, or do it in a spin off bug.

14)

>+
>+          <property name="view" 
>+                    onget="return this.mInput.controller;">
>+            <setter><![CDATA[
>+            // XXX not being called for the richlist implementation, make this property readonly?
>+            return val;

> No, returning val is correct.

fixed, thanks.

15)

>+          if (height == 0)
>+            this.tree.setAttribute("collapsed", "true");
>+          else {
>+            if (this.tree.hasAttribute("collapsed"))
>+              this.tree.removeAttribute("collapsed");
>+
>+            this.tree.setAttribute("height", height);
>+          }

> Similar simplification can be made as earlier.

This code was "tree cruft" and has been removed.

16)

>+        <handlers>
>+          <handler event="popupshowing"><![CDATA[
>+        // If normalMaxRows wasn't already set by the input, then set it here
>+        // so that we restore the correct number when the popup is hidden.
>+        if (this._normalMaxRows < 0)
>+          this._normalMaxRows = this.mInput.maxRows;
>+
>+        this.mPopupOpen = true;
>+      ]]></handler>

> The popup won't necessarily be opened, for instance if another listener called
> preventDefault. You should just get the popup's open state directly when needed
> via its 'state' property.

Neil, can I make this a spin off item?  The existing autocomplete popup code has the same problem (and mPopupOpen is used in onKeyPress)

17)

>+      <binding id="autocomplete-richlistitem" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>+        <content>
>+          <xul:image anonid="ac-favicon-image" class="ac-site-icon"/>
>+          <xul:label anonid="pre-url-overflow-ellipsis" class="ac-ellipsis-before"/>

> favicon? boo. Just ac-image is ok here. Or, better still, just 'image'.
> It could also just inherit the src attribute rather than set it manually later.

I've gone the route of removing the anonid and just inheriting src attribute, thanks for the tip!

<xul:image xbl:inherits="src=image" class="ac-site-icon"/>

18)

>+    </constructor>
>+    <method name="adjustAcItem">

> This function seems to rely on classes that are only used in browser.css. Was
> that intentional?

Intentional in that this richlistbox autocomplete binding was designed for the url bar.   Do you propose that I move it or keep it in autocomplete.xml and make it more general purpose?
> I've left mInput and mPopupOpen alone, as they are used by existing
> autocomplete.xml code.

OK, I have another bug somewhere that cleans that up.

> Neil, can I make this a spin off item?  The existing autocomplete popup code
> has the same problem (and mPopupOpen is used in onKeyPress)

Sure.

> Intentional in that this richlistbox autocomplete binding was designed for the
> url bar.   Do you propose that I move it or keep it in autocomplete.xml and
> make it more general purpose?

I was proposing that stuff in browser.css be moved to autocomplete.css since most of it isn't browser specific. Except for the .ac-result-type-* items of course. But it doesn't matter as long as it looks ok without those styles. I could imagine  this url autocomplete functionality being useful for other apps/extensions.

new patch coming.  still more work to be done.

here are some responses to gavin's review comments:

1)

>Index: browser/base/content/browser.xul

>+    <!-- for search -->
>     <panel type="autocomplete" chromedir="&locale.dir;" id="PopupAutoComplete" noautofocus="true"/>
> 
>+    <!-- for url bar autocomplete -->
>+    <panel type="autocomplete" chromedir="&locale.dir;" id="PopupAutoCompleteRichResult" noautofocus="true"/>

> Note: using separate <panel>s for each of the different kinds of autocomplete
> has been attempted before, but had to be backed out because it caused a Ts/Txul
> hit. See bug 391385. Not sure there's much we can do about that in this case,
> but we need to be aware of it.

I'm also concerned about the additional panel, but not sure what we can do in this case.
Over irc, we're hoping that bug #394887 helps to minimize the Ts/Txul impact here.

2)

>     <splitter id="sidebar-splitter" class="chromeclass-extrachrome" hidden="true"/>
>     <vbox id="appcontent" flex="1">
>+      <!-- what is this thing? -->
>       <tabbrowser id="content" disablehistory="true"
>                   flex="1" contenttooltip="aHTMLTooltip"
>                   contentcontextmenu="contentAreaContextMenu"
>                   onnewtab="BrowserOpenTab();"
>                   autocompletepopup="PopupAutoComplete"

> Assuming your question refers to the autocompletepopup attribute on the
> tabbrowser, it specifies which popup to use for content autocomplete (e.g. form
> history, password manager). It's inherited by the initial browser, and set by
> addTab on any additional child browsers.

That was my question.  Thanks for the info.

3)

>Index: browser/base/content/urlbarBindings.xml

>+  <binding id="urlbar-rich-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup">
>+    <implementation>
>+      <method name="onPopupClick">

> This only differs from the existing "urlbar-result-popup" because it uses a
> different way to get the selectedIndex, right? Seems like we should be able to
> continue to use one "popup" binding for all autocomplete popups, and just use
> this.selectedIndex, which maps to this.tree.currentIndex or
> this.richlistbox.selectedIndex as needed. The existing binding uses
> this.tree.view.selection.currentIndex where it could be using
> this.selectedIndex, I should have caught that when I did that review.

As discussed over irc and email, I'm not able to do this.

I can't just have one "urlbar-result-popup" binding because for the richlistbox case, I need the binding to extend autocomplete-rich-result-popup binding, 
but for the other instance, I need to extend the autocomplete-result-popup binding.
I thought about removing the urlbar-result-popup binding, and have the common "onPopupClick" method be part of a base binding that 
both autocomplete-rich-result-popup and autocomplete-result-popup could inherit from (see your other comment)
but the urlbar-result-popup binding in urlbarBindings.xml overrides the autocomplete-result-popup binding's onPopupClick() method the way it does 
for a good reason:  it special cases how we handle click the popup for the url bar and the search bar.  
We wouldn't want that special case code in the generic toolkit autocomplete.xml (as url and search bar are browser specific.) 


4)

>+/* XXX can we remove some of these style rules? */

> Certainly seems like we should be able to remove the browser-specific
> treecolAutoCompleteImage rules.

I've removed them and backed out the rest of bug #387718, too.

5)

>+
>+/* Popup Bounding Box */
>+#identity-popup-container {
>+    background-image: none;
>+    background-color: white;
>+    min-width: 280px;
>+    padding: 10px;
>+}

> Bad merge?

Yes.  Not sure how that happened, but I've cleaned it up, thanks for spotting it.

6)

>+/* XXX TODO handle RTL / LTR for rich results */

I think what we actually want is to keep the URL itself LTR, but have the
surrounding chrome and such RTL/LTR as needed. That might mean the existing
rules aren't quite right either. But I might also be wrong, I don't really know
what the correct RTL behavior is. The people involved in bug 350611 would be
the ones to ask.

Still working on getting the RTL right.

7)

>+panel[type="rich-autocomplete"] {
>+  -moz-binding: url("chrome://global/content/autocomplete.xml#autocomplete-rich-result-popup");
>+}
>+

>+.autocomplete-richlistbox {
>+  -moz-binding: url("chrome://global/content/autocomplete.xml#autocomplete-richlistbox");
>+}

>+.autocomplete-richlistitem {
>+  -moz-binding: url("chrome://global/content/autocomplete.xml#autocomplete-richlistitem");
>+}

> I'm not sure we should really worry about adding these in the %ifdef
> AUTOCOMPLETE_OLD_STYLE block... they're not going to work there without more
> work anyways, right?

You are right.  I've removed them, thanks.

8)

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

>+  <binding id="autocomplete-rich-result-popup" extends="chrome://global/content/bindings/popup.xml#popup">

> It looks like this binding duplicates a lot of methods from the existing
> autocomplete-result-popup. Seems like it would make sense to have a base
> binding that they both inherit from, or just have this
> autocomplete-rich-result-popup binding extend autocomplete-result-popup and
> override it's content+certain methods?

working on this now.

9)

> Having a toolkit widget depend on a browser preference is wrong. There may be
> other existing cases of this, but we should try to reduce them, not add to
> them.

Fixed, thanks.

10)

>+      <method name="openAutocompletePopup">
>+            this.popupBoxObject.setConsumeRollupEvent(this.mInput.consumeRollupEvent);

> This doesn't seem right, but it's a separate issue unrelated to your patch. I
> filed bug 401712.

Thanks for spotting that problem and logging that bug.

11)

>+      <binding id="autocomplete-richlistitem" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">

> I kinda wish there was a cleaner way to do this without so many anonymous children, but I can't think of any off-hand.

Yes, there is a lot of anonymous children for each richlistitem.  

But given what we want to show the favicon, the url, the title, and the tag or star icon, 
and for the title and url we want to bold the matching part, make sure it is visible, show ellipsis on both sides, I do know how else to do this.

12)

>+    <method name="adjustAcItem">
>+      <body><![CDATA[

>+        var ensureBoldIsVisible = true;
>+        var showPreEllipsis = true;

> Are these just here for testing?

They were for testing, yes.  I was doing some performance testing to evaluate how costly those feature were.  I've removed them.

13)

>+        if (ensureBoldIsVisible) {
>+          var url_sbo = this._urlScrollbox.boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject);
>+          var title_sbo = this._titleScrollbox.boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject);
>+        }

> Do you really need those QIs? Shouldn't be necessary.

They are necessary, as I need to call the scrollToElement method of the nsIScrollBoxObject interface.

14)

>+        this._acTypeImage.setAttribute("class", result_type ? ("ac-result-type-" + result_type) : "");

> .className (this happens in other places throughout the patch as well).

fixed, thanks.

15)

> I also noticed quite a few indentation problems that I'm guessing you'll fix as
> part of the de-WIPing.

Yes, I'll be fixing all that.  (curse me and my editor)
also contains:

1)  fix for the height hack, a work around until richlistbox supports the "rows" attribute like listbox (logged bug #401939)

2)  backs out the (now unused) changes for bug #387718
Attachment #286797 - Attachment is obsolete: true
neil wrote:

> I was proposing that stuff in browser.css be moved to autocomplete.css since
> most of it isn't browser specific. Except for the .ac-result-type-* items of
> course. But it doesn't matter as long as it looks ok without those styles. I
> could imagine  this url autocomplete functionality being useful for other
> apps/extensions.

I've moved the rules (except for the "ac-result-type-*" ones) from browser.css to autocomplete.css.  

That will be in the next patch I attach.

> Neil, can I make this a spin off item?  The existing autocomplete popup code
> has the same problem (and mPopupOpen is used in onKeyPress)

spun off to bug #401948
Depends on: 401939, 401948
Attached patch updated wip patch (obsolete) — Splinter Review
1) move from browser.css to autocomplete.css

note, had to add !important to the label margin rules due to formatting.css (http://lxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/formatting.css#149)

2)  implement a base binding (autocomplete-base-popup) that both autocomplete-result-popup and autocomplete-rich-result-popup extend (to share common code), per gavin
Attachment #286879 - Attachment is obsolete: true
Attached patch updated version (obsolete) — Splinter Review
Attachment #286924 - Attachment is obsolete: true
also, for the richlist autocomplete, fix a problem with mousing up / down through results.

because richlistems are collapsed, the row count in our richlistbox is not the same as our match count.

we want match count, not the number of rows in the richlistbox, for determing the "next index", see getNextIndex()
Attachment #287057 - Attachment is obsolete: true
Attachment #287181 - Attachment is obsolete: true
Attachment #287201 - Flags: review?(gavin.sharp)
Whiteboard: [has wip patch] → [has patch][need review gavin]
Comment on attachment 287201 [details] [diff] [review]
change default max results to 50, fix hard coding with adjustWidth()

>Index: browser/base/content/browser.xul

>+    <!-- for search -->
>     <panel type="autocomplete" chromedir="&locale.dir;" id="PopupAutoComplete" noautofocus="true"/>

Add "and content formfill/pw manager" to the comment?

>Index: browser/base/content/urlbarBindings.xml

>+            if (this.mInput._getParentSearchbar) {
>               // handle search bar click
>               var search = controller.getValueAt(this.tree.view.selection.currentIndex);

You could fix this to just use "this.selectedIndex", or at least file a followup to clean these two bindings up (assign to me if you want).

>+    <binding id="urlbar-rich-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup">
>+      <implementation>
>+        <constructor><![CDATA[
>+          try {
>+            var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                        .getService(Components.interfaces.nsIPrefBranch);      
>+            this._maxResults = prefService.getIntPref("browser.urlbar.maxResults");
>+          }
>+          catch (ex) {
>+          }
>+        ]]></constructor>

Getting the prefsService at binding attachment time might not be optimal, could get this lazily by making this a property on the autocomplete-rich-result-popup, and overriding it here.

>+      <method name="onPopupClick">

>+          // completely ignore right-clicks
>+          else if (aEvent.button != 2) {
>+            if (gURLBar && this.mInput == gURLBar) {
>+              // handle address bar click
>+              var url = controller.getValueAt(this.tree.view.selection.currentIndex);

Looks like you copy/pasted the tree-based binding and forgot to change this?

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

(these comments apply to both theme files)

>+/* Keep the URL bar LTR */
>+
>+#PopupAutoCompleteRichResult {
>+  direction: ltr !important;
>+}

The patch for bug 350611 added rules to keep "treerows" RTL in RTL locales, do we need something similar here? Since "direction" is inherited this will force everything in the popup to be ltr. Can probably sort this out in a followup if you'd prefer.

>   height: 16px;
> }
> 
> .autocomplete-treebody::-moz-tree-image(bookmark, treecolAutoCompleteValue) {
>   width: 16px;
>   height: 16px;
> }

Should be able to remove these lines too, right?
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/themes/winstripe/browser/browser.css&rev=1.126&mark=940-951#940
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/themes/pinstripe/browser/browser.css&rev=1.89&mark=907-916#907

>+.ac-result-type-tag {

>+.ac-result-type-bookmark {

Put these next to the other autocomplete rules (that you're removing)?

>Index: toolkit/components/autocomplete/public/nsIAutoCompleteInput.idl

>-  /*
>-   * Option to show a third column in the popup which contains
>-   * an additional image for each autocomplete result
>-   */
>-  attribute boolean showImageColumn;

Why this change? Seems like it's functionality that some autocomplete users might still want (support for the tree popup with images). Ideally both types of popups would support toggling the presence of the "image" column, but if that's too hard for the rich-popup we can just have it ignore the attribute on the input (or implement it later).

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

>+  <binding id="autocomplete-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-base-popup">

>     <implementation implements="nsIAutoCompletePopup">

>       <field name="mShowCommentCol">false</field>

>+      <property name="showCommentColumn"
>+                   onget="return this.mShowCommentColumn;">

Turns out this was wrong before, but since you're changing this you might as well change the field to be "mShowCommentColumn" instead of "mShowCommentCol" to match all it's other uses.

>+  <binding id="autocomplete-base-popup" extends="chrome://global/content/bindings/popup.xml#popup">
>+    <implementation implements="nsIAutoCompletePopup">

Since this binding doesn't actually fully implement nsIAutoCompletePopup on it's own, you might as well remove this "implements". The "implements" attribute isn't inherited anyways, so you need to put it on bindings that extend this one either way.

>+      <constructor>
>+        <![CDATA[
>+        this.setAttribute("ignorekeys", "true");
>+      ]]>
>+      </constructor>

I guess Neil's issue #4 in comment 24 got unfixed when you split out the bindings? You can remove this constructor and set ignorekeys="true" on the <content> of the two bindings that extend it.

>+      <!-- =================== nsIAutoCompletePopup =================== -->
>+
>+      <property name="input"
>+                onget="return this.mInput"/>

readonly="true"

>+      <property name="overrideValue" readonly="true"
>+                onget="return null;"/>
>+
>+      <property name="popupOpen" readonly="true"
>+                onget="return this.mPopupOpen;"/>

Should at least file a followup on making this use this.mPopup.popupState as Neil mentions in his comment.

>+  <binding id="autocomplete-rich-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-base-popup">
>+    <resources>
>+      <stylesheet src="chrome://global/skin/richlistbox.css"/>

Is this actually needed? I know that the current autocomplete-popup binding has tree.css, but since these styles only apply to the anonymous <tree> element (or <richlistbox> element in this case) I would have expected XBL to take care of loading this for you.

>+      <method name="openAutocompletePopup">

This seems to share almost all of it's code with the equivalent in autocomplete-result-popup. Perhaps you could factor out the common code into a private method they both call?

>+      <method name="invalidate">
>+        <body>
>+          <![CDATA[
>+          // collapsed if no matches
>+          this.richlistbox.collapsed = (this._matchCount == 0);
>+
>+          // make sure to collapse any existing richlistitems
>+          // that we aren't going to be used

nit: s/we//. I guess we never remove richlistitems from the richlistbox once they've been added? There's a cap of _maxResults elements, but a default of 100 seems like a lot of elements to be keeping around indefinitely. I think we might want to consider lowering that maximum for the base binding (it's already lowered to 50 for the URL bar case), and possibly consider removing these elements at some point. I guess it comes down to a perf/memory use tradeoff, and perf probably is more important in this case, so this is probably the right approach.

>+      <property name="_matchCount" readonly="true">

>+          var matchCount = this.mInput.controller.matchCount;
>+
>+          // XXX note AUTOCOMPLETE_MAX_PER_TYPED is hard coded 
>+          // to 100 in nsNavHistoryAutoComplete.cpp
>+          if (matchCount > this._maxResults)
>+            matchCount = this._maxResults;
>+          return matchCount;

The implementation details of nsNavHistoryAutoComplete don't seem relevant to users of the generic toolkit autocomplete binding, so I would remove the comment. Could also simplify to |return Math.min(this.mInput.controller.matchCount, this._maxResults);|

>+      <method name="_appendCurrentResult">

>+          // detect the desired height of the tree  
>+          var rows = this.maxRows;  
>+          if (!this._matchCount || (rows && this._matchCount < rows))  
>+            rows = this._matchCount;  
>+  
>+          // until we have support for "rows" on richlistbox,
>+          // determine the height dymamically.  (see bug #401939)

nit: "dynamically" :)

>+          if (!this._rowHeight && this.richlistbox.childNodes.length)
>+            this._rowHeight = this.richlistbox.childNodes[0].boxObject.height;
>+
>+          var height = this._rowHeight * rows;
>+          if (this._rowHeight && this.richlistbox.height != height)
>+            this.richlistbox.height = height;

I still don't really understand why this height stuff is needed, but I suppose we can look into this stuff more once it's landed.

>+            if (this._currentIndex < existingItemsCount) {
>+              // re-use the existing item
>+              item = this.richlistbox.childNodes[this._currentIndex];
>+              item.setAttribute("image", controller.getImageAt(this._currentIndex));
>+              item.setAttribute("url", controller.getValueAt(this._currentIndex));
>+              item.setAttribute("title", controller.getCommentAt(this._currentIndex));
>+              item.setAttribute("type", controller.getStyleAt(this._currentIndex));
>+              item.setAttribute("text", trimmedSearchString);

This code is common to both branches, could factor it out if you have two conditionals (one for getting/creating the item and one for the stuff that comes after this).

>+            // yield after creating each item so that the UI is responsive
>+            var self = this;
>+            setTimeout(function() { self._appendCurrentResult(); }, 0);

Have you tried yielding once every two additions, or something along those lines? Might help with the "jitter" I see as the richlistbox is made larger before the items are there to fill in the whitespace. Ideally we wouldn't need this and could just use a loop to build the list. Does it make much of a difference for you? I didn't notice any obvious slowdowns if I removed this setTimeout and just called _appendCurrentResult() directly.

>+      <method name="selectBy">

We don't seem to have any callers of this method, did you manage to test it somehow?

>+      <field name="richlistbox">
>+        document.getAnonymousElementByAttribute(this, "anonid", "richlistbox");
>+      </field>

I've somehow managed to forget what I knew about XBL fields. I was worried that this would be recomputed on each access, but I think that it will only be computed once (on first access due to bz's patch for bug 372769) and then stored. Should probably confirm that.

>+  <binding id="autocomplete-richlistitem" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>+    <content>
>+      <xul:image xbl:inherits="src=image" class="ac-site-icon"/>
>+      <xul:label anonid="pre-url-overflow-ellipsis" class="ac-ellipsis-before"/>
>+      <xul:scrollbox anonid="url-scrollbox" class="ac-url">
>+        <xul:description anonid="url" class="ac-normal-text"/>
>+      </xul:scrollbox>
>+      <xul:label anonid="url-overflow-ellipsis" class="ac-ellipsis-between"/>
>+      <xul:label anonid="pre-title-overflow-ellipsis" class="ac-ellipsis-before"/>
>+      <xul:scrollbox anonid="title-scrollbox" class="ac-title">
>+        <xul:description anonid="title" class="ac-normal-text"/>
>+      </xul:scrollbox>
>+      <xul:label anonid="title-overflow-ellipsis" class="ac-ellipsis-after"/>
>+      <xul:image anonid="type-image" class="ac-type-icon"/>
>+    </content>

This still seems like a lot of anonymous content needed to produce the desired effects, but I don't have any suggestions for a better approach.

>+      <constructor>

>+            if (this._preUrlOverflowEllipsis.boxObject.width)
>+              this._adjustWidth();
>+            else {
>+              var self = this;
>+              setTimeout(function() { self._adjustWidth(); }, 0);
>+            }

What's this about? Seems like one or the other of these conditions will always be true.

>+      <method name="_setUpDescription">

>+          while (aDescriptionElement.hasChildNodes())
>+            aDescriptionElement.removeChild(aDescriptionElement.firstChild);

The current children will either be a single textnode, or a textnode+label+textnode, right? Perhaps we could check whether the current item already has the correct number of children (1 or 3), and just modify the current children (via .textContent) if it does? Followup bug fodder, perhaps.

>+          // ensure the beginning part of the visible
>+          aScrollBoxObject.scrollToElement(aDescriptionElement);

nit: comment needs rewording

>+      <method name="_setUpEllipsis">

>+          // determine if we need to show the "pre" ellipsis          
>+          // since we're at the beginning part, there is no "pre" ellipsis
>+          if (scrollPos.value + aScrollBoxObject.x == aDescriptionElement.boxObject.x)
>+            aPreEllipsis.value = "";
>+          else 
>+            aPreEllipsis.value = "\u2026";
>+
>+          // determine if we need to show the "post" ellipsis
>+          if ((aScrollBoxObject.x + aScrollBoxObject.width) >= (aDescriptionElement.boxObject.x + aDescriptionElement.boxObject.width))
>+            aEllipsis.value = "";
>+          else 
>+            aEllipsis.value = "\u2026";

Could just hardcode the value and collapse the ellipsis when it isn't needed, no?

>+      <method name="_adjustAcItem">
>+        <body>
>+          <![CDATA[
>+        var text = this.getAttribute("text");

Indentation off here.

>+      <method name="_adjustWidth">

This is incredibly hacky, even without considering the hardcoded "16"s :( I'm not sure I even understand why this method is needed, can't the same be achieved with XUL flexing?

>+  <binding id="autocomplete-richlistbox" extends="chrome://global/content/bindings/richlistbox.xml#richlistbox">

>+      <handler event="mouseup">
>+        <![CDATA[
>+        // don't call onPopupClick for the scrollbar buttons, thumb, slider, etc.

Shouldn't this be a click handler?

>+      <handler event="mousemove">

I know this is mostly copied code, but Date.now() is more efficient than (new Date()). Can file a followup to make this change to both, I guess.

>Index: toolkit/themes/pinstripe/global/autocomplete.css
>Index: toolkit/themes/winstripe/global/autocomplete.css

>+richlistbox.autocomplete-richlistbox {

>+richlistbox.autocomplete-richlistbox > richlistitem[selected="true"] {

Remove the "richlistbox.", just use a class selector (see http://developer.mozilla.org/en/docs/Writing_Efficient_CSS#Don.27t_qualify_class-categorized_rules_with_tag_names ).


I think this is fine for now. Once these comments are addressed I think this is OK to land, but I think it would be good to solicit the opinion of some XUL gurus (perhaps in the XUL newsgroup) to get some post-landing feedback one whether there's a better approach that avoids some of the height hacks or reduces the amount of anonymous content needed to get the desired effect. I'm a bit worried that there might be issues with complex scripts or RTL, but we can get feedback on that from beta users and fix any issues for b2.
Attachment #287201 - Flags: review?(gavin.sharp) → review+
>+      <method name="_adjustWidth">

> This is incredibly hacky, even without considering the hardcoded "16"s :(

no argument here.

> I'm not sure I even understand why this method is needed, can't the same be
> achieved with XUL flexing?

here's the problem:

every richlistitem in the richlistbox has the two scrollboxes which contain the
(cropped and scrolled) description elements for url and title.

for each richlistitem, the url and title scrollboxes have to be the same width
(so that everything is aligned, like it is for tree columns.)

so I am setting the min-height / max-height on the scrollboxes, determining
them dynamically as a percentage of the total width (minus the width of the
fixed elements and leaving 20 px for the scrollbar)

the hardcoded 16s are because I still haven't found a reliable way to get the
width of the image elements.  (all the ways I tried could still sometimes
result in 0.)
thanks for the review, gavin.

Still working on your comments, but here's what I've done so far:

1)

> Add "and content formfill/pw manager" to the comment?

added, thanks.

2)

>Index: browser/base/content/urlbarBindings.xml

>+            if (this.mInput._getParentSearchbar) {
>               // handle search bar click
>               var search = controller.getValueAt(this.tree.view.selection.currentIndex);

You could fix this to just use "this.selectedIndex", or at least file a
followup to clean these two bindings up (assign to me if you want).

fixed that to be this.selectedIndex, thanks.

3)

>+    <binding id="urlbar-rich-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup">
>+      <implementation>
>+        <constructor><![CDATA[
>+          try {
>+            var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                        .getService(Components.interfaces.nsIPrefBranch);      
>+            this._maxResults = prefService.getIntPref("browser.urlbar.maxResults");
>+          }
>+          catch (ex) {
>+          }
>+        ]]></constructor>

> Getting the prefsService at binding attachment time might not be optimal, could
> get this lazily by making this a property on the
> autocomplete-rich-result-popup, and overriding it here.

fixed, thanks.

4)

>+      <method name="onPopupClick">

>+          // completely ignore right-clicks
>+          else if (aEvent.button != 2) {
>+            if (gURLBar && this.mInput == gURLBar) {
>+              // handle address bar click
>+              var url = controller.getValueAt(this.tree.view.selection.currentIndex);

> Looks like you copy/pasted the tree-based binding and forgot to change this?

Thanks for catching that copy/paste error.  fixed (and tested, by making sure that middle click works and doesn't give me the js error:

JavaScript strict warning: chrome://browser/content/urlbarBindings.xml, line 247: reference to undefined property this.tree
JavaScript error: chrome://browser/content/urlbarBindings.xml, line 247: this.tree has no properties

5)

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

(these comments apply to both theme files)

>+/* Keep the URL bar LTR */
>+
>+#PopupAutoCompleteRichResult {
>+  direction: ltr !important;
>+}

> The patch for bug 350611 added rules to keep "treerows" RTL in RTL locales, do
> we need something similar here? Since "direction" is inherited this will force
> everything in the popup to be ltr. Can probably sort this out in a followup if
> you'd prefer.

Still investigating, may turn this into a spin off bug about RTL.

6)

> Should be able to remove these lines too, right?
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/themes/winstripe/browser/browser.css&rev=1.126&mark=940-951#940
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/themes/pinstripe/browser/browser.css&rev=1.89&mark=907-916#907

>No, we still need those for the non-rich history autocomplete usages, such as:

a) the autocomplete when in preferences, to choose your homepage
b) the autocomplete when in the "open web location" dialog (when the navigation toolbar is hidden)

7)

>+.ac-result-type-tag {

>+.ac-result-type-bookmark {

> Put these next to the other autocomplete rules (that you're removing)?

moved them, thanks.

8)

>Index: toolkit/components/autocomplete/public/nsIAutoCompleteInput.idl

>-  /*
>-   * Option to show a third column in the popup which contains
>-   * an additional image for each autocomplete result
>-   */
>-  attribute boolean showImageColumn;

Why this change? Seems like it's functionality that some autocomplete users
might still want (support for the tree popup with images). Ideally both types
of popups would support toggling the presence of the "image" column, but if
that's too hard for the rich-popup we can just have it ignore the attribute on
the input (or implement it later).

I (relatively recently) added the image column to support the additional "star / tag" image column as the third (and far right) column for url bar autocomplete.

Autocomplete users can still use the primary image (the image on the far left of the primary column.)

Do you still think we should keep support for that, or should I continue to back it out?

9)


>+      <property name="showCommentColumn"
>+                   onget="return this.mShowCommentColumn;">

> Turns out this was wrong before, but since you're changing this you might as
> well change the field to be "mShowCommentColumn" instead of "mShowCommentCol"
> to match all it's other uses.

fixed, thanks.

10)


>+  <binding id="autocomplete-base-popup" extends="chrome://global/content/bindings/popup.xml#popup">
>+    <implementation implements="nsIAutoCompletePopup">

> Since this binding doesn't actually fully implement nsIAutoCompletePopup on
> it's own, you might as well remove this "implements". The "implements"
> attribute isn't inherited anyways, so you need to put it on bindings that
>extend this one either way.

fixed, thanks.

11)

>+      <constructor>
>+        <![CDATA[
>+        this.setAttribute("ignorekeys", "true");
>+      ]]>
>+      </constructor>

> I guess Neil's issue #4 in comment 24 got unfixed when you split out the
> bindings? You can remove this constructor and set ignorekeys="true" on the
> <content> of the two bindings that extend it.

fixed, thanks.

12)

>+      <!-- =================== nsIAutoCompletePopup =================== -->
>+
>+      <property name="input"
>+                onget="return this.mInput"/>

> readonly="true"

fixed, thanks.

13)

>+      <property name="overrideValue" readonly="true"
>+                onget="return null;"/>
>+
>+      <property name="popupOpen" readonly="true"
>+                onget="return this.mPopupOpen;"/>

> Should at least file a followup on making this use this.mPopup.popupState as
> Neil mentions in his comment.


logged https://bugzilla.mozilla.org/show_bug.cgi?id=401948

14)

>+  <binding id="autocomplete-rich-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-base-popup">
>+    <resources>
>+      <stylesheet src="chrome://global/skin/richlistbox.css"/>

> Is this actually needed? I know that the current autocomplete-popup binding has
> tree.css, but since these styles only apply to the anonymous <tree> element (or
> <richlistbox> element in this case) I would have expected XBL to take care of
> loading this for you.

It was not needed, thanks for catching that.  Removed, thanks.

15)

>+      <method name="openAutocompletePopup">

This seems to share almost all of it's code with the equivalent in
autocomplete-result-popup. Perhaps you could factor out the common code into a
private method they both call?

The two are very similar, except that the rich version calls invalidate() after determining the width of the richlistbox, because it uses it for the min/max-widths of the richlistitems.  

(I've added a comment.)

If it is ok with you, I'll log a spin off bug about figuring out how to refactor openAutocompletePopup() into a private method.

If we can figure out the width hack, it will be easier to refactor.

16)

>+      <method name="invalidate">
>+        <body>
>+          <![CDATA[
>+          // collapsed if no matches
>+          this.richlistbox.collapsed = (this._matchCount == 0);
>+
>+          // make sure to collapse any existing richlistitems
>+          // that we aren't going to be used

> nit: s/we//. 

nit fixed, thanks.

17)

> I guess we never remove richlistitems from the richlistbox once
> they've been added? There's a cap of _maxResults elements, but a default of 100
> seems like a lot of elements to be keeping around indefinitely. I think we
> might want to consider lowering that maximum for the base binding (it's already
> lowered to 50 for the URL bar case), and possibly consider removing these
> elements at some point. I guess it comes down to a perf/memory use tradeoff,
> and perf probably is more important in this case, so this is probably the right
> approach.

You are correct, this was done for perf and this means we will keep around these richlistitems.

(When we've got a tree, with a tree view, there is no reason to do this.)

I've made the default maxResults be 20 (for the base binding), and the url bar binding overrides the readonly property to use the pref value.

17)

>+          var matchCount = this.mInput.controller.matchCount;
>+
>+          // XXX note AUTOCOMPLETE_MAX_PER_TYPED is hard coded 
>+          // to 100 in nsNavHistoryAutoComplete.cpp
>+          if (matchCount > this._maxResults)
>+            matchCount = this._maxResults;
>+          return matchCount;

> The implementation details of nsNavHistoryAutoComplete don't seem relevant to
> users of the generic toolkit autocomplete binding, so I would remove the
> comment. Could also simplify to |return
> Math.min(this.mInput.controller.matchCount, this._maxResults);|

comment removed and code simplified, thanks.


18)

>+          // determine the height dymamically.  (see bug #401939)

> nit: "dynamically" :)

fixed, thanks.

19)

>+          if (!this._rowHeight && this.richlistbox.childNodes.length)
>+            this._rowHeight = this.richlistbox.childNodes[0].boxObject.height;
>+
>+          var height = this._rowHeight * rows;
>+          if (this._rowHeight && this.richlistbox.height != height)
>+            this.richlistbox.height = height;

> I still don't really understand why this height stuff is needed, but I suppose
> we can look into this stuff more once it's landed.

what I want to be able to do is limit the number of rows like I can with a listbox.
since we can't do that yet (spin off bug logged #401939), what I do is set the height of the richlistbox to a multiple of the height of the richlistitems.
(It needs to be a multiple, so that rows are cut off.)  The tricky part was determining the height of these dynamically created richlistitems.

(We do something similar with the tree based autocomplete binding, but there, we have a way to determine the row height.)

I'll log another spin off bug (that depends on #401939) and seek some help from xul gurus about a better way to do this.

20)

>+              item = this.richlistbox.childNodes[this._currentIndex];
>+              item.setAttribute("image", controller.getImageAt(this._currentIndex));
>+              item.setAttribute("url", controller.getValueAt(this._currentIndex));
>+              item.setAttribute("title", controller.getCommentAt(this._currentIndex));
>+              item.setAttribute("type", controller.getStyleAt(this._currentIndex));
>+              item.setAttribute("text", trimmedSearchString);

> This code is common to both branches, could factor it out if you have two
> conditionals (one for getting/creating the item and one for the stuff that
> comes after this).

fixed, thanks.

21)

>+            // yield after creating each item so that the UI is responsive
>+            var self = this;
>+            setTimeout(function() { self._appendCurrentResult(); }, 0);

> Have you tried yielding once every two additions, or something along those
> lines? Might help with the "jitter" I see as the richlistbox is made larger
> before the items are there to fill in the whitespace. Ideally we wouldn't need
> this and could just use a loop to build the list. 
> Does it make much of a difference for you? I didn't notice any obvious slowdowns if I removed this
> setTimeout and just called _appendCurrentResult() directly.

I see the whitespace-then-fill-in.

Calling _appendCurrentResult() directly make a very big difference as far as responsiveness when typing in the url bar (with my windows build.)

Try removing that timeout and see what happens when you type h-t-t-p.

I did try 10,5,3, but responsiveness while typing in the url is a top concern, so I've kept it at one per timeout.  Let me know what you think.


22)

>+      <method name="selectBy">

> We don't seem to have any callers of this method, did you manage to test it
> somehow?

The caller is in nsAutoCompleteController::HandleKeyNavigation()

To test it, I tested key navigation.

23)

>+      <field name="richlistbox">
>+        document.getAnonymousElementByAttribute(this, "anonid", "richlistbox");
>+      </field>

> I've somehow managed to forget what I knew about XBL fields. I was worried that
> this would be recomputed on each access, but I think that it will only be
> computed once (on first access due to bz's patch for bug 372769) and then
> stored. Should probably confirm that.

Confirmed that we only compute that once.

23)

>+  <binding id="autocomplete-richlistitem" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>+    <content>
>+      <xul:image xbl:inherits="src=image" class="ac-site-icon"/>
>+      <xul:label anonid="pre-url-overflow-ellipsis" class="ac-ellipsis-before"/>
>+      <xul:scrollbox anonid="url-scrollbox" class="ac-url">
>+        <xul:description anonid="url" class="ac-normal-text"/>
>+      </xul:scrollbox>
>+      <xul:label anonid="url-overflow-ellipsis" class="ac-ellipsis-between"/>
>+      <xul:label anonid="pre-title-overflow-ellipsis" class="ac-ellipsis-before"/>
>+      <xul:scrollbox anonid="title-scrollbox" class="ac-title">
>+        <xul:description anonid="title" class="ac-normal-text"/>
>+      </xul:scrollbox>
>+      <xul:label anonid="title-overflow-ellipsis" class="ac-ellipsis-after"/>
>+      <xul:image anonid="type-image" class="ac-type-icon"/>
>+    </content>

> This still seems like a lot of anonymous content needed to produce the desired
> effects, but I don't have any suggestions for a better approach.

Agreed, I still don't know a better way either.
24)

>+      <constructor>

>+            if (this._preUrlOverflowEllipsis.boxObject.width)
>+              this._adjustWidth();
>+            else {
>+              var self = this;
>+              setTimeout(function() { self._adjustWidth(); }, 0);
>+            }

> What's this about? Seems like one or the other of these conditions will always
> be true.


I've tweaked this hack and added a comment, hopefully this answers your question:

           if (!this._preUrlOverflowEllipsis.boxObject.width ||
               !this._urlOverflowEllipsis.boxObject.width ||
               !this._preTitleOverflowEllipsis.boxObject.width ||
               !this._titleOverflowEllipsis.boxObject.width) {
             // for the first richlistitem, when we call _adjustWidth()
             // from the xbl constructor, these elements don't have widths
             // but we rely on those widths to properly set the widths
             // of the scrollboxes.  if we don't have the widths
             // try again on a timeout.
             var self = this;
             setTimeout(function() { self._adjustWidth(); }, 0);
           }
           else
             this._adjustWidth();

This hack (again, trying to make richlistbox behave like rich tree) is to make it so for the first richlistem, everything lines up.

25)

>+      <method name="_setUpDescription">

>+          while (aDescriptionElement.hasChildNodes())
>+            aDescriptionElement.removeChild(aDescriptionElement.firstChild);

The current children will either be a single textnode, or a
textnode+label+textnode, right? Perhaps we could check whether the current item
already has the correct number of children (1 or 3), and just modify the
current children (via .textContent) if it does? Followup bug fodder, perhaps.

fixed.  as discussed with gavin over irc, we can have 

      <xul:scrollbox anonid="url-scrollbox" class="ac-url">
        <!-- note, we rely on the newlines here so that we have
             a textNode before and after the label.  see _setUpDescription()
             for more details -->
        <xul:description anonid="url" class="ac-normal-text">
          <xul:label class="ac-emphasize-text"/>
        </xul:description>
      </xul:scrollbox>

the newlines are converted to text nodes, so now _setUpDescription() can just set the values of the 
text nodes using textContent and the value of the label using .value, without needing to create / remove children every time.

Because I am depending on the newlines, I've added that comment to the xbl, per gavin.


26)

>+          // ensure the beginning part of the visible
>+          aScrollBoxObject.scrollToElement(aDescriptionElement);

> nit: comment needs rewording

fixed, thanks.

27)

>+          // determine if we need to show the "post" ellipsis
>+          if ((aScrollBoxObject.x + aScrollBoxObject.width) >= (aDescriptionElement.boxObject.x + aDescriptionElement.boxObject.width))
>+            aEllipsis.value = "";
>+          else 
>+            aEllipsis.value = "\u2026";

> Could just hardcode the value and collapse the ellipsis when it isn't needed,
> no?

I can't, because I need those elements (before and after the description) to always be there to make everything line up (when you have the ellipsis or not)

28)

>+      <method name="_adjustAcItem">

> Indentation off here.

fixed, thanks.

29)

>+      <method name="_adjustWidth">

> This is incredibly hacky, even without considering the hardcoded "16"s :( I'm
> not sure I even understand why this method is needed, can't the same be
> achieved with XUL flexing?

I see comment #39 about why xul flexing didn't work for me and about the hardcoding.  

I'll make sure to log a spin off bug about this hack, and seek help from xul gurus about how to make this work.

I'm really abusing richlistbox here, making it act like a "richtree".  I'll log a spin off bug about how that could be useful for toolkit users.

30)

>+  <binding id="autocomplete-richlistbox" extends="chrome://global/content/bindings/richlistbox.xml#richlistbox">

>+      <handler event="mouseup">
>+        <![CDATA[
>+        // don't call onPopupClick for the scrollbar buttons, thumb, slider, etc.

> Shouldn't this be a click handler?

I went for parity with the tree based autocomplete here.

But looking back in cvsblame, I think the mouseup/move/down was a circa 2001 workaround from by hewitt:

https://bugzilla.mozilla.org/show_bug.cgi?id=43189#c47

"This is because the event.clientX/Y properties are miscalculated whenever the
event target is inside of a popup. I had to file and fix bug 76297 to workaround
this problem for calculating the selected row on mousemove."

If it is ok with you, I'll log a spin off bug about removing this workaround from both the tree and richlistbox versions.

31)

>+      <handler event="mousemove">

> I know this is mostly copied code, but Date.now() is more efficient than (new
> Date()). Can file a followup to make this change to both, I guess.

Thanks for the tip about Date.now() vs "new Date()".

I've moved to Date.now() for the new and old code, thanks.

32)


>+richlistbox.autocomplete-richlistbox {
>+richlistbox.autocomplete-richlistbox > richlistitem[selected="true"] {

> Remove the "richlistbox.", just use a class selector

fixed, thanks.

New patch coming with all these changes.
Attached patch patch updated per gavin's review (obsolete) — Splinter Review
Attachment #287201 - Attachment is obsolete: true
> I see comment #39 about why xul flexing didn't work for me and about the hardcoding.

s/I see/Please see
Comment on attachment 287493 [details] [diff] [review]
patch updated per gavin's review

carrying over gavin's review.  (and, from irc, "looks good")
Attachment #287493 - Flags: review+
fixed.

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.218; previous revision: 1.217
done
Checking in browser/base/content/browser.css;
/cvsroot/mozilla/browser/base/content/browser.css,v  <--  browser.css
new revision: 1.38; previous revision: 1.37
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.386; previous revision: 1.385
done
Checking in browser/base/content/urlbarBindings.xml;
/cvsroot/mozilla/browser/base/content/urlbarBindings.xml,v  <--  urlbarBindings.
xml
new revision: 1.39; previous revision: 1.38
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.cs
s
new revision: 1.90; previous revision: 1.89
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.cs
s
new revision: 1.127; previous revision: 1.126
done
Checking in toolkit/components/autocomplete/public/nsIAutoCompleteController.idl
;
/cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteControlle
r.idl,v  <--  nsIAutoCompleteController.idl
new revision: 1.17; previous revision: 1.16
done
Checking in toolkit/components/autocomplete/public/nsIAutoCompleteInput.idl;
/cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteInput.idl
,v  <--  nsIAutoCompleteInput.idl
new revision: 1.8; previous revision: 1.7
done
Checking in toolkit/components/autocomplete/src/nsAutoCompleteController.cpp;
/cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cp
p,v  <--  nsAutoCompleteController.cpp
new revision: 1.66; previous revision: 1.65
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHis
tory.cpp
new revision: 1.185; previous revision: 1.184
done
Checking in toolkit/components/satchel/src/nsFormFillController.cpp;
/cvsroot/mozilla/toolkit/components/satchel/src/nsFormFillController.cpp,v  <--
 nsFormFillController.cpp
new revision: 1.95; previous revision: 1.94
done
Checking in toolkit/content/xul.css;
/cvsroot/mozilla/toolkit/content/xul.css,v  <--  xul.css
new revision: 1.106; previous revision: 1.105
done
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.x
ml
new revision: 1.86; previous revision: 1.85
done
Checking in toolkit/themes/pinstripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/autocomplete.css,v  <--  autoco
mplete.css
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/themes/winstripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/autocomplete.css,v  <--  autoco
mplete.css
new revision: 1.18; previous revision: 1.17
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Summary: In location bar auto-complete, indicate which part of the result matches the query → In location bar auto-complete, indicate which part of the result matches the query (and make sure it is visible)
Whiteboard: [has patch][need review gavin]
Depends on: 402667
No longer blocks: 402668
Depends on: 402668
re-opening.

I've backed out my fix due to a 2% Ts regression, see bug #402668 for details
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
No longer blocks: 402118, 402673
Depends on: 402118, 402673
Blocks: 402667, 402668, 402673
No longer depends on: 402667, 402668, 402673
No longer blocks: 402667, 402668, 402673
Depends on: 402667, 402668, 402673
--> M10

Seth, thanks for all the hard work on trying to get this for beta. The feeling of drivers was that, at this point with a three-week slip, we're unwilling to hold beta back for just this fix.

Let's keep hammering away for Beta 2.
Target Milestone: Firefox 3 M9 → Firefox 3 M10
No longer blocks: 402674
Depends on: 402674
Comment on attachment 287657 [details] [diff] [review]
include fix for bug #402673 (don't stretch the images)

>+            var self = this;
>+            setTimeout(function() { self._appendCurrentResult(); }, 0);

In order to avoid the closure, I think this should be:

setTimeout(function (self) { self._appendCurrentResult(); }, 0, this);
Also I think it would make sense to use overflow:-moz-hidden-unscrollable instead of overflow:hidden.
thanks to dao for suggesting this approach instead of using additional hboxes.
Attachment #287721 - Attachment is obsolete: true
> In order to avoid the closure, I think this should be:
> setTimeout(function (self) { self._appendCurrentResult(); }, 0, this);

I'll fix that, thanks dao.

> Also I think it would make sense to use overflow:-moz-hidden-unscrollable
> instead of overflow:hidden.

I can try that, thanks Dao.

About -moz-hidden-unscrollable, all I have founnd was this was a discussion between Martjin and Boris:

http://osdir.com/ml/mozilla.devel.layout/2005-02/msg00000.html

Was there something else (in addition to avoid creating the scrollframe) that you were thinking of?

(perhaps we can get something up on http://developer.mozilla.org/en/docs/Mozilla_CSS_Extensions#-moz-hidden-unscrollable about this topic)
Status: REOPENED → ASSIGNED
>> Also I think it would make sense to use overflow:-moz-hidden-unscrollable
>> instead of overflow:hidden.
>
> I can try that, thanks Dao.

I tried this:

.autocomplete-richlistitem {
  overflow:-moz-hidden-unscrollable; 
  padding: 1px 0px 1px 2px;
}

But unlike overflow:hidden, it results in a horizontal scrollbar.
(In reply to comment #54)
> Was there something else (in addition to avoid creating the scrollframe) that
> you were thinking of?

No, that's exactly what I was thinking of.

(In reply to comment #56)
> Created an attachment (id=287790) [details]
> screen shot of the horizonal scrollbar problem if I don't use overflow:hidden

You could probably use overflow-x:hidden on the element with the horizontal scrollbar.
Priority: -- → P1
Attachment #288808 - Attachment is obsolete: true
Attached patch updated to trunk (obsolete) — Splinter Review
Attachment #288962 - Attachment is obsolete: true
Attached patch updated patch (obsolete) — Splinter Review
Attachment #288965 - Attachment is obsolete: true
Comment on attachment 288973 [details] [diff] [review]
updated patch

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

>+.ac-url-text,
> .autocomplete-treebody::-moz-tree-cell-text(treecolAutoCompleteComment) {
>   color: #555566;
> }

this should be GrayText

>Index: toolkit/themes/winstripe/global/autocomplete.css

>+.autocomplete-richlistitem {
>+  overflow: hidden; 
>+  padding: 1px 0px 1px 2px;
>+  border-bottom: solid thin GrayText;
>+}

this should be ThreeDShadow

And I think it would have been better to tackle this and bug 403159 separately.
Note that gnomestripe needs the update, too.

(In reply to comment #62)
> this should be ThreeDShadow

Or ThreeDLightShadow, but a native border color in any case.
>+  border-bottom: solid thin GrayText;

switch to ThreeDShadow, thanks Dao.

>>   color: #555566;
> this should be GrayText

I tried that, and it makes the text very hard to read, which is probably why the existing autocomplete code uses 555566 instead of greytext.

I'll attach a screen shot.
> You could probably use overflow-x:hidden on the element with the horizontal
> scrollbar.

That worked, thanks Dao!
A while back, gavin wrote:

">-  attribute boolean showImageColumn;

Why this change? Seems like it's functionality that some autocomplete users
might still want (support for the tree popup with images). Ideally both types
of popups would support toggling the presence of the "image" column, but if
that's too hard for the rich-popup we can just have it ignore the attribute on
the input (or implement it later)."

If I keep support for showImageColumn (and leave the current css rules for tree based autocomplete for the url bar), then you should be able to switch between the "rich" autocomplete and the current autocomplete by just reverting this change:

         <hbox id="urlbar-button-box" flex="1">
           <textbox id="urlbar" flex="1"
                    chromedir="&locale.dir;"
                    type="autocomplete"
-                   autocompletesearch="history"
-                   autocompletepopup="PopupAutoComplete"
+                   autocompletesearch="history"
+                   autocompletepopup="PopupAutoCompleteRichResult"
                    completeselectedindex="true"
                    tabscrolling="true"
-                   showcommentcolumn="true"
-                   showimagecolumn="true"
                    enablehistory="true"

I'm going to confirm that, and if so, come up with a new patch that.
Attached image mixing hardcoded and native colors (obsolete) —
The problem with #555566 is that it doesn't change with the OS theme. In the attached screenshot, using a high-contrast theme, GrayText is mapped to lime. Now I can still read #555566, but quite obviously the contrast isn't high, which kinda defeats the purpose of that theme. Also, even though I don't know such a theme, I don't think the background is guaranteed to be white or black, which means that the contrast could become much worse.

If you find GrayText hard to read, then... well, that's not good. But strictly speaking, your operating system is in charge of that.
great point about high contrast mode.  I'll keep it as greytext.
with this patch you can toggle between the current result style and the new, rich, two line style by changing:

                   autocompletepopup="PopupAutoCompleteRichResult"
to
                   autocompletepopup="PopupAutoComplete"

Note, I've left these attributes:

                   showcommentcolumn="true"
                   showimagecolumn="true"

But they are ignore if you are in rich result mode.
Attached image screen shot (obsolete) —
Attached patch updated patch (obsolete) — Splinter Review
Attachment #286034 - Attachment is obsolete: true
Attachment #287790 - Attachment is obsolete: true
Attachment #288973 - Attachment is obsolete: true
Attachment #289043 - Attachment is obsolete: true
Attachment #289062 - Attachment is obsolete: true
Attachment #289064 - Attachment is obsolete: true
Attachment #289068 - Attachment is obsolete: true
Attached patch updated to trunk (obsolete) — Splinter Review
note, even disabled, there is an additional panel (hidden by default).

So this might have a slight impact on Ts and Txul, see bug #402668 for some details.

From that bug, my tests showed this hidden panel will cost us about 2ms to Ts:

If we apply gavin's trick for the trunk:
1 panel, hidden: 1435 (1437, 1436, 1432)

If I apply gavin's trick to my patch:
2 panels, both hidden: 1437 (1433, 1438, 1439)
Attachment #289084 - Attachment is obsolete: true
Attachment #289086 - Flags: review?(gavin.sharp)
as discussed over email / irc, in the latest patch, I punted on "(and make sure it is visible)", so removing from the summary.

the left ellipsis is gone, but I still need the scrollbox for cropping (see bug #404427)
Summary: In location bar auto-complete, indicate which part of the result matches the query (and make sure it is visible) → In location bar auto-complete, indicate which part of the result matches the query
Attachment #289086 - Attachment is obsolete: true
Attachment #289452 - Flags: review?(gavin.sharp)
Attachment #289086 - Flags: review?(gavin.sharp)
Comment on attachment 289452 [details] [diff] [review]
patch (updated to trunk and with new colors per Beltzner)

>+.ac-comment {
>+  font-size: larger;
>+  color: #333366;
>+}
>+
>+.ac-url-text {
>+  color: #336633;
>+}

Does this work with a dark theme, e.g. a high-contrast one?

>+.autocomplete-richlistitem {
>+  overflow:-moz-hidden-unscrollable;
>+  overflow-x: hidden !important;
>+  padding: 1px 0px 1px 2px;
>+  border-bottom: 1px solid ThreeDShadow;
>+}

overflow-x makes -moz-hidden-unscrollable probably void... What about this:

.autocomplete-richlistbox > scrollbox {
  overflow-x: hidden;
  overflow-y: auto;
}
(In reply to comment #78)
> Does this work with a dark theme, e.g. a high-contrast one?

Since most high contrast themes have either a black or white background, this should be OK (e.g. it works fine with the high-contrast themes that come with Vista). It obviously wouldn't work great if people have blue or green default backgrounds, but I think that's pretty uncommon, and there isn't really anything we can do about that anyways, assuming we want to keep the colors.
(In reply to comment #79)
> Since most high contrast themes have either a black or white background, this
> should be OK (e.g. it works fine with the high-contrast themes that come with
> Vista).

I haven't tried it, but #333366 is darker than #555566 (attachment 289062 [details]). I don't think that's OK.
Note that there's a pref for the ellipsis, intl.ellipsis, introduced in bug 403484.
(In reply to comment #80)
> (In reply to comment #79)
> > Since most high contrast themes have either a black or white background, this
> > should be OK (e.g. it works fine with the high-contrast themes that come with
> > Vista).
> 
> I haven't tried it, but #333366 is darker than #555566 (attachment 289062 [details]). I
> don't think that's OK.

data:text/html,<body%20style="background:#000;color:#336">#333366
Ah, yeah, I hadn't tested the latest colors. I'm not sure we should be feeling too constrained by high-contrast themes here. We should try to make it be as readable as we can with dark backgrounds, but not necessarily at the expense of the large majority of users that use standard light backgrounds.
todo list:

1)  decide on the colors (what color green, what color blue)
2)  fix gnomestripe theme, per dao 
3)  fix proto theme
4)  use intl.ellipsis for localized ellipsis, per dao
5)  address this, per dao

overflow-x makes -moz-hidden-unscrollable probably void... What about this:

.autocomplete-richlistbox > scrollbox {
  overflow-x: hidden;
  overflow-y: auto;
}
new patch coming with:

> 1)  decide on the colors (what color green, what color blue)

fixed, title is black, url is green (hex values per beltzner).

> 2)  fix gnomestripe theme, per dao 

fixed browser.css but untested (any linux users able to test a try build for me?)

> 3)  fix proto theme

not fixed yet, need to throw over the wall to the proto guys

> 4)  use intl.ellipsis for localized ellipsis, per dao

fixed

> 5)  address this, per dao

overflow-x makes -moz-hidden-unscrollable probably void... What about this:

.autocomplete-richlistbox > scrollbox {
  overflow-x: hidden;
  overflow-y: auto;
}

went with:

.autocomplete-richlistbox > scrollbox {
  overflow-x: hidden !important;
}

.autocomplete-richlistitem {
  overflow:-moz-hidden-unscrollable;
}

other changes:

a) I was seeing a problem where the ellipsis would not always appear.  I had addressed this in an earlier patch, so I've restored my solution

b) I've underlined emphasized text, per beltzner/mconnor/others.  (screen shot coming)
Attached patch updated patch (obsolete) — Splinter Review
also, browser.urlbar.richResults is now on by default
Attachment #289080 - Attachment is obsolete: true
Attachment #289452 - Attachment is obsolete: true
Attachment #289452 - Flags: review?(gavin.sharp)
Attached patch updated to trunk (obsolete) — Splinter Review
Attachment #290314 - Attachment is obsolete: true
(In reply to comment #85)

> b) I've underlined emphasized text, per beltzner/mconnor/others.  (screen shot
> coming) 

Underline and bold looks like overkill IMO. Not pretty at all either. Bold would be enough to catch attention.
(In reply to comment #87)
> Created an attachment (id=290316) [details]
> screen shot

The green is terrible. It really hurts my eyes to read. :(
(In reply to comment #87)
> Created an attachment (id=290316) [details]
> screen shot
> 

This looks good. 

A few comments:
Colors: They are fine (what happened to the blue?) I think its more about getting used to them. 

Separators: They are a bit too over-emphasized. Can they be lightened a bit?

Selection Highlight: Is the selection highlight going to be the standard 'dark blue' even on Vista or more like Vista's 'light blue' type as shown in this mockup under 'Proposed Solution', which really looks neat ... http://people.mozilla.com/~faaborg/files/granParadisoUI/places_UnifyingSearch_i2.png

"Search Google for" ... option. I see this in the proposed solution above. Is there a separate bug for that?

Once this lands along with Bug #401869 and Bug #405320, we are really going to have an awesome bar :-)
Attached image screen shot on Vista
(In reply to comment #93)
> from some try builds, see
> https://build.mozilla.org/tryserver-builds/2007-11-26_18:59-sspitzer@mozilla.com-1196132319/
> 

A screen shot from the trial build in Vista. Can't do without it now ;-)
Attachment #290322 - Flags: review?(gavin.sharp)
Comment on attachment 290322 [details] [diff] [review]
updated to trunk

>+              <xul:label class="ac-emphasize-text"/>
Nit: inside a xul:description, an html:span would be more suitable.
my thoughts: 
- use dotted lines for separators
- title font feels like too big
- underline and bold is maybe less usable than having only one of them (i'd use only bold, and add a little margin-left/right around the bolded word so that it's more prominent, or use background yellow hilight as in searchbar).
- Microsoft windows themes are blue based, so maybe light blu urls would be better than dark green (don't know aboud colour blindness though)
I would like to second what Marco said. I think these are great points, except for the last one. Green is a very smart choice, because that's what search engines use (at least Google and Yahoo!). Blue on the other hand represents clickable links, which not what we have here. I wonder if that indeed was the rationale?
note to gavin / code reviews / try server build testers:

while testing, I'm seeing that sometimes the selected item persists between searches.  I think this might be due to my fix for bug #403832, as we are no longer calling "popup->SetSelectedIndex(-1);" when the popup is closed.

If that is the cause, then this patch would stay the same, but bug #403832 might need to be backed out.

I'll investigate.
(In reply to comment #99)
> 
> while testing, I'm seeing that sometimes the selected item persists between
> searches.
> 

Yes, I did experience this a few times during the day.

Hmm, perhaps we should go with your original patch in bug 403832.
Attachment #290322 - Attachment is obsolete: true
Attachment #290496 - Flags: review?(gavin.sharp)
Attachment #290322 - Flags: review?(gavin.sharp)
this isn't a problem for the tree implementation, as when we process results [nsAutoCompleteController::ProcessResult()], we will call nsTreeBodyFrame::RowCountChanged() if we have a tree.

Calling RowCountChange() will adjust the selection and reset the current index if we've deleted it.

See http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/tree/src/nsTreeSelection.cpp#710

what I'm not 100% sure about is why I didn't start seeing the issue until recently.
gavin / mconnor:  

perhaps browser.urlbar.maxResults should be browser.urlbar.maxRichResults?

additionally, do we want a default of 25 (per mconnor over email or face-to-face) instead of 50?
Attachment #290496 - Attachment is obsolete: true
Attachment #290500 - Flags: review?(gavin.sharp)
Attachment #290496 - Flags: review?(gavin.sharp)
"No visual feedback when clicking on scrollbarbutton in autocomplete popups (no depressed state)"
Attachment #290500 - Attachment is obsolete: true
Attachment #290586 - Flags: review?(gavin.sharp)
Attachment #290500 - Flags: review?(gavin.sharp)
Attached patch updated patch (obsolete) — Splinter Review
1)  address neil's nit:

Nit: inside a xul:description, an html:span would be more suitable.

2)  turn my two per-richlistitem html style comments into comments that will get pre-processed out
Attachment #290586 - Attachment is obsolete: true
Attachment #290608 - Flags: review?(gavin.sharp)
Attachment #290586 - Flags: review?(gavin.sharp)
been running with html:span, and it was noticeably slower than the previous implementation with xul:label, so I'm undoing that change.

neil, can you elaborate why html:span was more suitable?
Attachment #290608 - Attachment is obsolete: true
Attachment #290627 - Flags: review?(gavin.sharp)
Attachment #290608 - Flags: review?(gavin.sharp)
(In reply to comment #110)

> neil, can you elaborate why html:span was more suitable?
> 

I think he means because span is an inline element, which fits better with using the description (which here is a block element), rather than mixing blocks and xul boxes.
Comment on attachment 290627 [details] [diff] [review]
patch, use xul:label instead of html:span

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

>+      <constructor>

>+            // XXX hack

>+            if (!this._urlOverflowEllipsis.boxObject.width ||
>+                !this._titleOverflowEllipsis.boxObject.width)
>+              setTimeout(function(self) { self._adjustWidth(); }, 0, this);
>+            else
>+              this._adjustWidth();

Are you sure both of those branches are hit? Seems like we're likely to just be always using the setTimeout.
Attachment #290627 - Flags: review?(gavin.sharp) → review+
1) fix conflict in browser.js (from dao's landing of bug #405482)

2) add comment explaining why I call _appendCurrentResult() on a timeout

<gavin|> fwiw I tried removing the "proces next item" setTimeout and wasn't able to notice a significant slowdown with a relatively large history
<gavin|> but maybe my macbook is too fast, or something
<sspitzerMsgMe> our current limit is also 25
<sspitzerMsgMe> the smaller that gets, the better off we are.

3)

gavin asked:  "Are you sure both of those branches are hit? Seems like we're likely to just be always using the setTimeout."

In my testing, they are both hit, but I think it is cleaner to just use the setTimeout().

remember, this is in the richlistitem constructor, and we keep them around.

4)  add a comment as to why I search title, then url

// assuming that people learn to use the urlbar for titles (and not urls)
// we should search in titles first, to potentially save the second call to
// CaseInsensitiveFindInReadable() for the url
fixed.

note, there are several spin off issues already logged.

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.232; previous revision: 1.231
done
Checking in browser/base/content/browser.css;
/cvsroot/mozilla/browser/base/content/browser.css,v  <--  browser.css
new revision: 1.42; previous revision: 1.41
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.905; previous revision: 1.904
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.395; previous revision: 1.394
done
Checking in browser/base/content/urlbarBindings.xml;
/cvsroot/mozilla/browser/base/content/urlbarBindings.xml,v  <--  urlbarBindings.
xml
new revision: 1.45; previous revision: 1.44
done
Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.
css
new revision: 1.139; previous revision: 1.138
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.cs
s
new revision: 1.97; previous revision: 1.96
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.cs
s
new revision: 1.137; previous revision: 1.136
done
Checking in toolkit/components/autocomplete/public/nsIAutoCompleteController.idl
;
/cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteControlle
r.idl,v  <--  nsIAutoCompleteController.idl
new revision: 1.19; previous revision: 1.18
done
Checking in toolkit/components/autocomplete/src/nsAutoCompleteController.cpp;
/cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cp
p,v  <--  nsAutoCompleteController.cpp
new revision: 1.70; previous revision: 1.69
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <
--  nsNavHistoryAutoComplete.cpp
new revision: 1.27; previous revision: 1.26
done
Checking in toolkit/content/xul.css;
/cvsroot/mozilla/toolkit/content/xul.css,v  <--  xul.css
new revision: 1.112; previous revision: 1.111
done
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.x
ml
new revision: 1.92; previous revision: 1.91
done
Checking in toolkit/themes/pinstripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/autocomplete.css,v  <--  autoco
mplete.css
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/themes/winstripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/autocomplete.css,v  <--  autoco
mplete.css
new revision: 1.22; previous revision: 1.21
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
reverting a recent change, as on the mac, for new windows, the first time you search, the stars "jump" (because they are on a timeout.)
Attachment #290831 - Flags: review+
> gavin asked:  "Are you sure both of those branches are hit? Seems like we're
> likely to just be always using the setTimeout."

yes.

> In my testing, they are both hit, but I think it is cleaner to just use the
> setTimeout().

...and on mac, it is noticable (the "stars" jump), so I've reverted.  (r=dietrich)
So, this hurt Ts... :(
However, Tp looks a bit better after this patch (weird).
> So, this hurt Ts... :(

There is a new, hidden by default panel in browser.xul which uses the same trick that I used for bug #402668 (to minimize the Ts / Txul impact.)

Which machine and how much?

> However, Tp looks a bit better after this patch (weird).

Yes, that is weird.  Again, which machine and how much?
(In reply to comment #119)
> > So, this hurt Ts... :(
> 
> There is a new, hidden by default panel in browser.xul which uses the same
> trick that I used for bug #402668 (to minimize the Ts / Txul impact.)
> 
> Which machine and how much?

http://build-graphs.mozilla.org/graph/query.cgi?tbox=bl-bldlnx03_fx-linux-tbox-head&testname=startup&autoscale=1&size=&units=ms&ltype=&points=&showpoint=2007%3A11%3A30%3A00%3A33%3A27%2C894&avg=1&days=1

First big jump is the cairo upgrade in bug 404092, the drop is roc's patch for bug 320378, and the jump after that is this patch.

> > However, Tp looks a bit better after this patch (weird).
> 
> Yes, that is weird.  Again, which machine and how much?

Linux perf, but it may have just been noise now that I look at it more.
Seth: I am seeing these empty rows when I search for the first time after starting Firefox. This wasn't the case with the Test build. So guess its from the last few changes that you made. Can you please confirm this behavior?

Also, do you want to handle this here or should I file a separate bug?
phil, I see it, and I see my mistake.  In https://bugzilla.mozilla.org/attachment.cgi?id=290831, I accidentally  removed a call to this._adjustAcItem();  

doh!
taking the 5ms Ts regression off to bug #406141, thanks for spotting it (and analyzing the graph), reed.


Depends on: 406141
No longer blocks: 406121
Depends on: 406194
No longer blocks: 406177
Depends on: 406177
No longer blocks: 406179
Depends on: 406179
Depends on: 406236
Depends on: 406237
Depends on: 406238
Depends on: 406239
Depends on: 406280
No longer depends on: 406236
No longer depends on: 406239
Blocks: 406355
Depends on: 406373
Depends on: 406487
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007120304 Minefield/3.0b2pre
Status: RESOLVED → VERIFIED
Component: Places → Location Bar and Autocomplete
QA Contact: places → location.bar
When both the URL and the title match the query why only the part in the title is highlighted ?
(In reply to comment #127)
> When both the URL and the title match the query why only the part in the title
> is highlighted ?

Generically, only the first match is highlighted.
> When both the URL and the title match the query why only the part in the title
> is highlighted ?

Ideally we would be showing all of the matches.  Seth can follow up for the specific reason, but I think it may have been a perf issue.
Depends on: 407861
> Seth can follow up for the specific reason.

Emphasizing the first match in title and the first match in url is bug #406255

Emphasizing all the matches is currently non-trivial, but see bug #407944 and
bug #407946.
Depends on: 409636
Removing doc needed keyword in favor of bug 409946: Document nsIAutoCompleteController.
Keywords: dev-doc-needed
Depends on: 409974
Depends on: 410091
Depends on: 416114
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: