585991 - Show a popup listing possible completions

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Jeff Balogh (:jbalogh)]

Here's what vim does: http://grab.by/5PHN.  It's easier to key up through the list to get what you want than to tab through a bunch of unknown completions. When the completion is inline my brain has to parse the whole string anew each time, and it isn't very good at that.

Comment 1

[Rob Campbell [:rc] (:robcee)]

I can't believe you took a screenshot of VIM for a UI suggestion! ;)

Comment 2

[Jeff Balogh (:jbalogh)]

I was going to use eclipse but it's still loading.

Comment 3

[Mihai Sucan [:msucan]]

Attachment: proposed patch (wip1)

This is the proposed patch, without any new tests.

All tests pass, no failures. There are already autocomplete tests, so we don't need many new tests actually. Only improvements needed.

Please test on Mac and Windows and let me know if the UI works fine. On my system it works as good as it does in Firebug 1.7 (perhaps even better).

The UI code uses the richlistbox because I'd like us in the future to move the AutocompletePopup into a separate JSM, and reuse it in Workspaces, and have fancier lists, with descriptions for each autocomplete item and more fancyness. Currently it only handles a "label" for the item - enough to get us started.

Looking forward to your feedback! If everything is fine, I'll add some more API coverage to existing completion tests. Thanks!

Comment 4

[Rob Campbell [:rc] (:robcee)]

Patch looked good on initial sweep through but the behavior could use some tweaking.

Currently when I complete something, say "window." the completion list pops up and I see all of the properties I can complete that belong to window. Great! But, my instinct is to use the cursor keys to select the property that I want and those are still tied to the command history. I need to tab first to get into the list and I think that's probably not the right thing, though typing should still update the autocomplete list.

Comment 5

[Mihai Sucan [:msucan]]

Attachment: up/down patch (wip)

Attaching a patch with Up/Down for completion popup navigation.

Problem: you can now use up/down to navigate in the history when there's nothing to autocomplete, but when there is.... you can't navigate in history - you only navigate in the popup list. Weird.

This is a usability issue. How can we make sure that up and down always work as expected? Actually, I'd like to know a clearer usage model - UI interaction.

I liked the idea of having only Tab and Shift-Tab for completion navigation and Up/Down for history navigation. There's a clear distinction. If we can get Up/Down to work nicely for both cases, I am all for it.

Perhaps we should first require the user to Tab once and then allow Up/Down for completion nav?

Looking forward to your feedback. Thanks!

(please also note that mochitests fail now. i'll fix them once we decide on the UI interaction model)

Comment 6

[Rob Campbell [:rc] (:robcee)]

msucan: I played with this patch applied and the interaction is very nice. (review comments forthcoming)

I think it makes sense to have return or tab perform the autocompletion and the up/down arrows for selection. That's what seemed natural to me with this kind of popup. Other comments welcome, but I believe this is consistent with most IDE's that do code-completion.

Comment 7

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 524199 [details] [diff] [review]
up/down patch (wip)

-      for each (var prop in properties) {
-        prop = prop.trim();
...
+      for (let i = 0; i < properties.length; i++) {
+        let prop = properties[i].trim();

Why this change?

...

nit:
           case Ci.nsIDOMKeyEvent.DOM_VK_UP:
-            // history previous
-            if (self.canCaretGoPrevious()) {
+            var completionResult = self.complete(self.COMPLETE_BACKWARD);
+            if (completionResult) {
+              aEvent.preventDefault();
+            }

and later in:
            case Ci.nsIDOMKeyEvent.DOM_VK_DOWN:
+            var completionResult = self.complete(self.COMPLETE_FORWARD);

you're using var here, which suggests you might want to use this somewhere else in this function (which you do later on). If so, please declare it once near the top and reuse the variable.

nit:
+    if (!this.lastCompletion || this.lastCompletion.value != inputValue) {
+      let matches = this.propertyProvider(this.sandbox.window, inputValue);
+      if (!matches || !matches.matches.length) {

this looks a little funny (matches.matches). Is the return of the propertyProvider really a "matches" or something that contains the matches? Might be able to pick a better name.

I think the AutocompletePopup could be implemented as a JSM to avoid bloating HUDService.jsm any further. Minimal changes should be required to do this.

Except for this:

+    this._list.width = this._panel.clientWidth +
+                       ConsoleUtils.scrollbarWidth(this._document);

which brings me to:

+
+  /**
+   * Determine the scrollbar width in a given document.
+   *
+   * Credits:
+   *   Fabian Jakobs <fabian AT ajax DOT org>, the Ajax.org Code Editor (ACE)
+   *
+   * @param nsIDOMDocument aDocument

Not sure that's the usual way to attribute a section of code. I'd recommend giving him credit in the contributors portion of the main license header and referencing the original code in the comment (a link to the ACE repo is probably sufficient).

Also, not really needed in consoleutils as it's only really used inside the autocomplete popup. Should move it there as a utility function if it's even really needed.

r+ with these fixes.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to comment #6)
> msucan: I played with this patch applied and the interaction is very nice.
> (review comments forthcoming)
> 
> I think it makes sense to have return or tab perform the autocompletion and the
> up/down arrows for selection. That's what seemed natural to me with this kind
> of popup. Other comments welcome, but I believe this is consistent with most
> IDE's that do code-completion.

Great, but there's still the weirdness between up/down for navigating in history versus up/down for the popup list. Please try that and see if you have suggestions for improvements wrt. those two cases.

Comment 9

[Rob Campbell [:rc] (:robcee)]

I don't see that as a problem. If you have something in the term and the popup showing, up/down should select from the autocomplete list. If you're typing and there's no completion suggestion, it should go up down in history.

I did notice one strange thing though. If I have a popup for autocompletion and hit the esc key, the popup closes but I'm still able to navigate through autocomplete suggestions with the up/down arrows.

try it with window.lo for an example.

Comment 10

[Mihai Sucan [:msucan]]

(In reply to comment #9)
> I don't see that as a problem. If you have something in the term and the popup
> showing, up/down should select from the autocomplete list. If you're typing and
> there's no completion suggestion, it should go up down in history.
> 
> I did notice one strange thing though. If I have a popup for autocompletion and
> hit the esc key, the popup closes but I'm still able to navigate through
> autocomplete suggestions with the up/down arrows.
> 
> try it with window.lo for an example.

Will look into that and make it work as expected by you. Thanks!

Comment 11

[Mihai Sucan [:msucan]]

(In reply to comment #7)
> Comment on attachment 524199 [details] [diff] [review]
> up/down patch (wip)
> 
> -      for each (var prop in properties) {
> -        prop = prop.trim();
> ...
> +      for (let i = 0; i < properties.length; i++) {
> +        let prop = properties[i].trim();
> 
> Why this change?

I made that change because it was a very wrong way to iterate an array, prone to errors.

Comment 12

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated the patch. Changes:

- rebased on top of bug 642615 because there's some rebasing needed, both patches touch keyboard event handling, and make each other fail tests. :)

- improved keyboard usability, now Escape and Up/Down work as expected.

- added jsdoc comments to all methods in AutocompletePopup.

- moved the new autocomplete popup code into its own JSM.

- added a new mochitest specific for AutocompletePopup.

- and hopefully addressed all your comments.

Thanks for your review. Re-asking for a short review, so you can take a look at it. Danke!

Comment 13

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 524942 [details] [diff] [review]
updated patch

this looks nice. r+ from me assuming no leaks on try. Thanks!

Comment 14

[Mihai Sucan [:msucan]]

Results seem to be fine:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=97c557d8bd07

Thanks for the r+!

Do we ask for any additional review?

Comment 15

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 524942 [details] [diff] [review]
updated patch

yes, we still need a toolkit peer review, though hopefully our review allows that to be speedy. :)

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 524942 [details] [diff] [review]
updated patch

A few code problems and questions to answer here, the test coverage also seems quite weak, I can't see any evidence that you're testing the keypresses are actually having the intended effect for example. You'll also need to speak to a super-reviewer to find out if this needs sr for adding a new module, I think it probably does.

The feature is big enough that I think it deserves a feature page writing up if only to document what the expected goals and release criteria are here. I presume that backing out is the weapon of choice for removing this if there are problems when it hits aurora?

>diff --git a/toolkit/components/console/hudservice/AutocompletePopup.jsm b/toolkit/components/console/hudservice/AutocompletePopup.jsm

>+ * The Original Code is DevTools (HeadsUpDisplay) Console Code
>+ *
>+ * The Initial Developer of the Original Code is
>+ *   Mozilla Foundation

I think we're meant to use "the Mozilla Foundation.".

>+function AutocompletePopup(aDocument)
>+{
>+  this._document = aDocument;
>+
>+  // Reuse the existing popup elements.
>+  this._panel = this._document.querySelector(".webConsole_autocompletePopup");

Any reason not to use an ID here rather than a class? Are you expecting there to be more than one ever (and if so your code doesn't seem to handle that).

>+  if (!this._panel) {
>+    this._panel = this._document.createElementNS(XUL_NS, "panel");
>+    this._panel.setAttribute("class", "webConsole_autocompletePopup");
>+    this._panel.setAttribute("label",
>+      stringBundle.GetStringFromName("Autocomplete.label"));
>+    this._panel.setAttribute("noautofocus", "true");
>+    this._panel.setAttribute("ignorekeys", "true");
>+    let mainPopupSet = this._document.getElementById("mainPopupSet");
>+    mainPopupSet.appendChild(this._panel);

For something you're putting as a general API this makes a big assumption about the document it is using. I don't think that panels need to be in any popupset so I'd just append it to the document element instead, or at least fall back to that if mainPopupSet isn't there.

>+  }
>+
>+  this._list = this._document.querySelector(".webConsole_autocompleteList");
>+  if (!this._list) {
>+    this._list = this._document.createElementNS(XUL_NS, "richlistbox");
>+    this._list.setAttribute("class", "webConsole_autocompleteList");
>+    this._panel.appendChild(this._list);
>+
>+    // Open and hide the panel, so we initialize the API of the richlistbox.
>+    this._panel.width = 1;
>+    this._panel.height = 1;
>+    this._panel.openPopup(null, "overlap", 0, 0, false, false);
>+    this._panel.hidePopup();
>+    this._panel.width = "";
>+    this._panel.height = "";

Eww is this really necessary? Where are you using the richlistbox methods before showing the panel?

>+  }
>+
>+  this._onSelect = this._onSelect.bind(this);
>+  this._onClick = this._onClick.bind(this);

Please don't do this, it makes the code behave in confusing ways.

>+AutocompletePopup.prototype = {
>+  _document: null,
>+  _panel: null,
>+  _list: null,
>+  _onSelect: null,
>+  _onClick: null,
>+
>+  /**
>+   * Open the autocomplete popup panel.
>+   *
>+   * @param nsIDOMNode aAnchor
>+   *        Optional node to anchor the panel to.
>+   */
>+  openPopup: function AP_openPopup(aAnchor)
>+  {
>+    this._panel.openPopup(aAnchor, "after_start", 0, 0, false, false);
>+    this._list.addEventListener("select", this._onSelect, false);
>+    this._list.addEventListener("click", this._onClick, false);

Any reason to add and remove these as the panel shows and hides rather than just adding them with the panel is created?

>+  getItems: function AP_getItems()
>+  {
>+    let items = [];
>+
>+    Array.forEach(this._list.children, function(aItem) {
>+      this.push(aItem._autocompleteItem);

I think it's less confusing to just use items.push rather than rebinding this here.

>+  setItems: function AP_setItems(aItems)
>+  {
>+    this.clearItems();
>+    aItems.forEach(this.appendItem, this);
>+
>+    // Make sure that the new content is properly fitted by the XUL richlistbox.
>+    if (this.isOpen) {
>+      this._document.defaultView.setTimeout(this._updateSize.bind(this), 1);

What is this timeout needed for? Include a comment explaining why at the very least.

>+  _updateSize: function AP__updateSize()
>+  {
>+    // Make sure the bottom border of the richlistbox is visible.
>+    let diff = this._panel.clientWidth - this._list.clientWidth;
>+
>+    this._list.width = this._panel.clientWidth +
>+                       this._scrollbarWidth;
>+
>+    this._list.height = this._panel.clientHeight - Math.floor(diff/2) + 1;

Spaces around operators please.

>+  /**
>+   * Determine the scrollbar width in the current document.
>+   *
>+   * Credits:
>+   *    This code comes from Fabian Jakobs, the ACE project.
>+   *    https://github.com/ajaxorg/ace

Please talk with gerv about the appropriate way to do attributions, I am not sure that this is it.

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+XPCOMUtils.defineLazyGetter(this, "AutocompletePopup", function () {
>+  var obj = {};
>+  try {
>+    Cu.import("resource:///modules/AutocompletePopup.jsm", obj);

This is toolkit code so you should be importing from resource://gre/modules/...

>   let properties = completionPart.split('.');
>   let matchProp;
>   if (properties.length > 1) {
>-      matchProp = properties[properties.length - 1].trimLeft();
>-      properties.pop();
>-      for each (var prop in properties) {
>-        prop = prop.trim();
>+      matchProp = properties.pop().trimLeft();
>+      for (let i = 0; i < properties.length; i++) {
>+        let prop = properties[i].trim();
> 
>         // If obj is undefined or null, then there is no change to run
>         // completion on it. Exit here.
>         if (typeof obj === "undefined" || obj === null) {
>           return null;
>         }

Can you correct the indentation problems while you're here?

Comment 17

[Mihai Sucan [:msucan]]

(In reply to comment #16)
> Comment on attachment 524942 [details] [diff] [review]
> updated patch
> 
> A few code problems and questions to answer here, the test coverage also seems
> quite weak, I can't see any evidence that you're testing the keypresses are
> actually having the intended effect for example. You'll also need to speak to a
> super-reviewer to find out if this needs sr for adding a new module, I think it
> probably does.

I'll look into improving the test coverage, but we also have other tests in the codebase for autocomplete, this is why this patch doesn't include a lot of testing. Those tests actually caught errors in this new code and helped me fix it before submitting the patch.

There's certainly stuff with this new UI that's not tested.

Will ask for super-review as well.


> The feature is big enough that I think it deserves a feature page writing up if
> only to document what the expected goals and release criteria are here. I
> presume that backing out is the weapon of choice for removing this if there are
> problems when it hits aurora?

It seems we'll go for that, if needed. We hope not. :)

Kevin's going to help us with the feature wiki page. I just need to push the updated patch to the try server, so he can get a build to play with.


> >diff --git a/toolkit/components/console/hudservice/AutocompletePopup.jsm b/toolkit/components/console/hudservice/AutocompletePopup.jsm
> 
> >+ * The Original Code is DevTools (HeadsUpDisplay) Console Code
> >+ *
> >+ * The Initial Developer of the Original Code is
> >+ *   Mozilla Foundation
> 
> I think we're meant to use "the Mozilla Foundation.".

Will fix.

> >+function AutocompletePopup(aDocument)
> >+{
> >+  this._document = aDocument;
> >+
> >+  // Reuse the existing popup elements.
> >+  this._panel = this._document.querySelector(".webConsole_autocompletePopup");
> 
> Any reason not to use an ID here rather than a class? Are you expecting there
> to be more than one ever (and if so your code doesn't seem to handle that).

Will switch to an ID.

> >+  if (!this._panel) {
> >+    this._panel = this._document.createElementNS(XUL_NS, "panel");
> >+    this._panel.setAttribute("class", "webConsole_autocompletePopup");
> >+    this._panel.setAttribute("label",
> >+      stringBundle.GetStringFromName("Autocomplete.label"));
> >+    this._panel.setAttribute("noautofocus", "true");
> >+    this._panel.setAttribute("ignorekeys", "true");
> >+    let mainPopupSet = this._document.getElementById("mainPopupSet");
> >+    mainPopupSet.appendChild(this._panel);
> 
> For something you're putting as a general API this makes a big assumption about
> the document it is using. I don't think that panels need to be in any popupset
> so I'd just append it to the document element instead, or at least fall back to
> that if mainPopupSet isn't there.

Good point. Will fix this.

> >+  }
> >+
> >+  this._list = this._document.querySelector(".webConsole_autocompleteList");
> >+  if (!this._list) {
> >+    this._list = this._document.createElementNS(XUL_NS, "richlistbox");
> >+    this._list.setAttribute("class", "webConsole_autocompleteList");
> >+    this._panel.appendChild(this._list);
> >+
> >+    // Open and hide the panel, so we initialize the API of the richlistbox.
> >+    this._panel.width = 1;
> >+    this._panel.height = 1;
> >+    this._panel.openPopup(null, "overlap", 0, 0, false, false);
> >+    this._panel.hidePopup();
> >+    this._panel.width = "";
> >+    this._panel.height = "";
> 
> Eww is this really necessary? Where are you using the richlistbox methods
> before showing the panel?

Same thought here: ewww. The richlistbox methods are accessed before showing the panel when the user wants to add the autocomplete stuff before showing them on screen (less flickering and faster).

If you want I can change the code to always require first opening the panel. Do you want this?


> >+  }
> >+
> >+  this._onSelect = this._onSelect.bind(this);
> >+  this._onClick = this._onClick.bind(this);
> 
> Please don't do this, it makes the code behave in confusing ways.

Will fix.


> >+AutocompletePopup.prototype = {
> >+  _document: null,
> >+  _panel: null,
> >+  _list: null,
> >+  _onSelect: null,
> >+  _onClick: null,
> >+
> >+  /**
> >+   * Open the autocomplete popup panel.
> >+   *
> >+   * @param nsIDOMNode aAnchor
> >+   *        Optional node to anchor the panel to.
> >+   */
> >+  openPopup: function AP_openPopup(aAnchor)
> >+  {
> >+    this._panel.openPopup(aAnchor, "after_start", 0, 0, false, false);
> >+    this._list.addEventListener("select", this._onSelect, false);
> >+    this._list.addEventListener("click", this._onClick, false);
> 
> Any reason to add and remove these as the panel shows and hides rather than
> just adding them with the panel is created?

We need to do this because we have multiple Web Consoles and instances of AutocompletePopup that reuse the same DOM elements. If we always add the event handlers, then all web consoles will be notified of select/click events. I need to have this only for the current Web Console that opens the popup.


> >+  getItems: function AP_getItems()
> >+  {
> >+    let items = [];
> >+
> >+    Array.forEach(this._list.children, function(aItem) {
> >+      this.push(aItem._autocompleteItem);
> 
> I think it's less confusing to just use items.push rather than rebinding this
> here.

No problem, will fix.

> >+  setItems: function AP_setItems(aItems)
> >+  {
> >+    this.clearItems();
> >+    aItems.forEach(this.appendItem, this);
> >+
> >+    // Make sure that the new content is properly fitted by the XUL richlistbox.
> >+    if (this.isOpen) {
> >+      this._document.defaultView.setTimeout(this._updateSize.bind(this), 1);
> 
> What is this timeout needed for? Include a comment explaining why at the very
> least.

If I invoke _updateSize() without any delay the sizes are not correctly given by Gecko. Perhaps it's a reflow problem or something. I only got it to work with that delay. Suggestions for a better fix are welcome!


> >+  _updateSize: function AP__updateSize()
> >+  {
> >+    // Make sure the bottom border of the richlistbox is visible.
> >+    let diff = this._panel.clientWidth - this._list.clientWidth;
> >+
> >+    this._list.width = this._panel.clientWidth +
> >+                       this._scrollbarWidth;
> >+
> >+    this._list.height = this._panel.clientHeight - Math.floor(diff/2) + 1;
> 
> Spaces around operators please.

Will fix.

> >+  /**
> >+   * Determine the scrollbar width in the current document.
> >+   *
> >+   * Credits:
> >+   *    This code comes from Fabian Jakobs, the ACE project.
> >+   *    https://github.com/ajaxorg/ace
> 
> Please talk with gerv about the appropriate way to do attributions, I am not
> sure that this is it.

Will do.

> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >+XPCOMUtils.defineLazyGetter(this, "AutocompletePopup", function () {
> >+  var obj = {};
> >+  try {
> >+    Cu.import("resource:///modules/AutocompletePopup.jsm", obj);
> 
> This is toolkit code so you should be importing from resource://gre/modules/...

Will fix.

> >   let properties = completionPart.split('.');
> >   let matchProp;
> >   if (properties.length > 1) {
> >-      matchProp = properties[properties.length - 1].trimLeft();
> >-      properties.pop();
> >-      for each (var prop in properties) {
> >-        prop = prop.trim();
> >+      matchProp = properties.pop().trimLeft();
> >+      for (let i = 0; i < properties.length; i++) {
> >+        let prop = properties[i].trim();
> > 
> >         // If obj is undefined or null, then there is no change to run
> >         // completion on it. Exit here.
> >         if (typeof obj === "undefined" || obj === null) {
> >           return null;
> >         }
> 
> Can you correct the indentation problems while you're here?

Sure.

Thanks for your review!

Comment 18

[Mihai Sucan [:msucan]]

The feature wiki page:

https://wiki.mozilla.org/DevTools/Features/AutocompletionPopup

Thanks to Kevin!

Comment 19

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Updated the patch to address the review comments from Dave.

Dave: I hope I have addressed all your concerns. I also added a test for keyboard usage of the autocomplete popup. Please let me know if there's anything else I need to change.

Benjamin: as per comment 16 I am asking for super review from you.

Gerv: as per comment 16 I am asking for feedback from you. I reuse a function from Ace and I need to know if I am handling the code attribution correctly.

Thank you all!

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 527365 [details] [diff] [review]
patch update 3

Please also add a test that pressing return has the desired result but otherwise this looks good and I don't need to see it again.

Comment 21

[Mihai Sucan [:msucan]]

Attachment: patch update 4

Updated the patch to also test the Return key. Thanks for the review+ Dave!

Still asking for super-review from Benjamin and for feedback on the code credits from Gerv.

Comment 22

[Mihai Sucan [:msucan]]

Pushed this patch to the try server.

Green results:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=d1ab851034fa

Builds and logs:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-d1ab851034fa

Comment 23

[Mihai Sucan [:msucan]]

Comment on attachment 527500 [details] [diff] [review]
patch update 4

As asked by Benjamin Smedberg, switching to Neil Rashbrook for superreview.

Neil: please see comment 16.

Thank you!

Comment 24

[[e-mail]]

Comment on attachment 527500 [details] [diff] [review]
patch update 4

Review of attachment 527500 [details] [diff] [review]:

(I'm assuming that I only need to review AutocompletePopup.jsm)

::: toolkit/components/console/hudservice/AutocompletePopup.jsm
@@ +72,5 @@
+  if (!this._panel) {
+    this._panel = this._document.createElementNS(XUL_NS, "panel");
+    this._panel.setAttribute("id", "webConsole_autocompletePopup");
+    this._panel.setAttribute("label",
+      stringBundle.GetStringFromName("Autocomplete.label"));

You don't give the panel a title bar, so the label has no effect?

@@ +81,5 @@
+    if (mainPopupSet) {
+      mainPopupSet.appendChild(this._panel);
+    }
+    else {
+      this._document.appendChild(this._panel);

This would need to be this._document.documentElement.appendChild

@@ +85,5 @@
+      this._document.appendChild(this._panel);
+    }
+  }
+
+  this._list = this._document.querySelector(".webConsole_autocompleteList");

The list is always the child of the panel; if the panel exists, just get it, otherwise you might as well create it when you create the panel.

@@ +91,5 @@
+    this._list = this._document.createElementNS(XUL_NS, "richlistbox");
+    this._list.setAttribute("class", "webConsole_autocompleteList");
+    this._panel.appendChild(this._list);
+
+    // Open and hide the panel, so we initialize the API of the richlistbox.

Out of interest, which API do you use before opening the popup?

@@ +117,5 @@
+  {
+    this._panel.openPopup(aAnchor, "after_start", 0, 0, false, false);
+
+    if (this.onSelect) {
+      this._onSelect = this.onSelect.bind(this);

Why bind this again? You already bound this when you set .onSelect

@@ +170,5 @@
+
+    this._document = null;
+    this._list = null;
+
+    this._panel = null;

Nit: stray blank line.

@@ +183,5 @@
+  getItems: function AP_getItems()
+  {
+    let items = [];
+
+    Array.forEach(this._list.children, function(aItem) {

Especially in XUL, where you generally don't have to worry about non-element nodes unless you put them there, please try to stick with .childNodes as it's more efficient than .children

@@ +222,5 @@
+
+    this._list.width = this._panel.clientWidth +
+                       this._scrollbarWidth;
+
+    this._list.height = this._panel.clientHeight - Math.floor(diff / 2) + 1;

Normally panels are pretty good at sizing to their children, although I know richlistboxes contain a scrollbox which might upset the calculations... do you have a screenshot of the problem?

@@ +290,5 @@
+  appendItem: function AP_appendItem(aItem)
+  {
+    let description = this._document.createElementNS(XUL_NS, "description");
+    let text = this._document.createTextNode(aItem.label);
+    description.appendChild(text);

You could just write description.textContent = text;

@@ +357,5 @@
+    if (this.selectedIndex < (this.itemCount - 1)) {
+      this.selectedIndex++;
+    }
+    else if (this.selectedIndex == (this.itemCount - 1)) {
+      this.selectedIndex = -1;

There's no need to check twice; either the selected index is the maximum or it is not. (Same applies to the -1 check below.)

@@ +391,5 @@
+   *
+   * @private
+   */
+  get _scrollbarWidth()
+  {

If you really really need to know the width of a scrollbar, just create one...

Comment 25

[Gervase Markham [:gerv]]

If you want my input, it helps to CC me or request feedback? or r? from me :-)

The answer is that given that ACE is tri-licensed, if you are importing code from an ACE file, you should import its Contributors list to the list at the top of the file you are copying the code into. If Fabien is not listed, you can add him too if you like. What you write above the function is unregulated. It helps to say where it came from in case bugs are found in it, but there's no legal requirement.

Gerv

Comment 26

[Mihai Sucan [:msucan]]

(In reply to comment #25)
> If you want my input, it helps to CC me or request feedback? or r? from me :-)
> 
> The answer is that given that ACE is tri-licensed, if you are importing code
> from an ACE file, you should import its Contributors list to the list at the
> top of the file you are copying the code into. If Fabien is not listed, you can
> add him too if you like. What you write above the function is unregulated. It
> helps to say where it came from in case bugs are found in it, but there's no
> legal requirement.

Thanks for your answer!

I did ask for feedback from you, but it seems I used the wrong email address.

Comment 27

[Mihai Sucan [:msucan]]

(In reply to comment #24)
> Comment on attachment 527500 [details] [diff] [review] [review]
> patch update 4
> 
> Review of attachment 527500 [details] [diff] [review] [review]:
> 
> @@ +91,5 @@
> +    this._list = this._document.createElementNS(XUL_NS, "richlistbox");
> +    this._list.setAttribute("class", "webConsole_autocompleteList");
> +    this._panel.appendChild(this._list);
> +
> +    // Open and hide the panel, so we initialize the API of the richlistbox.
> 
> Out of interest, which API do you use before opening the popup?

I add the richlistbox items before the popup opens. I prefer to add them off-screen first.

The manipulation of the richlistbox implies I use some of its specific API, like selectedIndex/Item and so on. I tried to avoid that, but I cannot guarantee that none of its richlistbox-specific API is called before the popup opens. The only way to guarantee that is to require the user of the AutocompletePopup.jsm to open the popup before working with the items, which I erred against - I believe that's not ideal.

If you consider I should change this, please let me know.


> @@ +222,5 @@
> +
> +    this._list.width = this._panel.clientWidth +
> +                       this._scrollbarWidth;
> +
> +    this._list.height = this._panel.clientHeight - Math.floor(diff / 2) + 1;
> 
> Normally panels are pretty good at sizing to their children, although I know
> richlistboxes contain a scrollbox which might upset the calculations... do you
> have a screenshot of the problem?

The richlistbox has the entire content height, so there's no scrollbar. The height of the list fits the entire list of items, but the panel popup does not (and cannot) make all the items visible.

For a screen shot:

http://img.i7m.de/show/sqnsj.png

So I am forced to set the list width and height based on the panel width and height. To do that properly I need a delay and I need the width of the vertical scroll bar.


> @@ +391,5 @@
> +   *
> +   * @private
> +   */
> +  get _scrollbarWidth()
> +  {
> 
> If you really really need to know the width of a scrollbar, just create one...

Hm, thanks for the suggestion. I didn't know of the xul:scrollbar element. I have updated the code locally and it works. Yay! This means I can remove the code I copied from Ace.

Thanks for your super-review! I will update the patch to address your comments.

Comment 28

[Mihai Sucan [:msucan]]

Attachment: patch update 5

Updated the patch to address the super-review comments.

Looking forward to your updated sr, thank you!

Comment 29

[[e-mail]]

(In reply to comment #27)
> The richlistbox has the entire content height, so there's no scrollbar. The
> height of the list fits the entire list of items, but the panel popup does
> not (and cannot) make all the items visible.
> 
> For a screen shot:
> 
> http://img.i7m.de/show/sqnsj.png

It just occurs to me that this might be because the richlistbox has no flex.
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#925

Comment 30

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased the patch on top of the latest attachment 531876 [details] [diff] [review] from bug 642615. There are only minor changes in the HUDService.jsm file - no other changes.

Comment 31

[[e-mail]]

(In reply to comment #29)
> It just occurs to me that this might be because the richlistbox has no flex.
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> autocomplete.xml#925

It was pointed out that this doesn't completely solve the problem, because the popup sizes to the width of the widest item before the scrollbar width is taken into account. One option is to force the scrollbar to appear by styling the appropriate element with overflow-y: scroll;

Comment 32

[[e-mail]]

Comment on attachment 531878 [details] [diff] [review]
rebased patch

>+    if (this._onSelect) {
>+      this._list.removeEventListener("select", this._onSelect, false);
>+      delete this._onSelect;
>+    }
>+
>+    if (this._onClick) {
>+      this._list.removeEventListener("click", this._onClick, false);
>+      delete this._onClick;
>+    }
You forgot to change these to match the changes to openPopup.

>+    this._list.width = this._panel.clientWidth +
>+                       this._scrollbarWidth;
So you're always going to allow space for the scrollbar, right?

>+    this._list.height = this._panel.clientHeight - Math.floor(diff / 2) + 1;
But I don't understand what this calculation is doing...

>+    hbox.setAttribute("style", "height: 1px; overflow: hidden");
height: 0%; might work better

Comment 33

[Mihai Sucan [:msucan]]

(In reply to comment #31)
> (In reply to comment #29)
> > It just occurs to me that this might be because the richlistbox has no flex.
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> > autocomplete.xml#925
> 
> It was pointed out that this doesn't completely solve the problem, because
> the popup sizes to the width of the widest item before the scrollbar width
> is taken into account. One option is to force the scrollbar to appear by
> styling the appropriate element with overflow-y: scroll;

Adding overflow-y: scroll to the richlistbox element does not solve the problem.

Comment 34

[Mihai Sucan [:msucan]]

(In reply to comment #32)
> Comment on attachment 531878 [details] [diff] [review] [review]
> rebased patch
> 
> >+    if (this._onSelect) {
> >+      this._list.removeEventListener("select", this._onSelect, false);
> >+      delete this._onSelect;
> >+    }
> >+
> >+    if (this._onClick) {
> >+      this._list.removeEventListener("click", this._onClick, false);
> >+      delete this._onClick;
> >+    }
> You forgot to change these to match the changes to openPopup.

Yeah, will fix this.

> >+    this._list.width = this._panel.clientWidth +
> >+                       this._scrollbarWidth;
> So you're always going to allow space for the scrollbar, right?

Yes.

> >+    this._list.height = this._panel.clientHeight - Math.floor(diff / 2) + 1;
> But I don't understand what this calculation is doing...

I need to update the richlistbox height as well, so it fits the panel height, otherwise the list is taller than the panel. The bottom border is not visible if I just use panel.clientHeight, so I determine the border size using in the diff variable. In the updated patch I am going to make this clearer.


> >+    hbox.setAttribute("style", "height: 1px; overflow: hidden");
> height: 0%; might work better

Will use this in the updated patch.

Thank you for your comments!

Comment 35

[Mihai Sucan [:msucan]]

Attachment: patch update 6

Updated the patch based on your latest comments.

Please take a look at this patch when you see fit for us to be able to land it in time for Firefox 6. Thank you!

Comment 36

[[e-mail]]

(In reply to comment #34)
> (In reply to comment #32)
> > (From update of attachment 531878 [details] [diff] [review])
> > >+    this._list.height = this._panel.clientHeight - Math.floor(diff / 2) + 1;
> > But I don't understand what this calculation is doing...
> I need to update the richlistbox height as well, so it fits the panel
> height, otherwise the list is taller than the panel
Does <richlistbox flex="1"> at least solve that part for you? (I don't think that the calculation you're using is guaranteed to work in all cases.)

Comment 37

[Mihai Sucan [:msucan]]

(In reply to comment #36)
> (In reply to comment #34)
> > (In reply to comment #32)
> > > (From update of attachment 531878 [details] [diff] [review] [review])
> > > >+    this._list.height = this._panel.clientHeight - Math.floor(diff / 2) + 1;
> > > But I don't understand what this calculation is doing...
> > I need to update the richlistbox height as well, so it fits the panel
> > height, otherwise the list is taller than the panel
> Does <richlistbox flex="1"> at least solve that part for you? (I don't think
> that the calculation you're using is guaranteed to work in all cases.)

Nice. It works with flex=1, I don't need to update the richlistbox height. Thanks! Will update the patch.

Comment 38

[Mihai Sucan [:msucan]]

Attachment: patch update 7

Added the flex=1 attribute to the richlistbox and simplified _updateSize().

Comment 39

[[e-mail]]

(In reply to comment #33)
> (In reply to comment #31)
> > (In reply to comment #29)
> > > It just occurs to me that this might be because the richlistbox has no flex.
> > > http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> > > autocomplete.xml#925
> > It was pointed out that this doesn't completely solve the problem, because
> > the popup sizes to the width of the widest item before the scrollbar width
> > is taken into account. One option is to force the scrollbar to appear by
> > styling the appropriate element with overflow-y: scroll;
> Adding overflow-y: scroll to the richlistbox element does not solve the
> problem.
I know, that wasn't the appropriate element ;-) But the problem anyway is that the appropriate elements are all anonymous and therefore not easily styled.

Comment 40

[[e-mail]]

Comment on attachment 532702 [details] [diff] [review]
patch update 7

>+    this._list.setAttribute("flex", "1");
Nit: Could write this as this._list.flex = 1;

Comment 41

[[e-mail]]

Heh, so it turns out that bug 562740 made life harder for you; before, you could have used the popup-with-scrollbar binding, and the scrollbar would have automatically extended the width of the popup for you.

Comment 42

[Mihai Sucan [:msucan]]

Attachment: [checked-in] [in-devtools] patch update 8

Changed to this._list.flex = 1 and rebased the patch on top of the latest attachment 532961 [details] [diff] [review] from bug 642615.

Thank you for the sr+!

I also pushed this patch and the one from bug 642615 to the try server. Results:

http://tbpl.mozilla.org/?tree=Try&rev=ec27167505a8

Builds and logs:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ec27167505a8

Comment 43

[Mihai Sucan [:msucan]]

Comment on attachment 532965 [details] [diff] [review]
[checked-in] [in-devtools] patch update 8

Patch landed:

http://hg.mozilla.org/projects/devtools/rev/38c72ce2dae4

Comment 44

[Mihai Sucan [:msucan]]

Attachment: [checked-in] [in-devtools] quick followup fix

A quick follow up patch. Now that both bug 577721 and this patch have landed in the devtools repo, there's a small issue when the Web Console is positioned inside a xul:panel: rarely, during quick typing, it happens that the xul:panel of the autocomplete popup is wrongly sized. This patch fixes the problem.

Comment 45

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 533306 [details] [diff] [review]
[checked-in] [in-devtools] quick followup fix

r+

Comment 46

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 533306 [details] [diff] [review]
[checked-in] [in-devtools] quick followup fix

http://hg.mozilla.org/projects/devtools/rev/28bb68eee2fd

Comment 47

[Dave Camp (:dcamp)]

Comment on attachment 532965 [details] [diff] [review]
[checked-in] [in-devtools] patch update 8

http://hg.mozilla.org/mozilla-central/rev/38c72ce2dae4

Comment 48

[Dave Camp (:dcamp)]

Comment on attachment 533306 [details] [diff] [review]
[checked-in] [in-devtools] quick followup fix

http://hg.mozilla.org/mozilla-central/rev/28bb68eee2fd

Comment 49

[AndreiD[QA]]

Verified fixed on:
Windows 7:
Mozilla/5.0 (Windows NT 6.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2
Window XP:
Mozilla/5.0 (Windows NT 5.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2
Mac OS 10.6
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a2) Gecko/20110525 Firefox/6.0a2 
Linux i686:
Mozilla/5.0 (X11; Linux i686; rv:6.0a2) Gecko/20110525 Firefox/6.0a2

*Note: A pop up listing of possible completions is available. Marking this as Verified.

Comment 50

[Eric Shepherd [:sheppy]]

Little doc update here:

https://developer.mozilla.org/en/Tools/Web_Console#The_command_line_interpreter