831693 - Experiment with auto completion in Inspector Searchbox

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Girish Sharma [:Optimizer]]

bug 650804 introduces searching features in Inspector.

There should be some sort of either auto completion, or popup, or some sort of help to the user to choose the correct node.

Comment 1

[Girish Sharma [:Optimizer]]

Some thoughts for autocompletion suggestions:

The use case for auto complete suggestion is when the user has not yet written a valid selector as in 'di' where a valid one is 'div' So somehow we have to come up with 'div' and make sure that 'div' selector exists on the page.

This can be done using the following approach.

Based on the user's last correct selector, we make a list of all classes, ids and tagnames. Show them in the suggestions to the user until he has written a valid selector again, in which case we update our list of classes, ids and tagnames and this continues.

While this approach becomes easier as the text in the searchbox increase, it is very heavy for the first time, like when user has just written 'di'. In this case, we will have to parse the whole DOM tree to get all the ids, tagnames and classes. We can optimize here to check if a '.' or '#' is present or not and only populate the required of the three [id, tagname or class].

Comment 2

[Girish Sharma [:Optimizer]]

I did some stress testing to get all the ids (or classNames or tags) in the worst case scenario that I mentioned above.

querySelectorAll is the best choice and its very fast too. I tested on tbpl after clicking on the green down arrow more than 30 times.

ids took 9 ms
classes took 14
tagnames took 103

scratchpad: http://pastebin.mozilla.org/2066540

Comment 3

[Girish Sharma [:Optimizer]]

Attachment: WIP 0.1

Adds basic functionality, along with a popup displaying 10 suggestions (alphabetically sorted and selected)

These suggestions will only show what is a possible selector. For example, if you have typed '#as' in the searchbox, the suggestion for '#asdf' will only appear if that is a  valid selector. Same goes for when you have 'div.a' typed in searchbox and 'div.asdf' is suggested.

Much more work to do on this.

Comment 4

[Girish Sharma [:Optimizer]]

Attachment: screenshot of WIP 0.1

Screenshot displaying the popup, the suggestions and a currently selected suggestion.

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: WIP 0.5

pretty much what I want functionality wise. A lot of edge cases to handle still.
Then code cleanup and tests.

Not requesting actual feedback, but it would be nice to get one.

Comment 6

[Girish Sharma [:Optimizer]]

Attachment: WIP 0.9

Ready for feedback.

Two cases still unsupported and marked as TODO. Working on them, but might open separate bug for them, given that we want to support those cases.

Comment 7

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #6)
> Created attachment 704345 [details] [diff] [review]
> WIP 0.9
> 
> Ready for feedback.
> 
> Two cases still unsupported and marked as TODO. Working on them, but might
> open separate bug for them, given that we want to support those cases.

I'm sorry, I won't have time to look at this in depth.
Functionality-wise this feels ok, but I haven't really tested thoroughly.

Comment 8

[Girish Sharma [:Optimizer]]

sad.
I am holding an auction for this bug now, for either reviewing or feedback :)

Comment 9

[Girish Sharma [:Optimizer]]

Comment on attachment 704345 [details] [diff] [review]
WIP 0.9

Yay :)

Comment 10

[Paul Rouget [:paul]]

Comment on attachment 704345 [details] [diff] [review]
WIP 0.9

This is a big patch. It will take some time to review it.

Can you extract the logic of this code into a standalone object? (as in, not part of the InspectorPanel object)

Comment 11

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #10)
> Comment on attachment 704345 [details] [diff] [review]
> WIP 0.9
> 
> This is a big patch. It will take some time to review it.
> 
> Can you extract the logic of this code into a standalone object? (as in, not
> part of the InspectorPanel object)

I was also thinking something like that. It will also make it possible for gcli to use the same module in the --selector options for any command.

Comment 12

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

Something like this ?

Comment 13

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

I meant something like this... ?

Comment 14

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2

- Cleaner css rules
- Handles one of the two TODO's i.e. #.someId is considered as ID only and similar for .#some-class while suggesting auto completion. Although the querySelectorAll is not as intelligent :( . For querySelectorAll('#.someId') it errors out saying invalid input string.
- For the above, I introduced a state manager, using the same I will not properly handle child node suggestion too.

Comment 15

[Girish Sharma [:Optimizer]]

Attachment: screenshot of patch v0.2

Comment 16

[Girish Sharma [:Optimizer]]

Attachment: Changes to autocompletepopup

These are the changes to webconsole.js and AutocompletePopup.jsm required to make it reusable by inspector's seachbox's autocomplete popup.

Fixed a couple of things alongside:
1) Webconsole's popup appears to be at an offset the very first time. (bug 785433)
2) Added several options to really make the popup useful.
3) With minor changes, bug 833333 is also fixed, which I have not done here though.

Comment 17

[Girish Sharma [:Optimizer]]

Attachment: patch v0.5

This is ready for review. All TODO's fixed, edge cases handled and webconsole's autocomplete popup reused.

This will even make it much much easy for the popup to be reused in autocompletion in scratchpad.

The Web Console side changes along with the AutocompletePopup.jsm file changes have also been separated out in the other patch that is attached, so that Mihai can review them.

The AutocompletePopup.jsm file as such is exactly similar but is moved from devtools/webconsole/ to devtools/shared/ in this patch (as compared to attachment 707164 [details] [diff] [review]: Changes to autocompletepopup). This was done so as to make the changes easier to review. (if the file is in same place, the changes are visible as a dif, but if a file is moved, no diff is there, just deletion of previous file and addition of new)

Comment 18

[Mihai Sucan [:msucan]]

Comment on attachment 707164 [details] [diff] [review]
Changes to autocompletepopup

Review of attachment 707164 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thank you for the improvements.

r+ with the comments below addressed.

::: browser/devtools/webconsole/AutocompletePopup.jsm
@@ +21,5 @@
>   *        The document you want the popup attached to.
> + * @param Object aOptions
> + *        An object consiting any of the following options:
> + *        - panelId {String} The id for the popup panel.
> + *        - panelClass {String} The clss for the popup panel.

type: s/clss/class/

Do we need both ids and classes for the panel and the listbox?

@@ +29,5 @@
> + *        - autoSelect {Boolean} Boolean to allow the first entry of the popup
> + *                     panel to be automatically selected when the popup shows.
> + *        - fixedWidth {Boolean} Boolean to control dynamic width of the popup.
> + *        - maxWidth {String} The maxwidth attribute for the richlistbox.
> + *        - maxHeight {String} The maxheight attribute for the richlistbox.

maxWidth and maxHeight are unused.

@@ +37,2 @@
>   */
> +this.AutocompletePopup = function AutocompletePopup(aDocument, aOptions)

You can provide defaults for optional arguments.

function foo(a, b={}) { ...}

@@ -38,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"));

Why did you remove the panel label?

@@ +78,2 @@
>  
> +  if (!this._list) {

Why do you do this? Is there a case when the popup is available but it has no richlistbox?

@@ +95,5 @@
> +    this._list.setAttribute("class", aOptions.listBoxClass);
> +  }
> +
> +  if (this.onSelect) {
> +    this._list.addEventListener("select", this.onSelect, false);

I did intentionally add/remove these event listeners in openPopup/hidePopup:

- I wanted to avoid these events from firing when the popup is not open.
- the same popup was reused by multiple web console instances, each instance with its own listeners.

Please double check that the popup is now added inside the toolbox, for each web console instance, such that we avoid potential breakage.

@@ +317,5 @@
>     */
>    set selectedIndex(aIndex) {
>      this._list.selectedIndex = aIndex;
> +    if (this.isOpen) {
> +      this._list.ensureIndexIsVisible(this._list.selectedIndex);

Why did you add the isOpen check here? To avoid breakage when ensureIndexIsVisible() is not available? If yes, then check for the method existence, instead of isOpen.

@@ +339,5 @@
>     */
>    set selectedItem(aItem) {
>      this._list.selectedItem = this._findListItem(aItem);
> +    if (this.isOpen) {
> +      this._list.ensureIndexIsVisible(this._list.selectedIndex);

Ditto.

@@ +350,5 @@
>     * @param object aItem
>     *        The item you want appended to the list. The object must have a
>     *        "label" property which is used as the displayed value.
>     */
>    appendItem: function AP_appendItem(aItem)

Please describe the new item properties.

@@ +419,5 @@
>     * Getter for the number of items in the popup.
>     * @type number
>     */
>    get itemCount() {
> +    return this._list.itemCount;

Please avoid this change: |itemCount| is not always available.

Comment 19

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #18)
> ::: browser/devtools/webconsole/AutocompletePopup.jsm
> @@ +21,5 @@
> >   *        The document you want the popup attached to.
> > + * @param Object aOptions
> > + *        An object consiting any of the following options:
> > + *        - panelId {String} The id for the popup panel.
> > + *        - panelClass {String} The clss for the popup panel.
> 
> type: s/clss/class/
> 
> Do we need both ids and classes for the panel and the listbox?

No idea, maybe ... or maybe its an overkill. For inspector, I only need id.

> @@ +29,5 @@
> > + *        - autoSelect {Boolean} Boolean to allow the first entry of the popup
> > + *                     panel to be automatically selected when the popup shows.
> > + *        - fixedWidth {Boolean} Boolean to control dynamic width of the popup.
> > + *        - maxWidth {String} The maxwidth attribute for the richlistbox.
> > + *        - maxHeight {String} The maxheight attribute for the richlistbox.
> 
> maxWidth and maxHeight are unused.

Shit, I missed it. I need that for inspector.

> @@ +37,2 @@
> >   */
> > +this.AutocompletePopup = function AutocompletePopup(aDocument, aOptions)
> 
> You can provide defaults for optional arguments.
> 
> function foo(a, b={}) { ...}

Yes, I can probably move 3-4 things up there.

> @@ -38,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"));
> 
> Why did you remove the panel label?

1. I don't think it is actually required.
2. To make things generic and not web console specific.

Why was the label there at all ?
> @@ +78,2 @@
> >  
> > +  if (!this._list) {
> 
> Why do you do this? Is there a case when the popup is available but it has
> no richlistbox?

When the panel itself is not there, the _list will be null. So I create it. See the above few lines where I do this._list = null. Also, its not guaranteed that the panel will have any child at all.

> @@ +95,5 @@
> > +    this._list.setAttribute("class", aOptions.listBoxClass);
> > +  }
> > +
> > +  if (this.onSelect) {
> > +    this._list.addEventListener("select", this.onSelect, false);
> 
> I did intentionally add/remove these event listeners in openPopup/hidePopup:
> 
> - I wanted to avoid these events from firing when the popup is not open.
> - the same popup was reused by multiple web console instances, each instance
> with its own listeners.
> 
> Please double check that the popup is now added inside the toolbox, for each
> web console instance, such that we avoid potential breakage.

Okay will check. But I think that events fired while panel is not open cannot happen, as click, keypress and select - all need the panel to be visible.


> @@ +317,5 @@
> >     */
> >    set selectedIndex(aIndex) {
> >      this._list.selectedIndex = aIndex;
> > +    if (this.isOpen) {
> > +      this._list.ensureIndexIsVisible(this._list.selectedIndex);
> 
> Why did you add the isOpen check here? To avoid breakage when
> ensureIndexIsVisible() is not available? If yes, then check for the method
> existence, instead of isOpen.

Yes, I did this to avoid the console errors if someone adds any item when the panel is not open.

> @@ +339,5 @@
> >     */
> >    set selectedItem(aItem) {
> >      this._list.selectedItem = this._findListItem(aItem);
> > +    if (this.isOpen) {
> > +      this._list.ensureIndexIsVisible(this._list.selectedIndex);
> 
> Ditto.
> 
> @@ +350,5 @@
> >     * @param object aItem
> >     *        The item you want appended to the list. The object must have a
> >     *        "label" property which is used as the displayed value.
> >     */
> >    appendItem: function AP_appendItem(aItem)
> 
> Please describe the new item properties.

Ah, yes. :)

> @@ +419,5 @@
> >     * Getter for the number of items in the popup.
> >     * @type number
> >     */
> >    get itemCount() {
> > +    return this._list.itemCount;
> 
> Please avoid this change: |itemCount| is not always available.

Is it ? I thought why do extra work when we already have a getter. Will revert.

Comment 20

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma [:Optimizer] from comment #19)
> (In reply to Mihai Sucan [:msucan] from comment #18)
> > ::: browser/devtools/webconsole/AutocompletePopup.jsm
> > @@ +21,5 @@
> > >   *        The document you want the popup attached to.
> > > + * @param Object aOptions
> > > + *        An object consiting any of the following options:
> > > + *        - panelId {String} The id for the popup panel.
> > > + *        - panelClass {String} The clss for the popup panel.
> > 
> > type: s/clss/class/
> > 
> > Do we need both ids and classes for the panel and the listbox?
> 
> No idea, maybe ... or maybe its an overkill. For inspector, I only need id.

Then don't add what you don't need.


> > @@ +29,5 @@
> > > + *        - autoSelect {Boolean} Boolean to allow the first entry of the popup
> > > + *                     panel to be automatically selected when the popup shows.
> > > + *        - fixedWidth {Boolean} Boolean to control dynamic width of the popup.
> > > + *        - maxWidth {String} The maxwidth attribute for the richlistbox.
> > > + *        - maxHeight {String} The maxheight attribute for the richlistbox.
> > 
> > maxWidth and maxHeight are unused.
> 
> Shit, I missed it. I need that for inspector.

Can you do this from CSS? I think we should avoid adding options here when CSS can do it for us.


> > @@ -38,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"));
> > 
> > Why did you remove the panel label?
> 
> 1. I don't think it is actually required.
> 2. To make things generic and not web console specific.

Agreed.


> > @@ +78,2 @@
> > >  
> > > +  if (!this._list) {
> > 
> > Why do you do this? Is there a case when the popup is available but it has
> > no richlistbox?
> 
> When the panel itself is not there, the _list will be null. So I create it.
> See the above few lines where I do this._list = null.

This was my point. You know the list is not there, so create it directly there.


> Also, its not guaranteed that the panel will have any child at all.

How is this possible? Does some other tool remove the contents of the panel?


> > @@ +95,5 @@
> > > +    this._list.setAttribute("class", aOptions.listBoxClass);
> > > +  }
> > > +
> > > +  if (this.onSelect) {
> > > +    this._list.addEventListener("select", this.onSelect, false);
> > 
> > I did intentionally add/remove these event listeners in openPopup/hidePopup:
> > 
> > - I wanted to avoid these events from firing when the popup is not open.
> > - the same popup was reused by multiple web console instances, each instance
> > with its own listeners.
> > 
> > Please double check that the popup is now added inside the toolbox, for each
> > web console instance, such that we avoid potential breakage.
> 
> Okay will check. But I think that events fired while panel is not open
> cannot happen, as click, keypress and select - all need the panel to be
> visible.

True, unless it's shared by other consoles. Back in the days of no toolbox, the popup was shared. Please check.


Thank you!

Comment 21

[Girish Sharma [:Optimizer]]

Attachment: patch v0.6

- Addressed Mihai's comments about AutocompletePopup.jsm
- Adding default classes to both panel and richlistbox, so that they can be styled easily.
- Fixed some platform issues in the css. (richlistbox appearance was not becoming none, and on windows, opacity: 0.9 was not appearing on the panel)

Comment 22

[Girish Sharma [:Optimizer]]

Attachment: patch v0.6

Forgot to remove the string Autocomplete.label from webconsole.properties file.

Comment 23

[Girish Sharma [:Optimizer]]

Attachment: screenshot of patch v0.6

Comment 24

[Girish Sharma [:Optimizer]]

Let me explain how the suggestions are sorted :

The selector with the highest matches is shown just above the searchbox as the first entry and the selector with the lowest matches is shown farthest as the last entry (even though the actual indexes of these two are opposite)

The selector with the same number of matches are sorted alphabetically with the lower lexicographical selector nearer to the searchbox.

The selectors with only one count do not show their count, as it is useless and they are ashamed :)

Comment 25

[Mihai Sucan [:msucan]]

Comment on attachment 707716 [details] [diff] [review]
patch v0.6

Review of attachment 707716 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the updates. Looks good, r+!

I do have some comments below. I did not review all of the code - I just skimmed through the code that is not related to the autocomplete popup.

My main concerns are with hard-coded pixel sizes and text directions. Nonetheless, I'm letting paul take the lead here with the rest of the review.

::: browser/devtools/inspector/InspectorPanel.jsm
@@ +23,5 @@
>    "resource:///modules/devtools/Highlighter.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "ToolSidebar",
>    "resource:///modules/devtools/Sidebar.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Selectors",
> +  "resource:///modules/devtools/Selectors.jsm");

Shouldn't this be more like SelectorsSearch.jsm?

::: browser/devtools/inspector/inspector.css
@@ +19,5 @@
> +  -moz-appearance: none !important;
> +  background: rgba(0,0,0,0);
> +  border-width: 0px !important;
> +  width: 250px;
> +  max-width: 250px;

Is max-width needed when width is non-relative?

Also, why px and not something in ems? I'd like the width to vary based on font sizes.

@@ +45,5 @@
> +  color: #ddd;
> +}
> +
> +#searchbox-panel-listbox > richlistitem > .initial-value {
> +  max-width: 130px;

px vs. em

@@ +51,5 @@
> +  margin-right: 0;
> +}
> +
> +#searchbox-panel-listbox > richlistitem > .autocomplete-value {
> +  max-width: 150px;

ditto

@@ +57,5 @@
> +  padding: 1px 0;
> +}
> +
> +#searchbox-panel-listbox > richlistitem > .autocomplete-count {
> +  text-align: right;

LTR versus RTL. Please test with RTL UIs. Add intl.uidirection.en=rtl to about:config.

See text-align start and end:
https://developer.mozilla.org/en-US/docs/CSS/text-align

::: browser/devtools/webconsole/AutocompletePopup.jsm
@@ +352,5 @@
> +   *                   that will be auto completed. When this property is
> +   *                   present, |preLabel.length| starting characters will be
> +   *                   removed from label.
> +   *        - count {Number} [Optional] The number to represent the count of
> +   *                autocompleted label.

Shouldn't preLabel be a property on the AutocompletePopup object instance? Like popup.highlightItemPrefix = "foobar", instead of you having to add the same prefix to each item. Thoughts?

Do we want/need fine-grained control for what gets highlighted for every item?

Comment 26

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #25)
> Shouldn't this be more like SelectorsSearch.jsm?

I am not final with the name yet. SelectorsSearch looks better , weird that I was coming up with random names but SelectorsSearch.

> ::: browser/devtools/inspector/inspector.css
> @@ +19,5 @@
> > +  -moz-appearance: none !important;
> > +  background: rgba(0,0,0,0);
> > +  border-width: 0px !important;
> > +  width: 250px;
> > +  max-width: 250px;
> 
> Is max-width needed when width is non-relative?
> 
> Also, why px and not something in ems? I'd like the width to vary based on
> font sizes.

Yes, I think em is more valid here. Max-width is still needed such that overflow-x: hidden comes into play. I will test removing it though.


> @@ +57,5 @@
> > +  padding: 1px 0;
> > +}
> > +
> > +#searchbox-panel-listbox > richlistitem > .autocomplete-count {
> > +  text-align: right;
> 
> LTR versus RTL. Please test with RTL UIs. Add intl.uidirection.en=rtl to
> about:config.
> 
> See text-align start and end:
> https://developer.mozilla.org/en-US/docs/CSS/text-align

I don't think that supporting RTL is an usecase in this case. The selectors are written only LTR, which means that I will have to add the preLabel and Label in that order only. Which leaves the count always to be the lastChild of the item. Thus it will always be right aligned.


> ::: browser/devtools/webconsole/AutocompletePopup.jsm
> @@ +352,5 @@
> > +   *                   that will be auto completed. When this property is
> > +   *                   present, |preLabel.length| starting characters will be
> > +   *                   removed from label.
> > +   *        - count {Number} [Optional] The number to represent the count of
> > +   *                autocompleted label.
> 
> Shouldn't preLabel be a property on the AutocompletePopup object instance?
> Like popup.highlightItemPrefix = "foobar", instead of you having to add the
> same prefix to each item. Thoughts?
> 
> Do we want/need fine-grained control for what gets highlighted for every
> item?

What about something like fuzzy matching ? That case will lag the preLabel totally, thus this kind of freedom is helpful. The change will anyways not make things easier or difficult code wise.

Comment 27

[Girish Sharma [:Optimizer]]

Attachment: patch v0.7

Fixed various issues that I recognized while writing tests.
Tests are awesome :)

Comment 28

[Girish Sharma [:Optimizer]]

Attachment: tests.

tests.

try at https://tbpl.mozilla.org/?tree=Try&rev=efe34dec194b

Comment 29

[Girish Sharma [:Optimizer]]

Last try was busted. Pushed again at https://tbpl.mozilla.org/?tree=Try&rev=ec3bf3d497dd

Comment 30

[Girish Sharma [:Optimizer]]

(In reply to Girish Sharma [:Optimizer] from comment #29)
> Last try was busted. Pushed again at
> https://tbpl.mozilla.org/?tree=Try&rev=ec3bf3d497dd

This oen failed on debug machines as the 70 ms delay was not enough. The machines seemed to respond very late to keypresses and thus all the keypress related tests failed.

Pushed again with 200ms delay and all green : https://tbpl.mozilla.org/?tree=Try&rev=edfcf8641021

Comment 31

[Girish Sharma [:Optimizer]]

Attachment: tests v1.1

Using 200ms delay.

Comment 32

[Paul Rouget [:paul]]

Comment on attachment 712116 [details] [diff] [review]
patch v0.7

Global notes:
. it rocks
. this is a first pass review
. I need more comments. Especially where you use regex
. "Selectors.jsm" and "InspectorPanel.selectors" are too generic names. Maybe you want to split logic and UI code at some point.
. This doesn't support page navigation
. I didn't look at the CSS code yet

>+   * Hooks the searchbar to show result and auto completion suggestions.
>+   */
>+  setupSearchBar: function InspectorPanel_setupSearchBar() {
>+    // Initiate the selectors search object.
>+    let setNodeFunction = function(node) {
>+      this.cancelLayoutChange();

Why is that needed?

>+      this.selection.setNode(node, "selectorsearch");
>+    };
>+    this.searchBox = this.panelDoc.getElementById("inspector-searchbox");
>+    this.selectors = new Selectors(this.browser.contentDocument,
>+                                   this.searchBox,
>+                                   setNodeFunction.bind(this));
>+  },

Move `bind` in `let setNodeFunction() = function{}.bind(this)`;

>diff --git a/browser/devtools/inspector/Selectors.jsm b/browser/devtools/inspector/Selectors.jsm

Could we imagine renaming Selectors.jsm to Selectors-utils.jsm and then move that into /shared/?

>+this.EXPORTED_SYMBOLS = ["Selectors"];

Too generic.

>+/**
>+ * Inspector's searchbox with autocompletion implementation.
>+ *
>+ * @constructor
>+ * @param nsIDOMDocument aContentDocument
>+ *        The content document which inspector is attached to.
>+ * @param nsiInputElement aInputNode
>+ *        The input element to which the panel will be attached and from where
>+ *        search input will be taken.
>+ * @param Function aCallback
>+ *        The method to callback when a seach is available.

s/seach/search/

>+ *        This method is called with the matched node as the first argument.
>+ */
>+this.Selectors = function(aContentDocument, aInputNode, aCallback) {
>+  this.doc = aContentDocument;
>+  this.callback = aCallback;
>+  this.searchBox = aInputNode;
>+  this.panelDoc = this.searchBox.ownerDocument;
>+
>+  // initialize variables.
>+  this._lastSearched = null;
>+  this._lastValidSearch = "";
>+  this._lastToLastValidSearch = null;
>+  this._searchResults = null;
>+  this._searchSuggestions = {};
>+  this._searchIndex = 0;
>+
>+  // bind!
>+  this._showPopup = this._showPopup.bind(this);
>+  this._onHTMLSearch = this._onHTMLSearch.bind(this);
>+  this._onSearchKeypress = this._onSearchKeypress.bind(this);
>+
>+  // Options for the AutocompletePopup.
>+  let options = {
>+    panelId: "inspector-searchbox-panel",
>+    listBoxId: "searchbox-panel-listbox",
>+    fixedWidth: true,
>+    autoSelect: true,
>+    position: "before_start",
>+    onClick: this._onListBoxKeypress.bind(this),
>+    onKeypress: this._onListBoxKeypress.bind(this),

Why didn't you bind this earlier?

>+  };
>+  this.searchPopup = new AutocompletePopup(this.panelDoc, options);
>+
>+  // event listeners.
>+  this.searchBox.addEventListener("command", this._onHTMLSearch, true);
>+  this.searchBox.addEventListener("keypress", this._onSearchKeypress, true);
>+}
>+
>+this.Selectors.prototype = {
>+
>+  // The possible states of the query.
>+  States: {
>+    CLASS: "class",
>+    ID: "id",
>+    TAG: "tag",
>+  },
>+
>+  // The current state of the query.
>+  _state: null,
>+
>+  // The query corresponding to last state computation.
>+  _lastStateCheckAt: null,
>+
>+  /**
>+   * Computes the state of the query. State refers to whether the query
>+   * currently requires a class suggestion, or a tag, or an Id suggestion.

Can you explain how often this getter is called?

>+   *
>+   * @example
>+   *        '#f' requires an Id suggestion, so the state is States.ID
>+   *        'div > .foo' requires class suggestion, so state is States.CLASS
>+   */
>+  get state() {
>+    if (!this.searchBox ||
>+        !this.searchBox.value ||
>+        this.searchBox.value == "") {

No need to test ` == ""`, the previous line does that already.

(!!"" is false)

> [...]

The following need comments:

>+    this._state = null;
>+    let subQuery = "";
>+    for (let i = 1; i <= query.length; i++) {
>+      // Calculate the state.
>+      subQuery = query.slice(0, i);
>+      let [secondLastChar, lastChar] = subQuery.slice(-2);
>+      switch (this._state) {
>+        case null:
>+          lastChar = secondLastChar;
>+        case this.States.TAG:
>+          this._state = lastChar == "."
>+            ? this.States.CLASS
>+            : lastChar == "#"
>+              ? this.States.ID
>+              : this.States.TAG;
>+          break;
>+
>+        case this.States.CLASS:
>+          if (subQuery.match(/[\.]+[^\.]*$/)[0].length > 2) {
>+            this._state = (lastChar == " " || lastChar == ">")
>+            ? this.States.TAG
>+            : lastChar == "#"
>+              ? this.States.ID
>+              : this.States.CLASS;
>+          }
>+          break;
>+
>+        case this.States.ID:
>+          if (subQuery.match(/[#]+[^#]*$/)[0].length > 2) {
>+            this._state = (lastChar == " " || lastChar == ">")
>+            ? this.States.TAG
>+            : lastChar == "."
>+              ? this.States.CLASS
>+              : this.States.ID;
>+          }
>+          break;
>+      }
>+    }
>+    return this._state;
>+  },



>+  _onHTMLSearch: function Selectors__onHTMLSearch() {
>+    let query = this.searchBox.value;
>+    if (query == this._lastSearched) {
>+      return;
>+    }
>+    this._lastSearched = query;
>+    this._searchIndex = 0;
>+
>+    if (query.length == 0) {
>+      this._lastValidSearch = "";
>+      this.searchBox.removeAttribute("filled");
>+      this.searchBox.classList.remove("devtools-no-search-result");
>+      if (this.searchPopup.isOpen) {
>+        this.searchPopup.hidePopup();
>+      }
>+      return;
>+    }
>+
>+    this.searchBox.setAttribute("filled", true);
>+    try {
>+      this._searchResults = this.doc.querySelectorAll(query);
>+    }
>+    catch (ex) {
>+      this._searchResults = [];
>+    }
>+    if (this._searchResults.length > 0) {
>+      this._lastValidSearch = query;
>+      if (query.match(/[ >]$/)) {
>+        this._lastValidSearch += "*";
>+      }

I need a comment about the following:

>+      else if (query.match(/[\s>][\.#a-zA-Z][\.#>\s]*$/)) {
>+        let lastPart = query.match(/[\s>][\.#a-zA-Z][^>\s]*$/)[0];
>+        this._lastValidSearch = query.slice(0, -1 * lastPart.length + 1) + "*";
>+      }
>+
>+      if (!query.slice(-1).match(/[\.# >]/)) {
>+        if (this.searchPopup.isOpen) {
>+          this.searchPopup.hidePopup();
>+        }
>+      }
>+      else {
>+        this.showSuggestions();
>+      }
>+      this.searchBox.classList.remove("devtools-no-search-result");
>+      this.callback(this._searchResults[0]);
>+    }
>+    else {
>+      if (query.match(/[ >]$/)) {
>+        this._lastValidSearch = query + "*";
>+      }
>+      else if (query.match(/[ >][\.#a-zA-Z][\.#>\s]*$/)) {
>+        let lastPart = query.match(/[ >][\.#a-zA-Z][^>\s]*$/)[0];
>+        this._lastValidSearch = query.slice(0, -1 * lastPart.length + 1) + "*";
>+      }
>+      this.searchBox.classList.add("devtools-no-search-result");
>+      this.showSuggestions();
>+    }
>+  },
>+
>+  /**
>+   * Handles keypresses inside the input box.
>+   */
>+  _onSearchKeypress: function Selectors__onSearchKeypress(aEvent) {
>+    let query = this.searchBox.value;
>+    switch(aEvent.keyCode) {
>+      case aEvent.DOM_VK_ENTER:
>+      case aEvent.DOM_VK_RETURN:
>+        if (query == this._lastSearched) {
>+          this._searchIndex = (this._searchIndex + 1) % this._searchResults.length;
>+        }
>+        else {
>+          this._onHTMLSearch();
>+          return;
>+        }
>+        break;
>+
>+      case aEvent.DOM_VK_UP:
>+        if (this.searchPopup.isOpen && this.searchPopup.itemCount > 0) {
>+          this.searchPopup.focus();
>+          if (this.searchPopup.selectedIndex == this.searchPopup.itemCount - 1) {
>+            this.searchPopup.selectedIndex =
>+              Math.max(0, this.searchPopup.itemCount - 2);
>+          }
>+          else {
>+            this.searchPopup.selectedIndex = this.searchPopup.itemCount - 1;
>+          }
>+          this.searchBox.value = this.searchPopup.selectedItem.label;
>+        }
>+        else if (--this._searchIndex < 0) {
>+          this._searchIndex = this._searchResults.length - 1;
>+        }
>+        break;
>+
>+      case aEvent.DOM_VK_DOWN:
>+        if (this.searchPopup.isOpen && this.searchPopup.itemCount > 0) {
>+          this.searchPopup.focus();
>+          this.searchPopup.selectedIndex = 0;
>+          this.searchBox.value = this.searchPopup.selectedItem.label;
>+        }
>+        this._searchIndex = (this._searchIndex + 1) % this._searchResults.length;
>+        break;
>+
>+      case aEvent.DOM_VK_TAB:
>+        if (this.searchPopup.isOpen &&
>+            this.searchPopup.getItemAtIndex(this.searchPopup.itemCount - 1)
>+                .preLabel == query) {
>+          this.searchPopup.selectedIndex = this.searchPopup.itemCount - 1;
>+          this.searchBox.value = this.searchPopup.selectedItem.label;
>+          this._onHTMLSearch();
>+        }
>+        break;
>+
>+      case aEvent.DOM_VK_BACK_SPACE:
>+      case aEvent.DOM_VK_DELETE:
>+        // need to throw away the lastValidSearch.
>+        this._lastToLastValidSearch = null;
>+        this._lastValidSearch = (query.match(/(.*)[\.#][^\.# ]{0,}$/) ||
>+                                 query.match(/(.*\s)[a-zA-Z][^\.# ]{0,}$/) ||
>+                                 ["",""])[1];
>+        return;
>+
>+      default:
>+        return;
>+    }

What the conditions to reach this part of the code (a comment would help).

>+    aEvent.preventDefault();
>+    aEvent.stopPropagation();
>+    if (this._searchResults.length > 0) {
>+      this.callback(this._searchResults[this._searchIndex]);
>+    }
>+  },

>+
>+  
>+  /**
>+   * Populates the suggestions list and show the suggestion popup.
>+   */
>+  _showPopup: function Selectors__showPopup(aList, aFirstPart) {
>+    // Sort alphabetically in increaseing order.
>+    aList = aList.sort(function([a1,a2], [b1,b2]) {
>+      return a1 > b1;
>+    });

If I'm not mistaken, you can just do 'aList = aList.sort()' (alphabetical order by default).

>+    // Sort based on count= in decreasing order.
>+    aList = aList.sort(function([a1,a2], [b1,b2]) {
>+      return a2 < b2;
>+    });
>+
>+    let total = 0;
>+    let query = this.searchBox.value;
>+    let toLowerCase = false;
>+    let items = [];
>+    // In case of tagNames, change the case to small.
>+    if (query.match(/.*[\.#][^\.#]{0,}$/) == null) {
>+      toLowerCase = true;
>+    }
>+    for (let [value, count] of aList) {
>+      // for cases like 'div ' or 'div >'
>+      if (query.match(/[ >]$/)) {
>+        value = query + value;
>+      }
>+      // for cases like 'div #a' or 'div .a' or 'div > d' and likewise
>+      else if (query.match(/[ >][\.#a-zA-Z][^ >\.#]*$/)) {
>+        let lastPart = query.match(/[ >][\.#a-zA-Z][^> \.#]*$/)[0];
>+        value = query.slice(0, -1 * lastPart.length + 1) + value;
>+      }
>+      // for cases like 'div.class' or '#foo.bar' and likewise
>+      else if (query.match(/[a-zA-Z][#\.][^#\. >]*$/)) {
>+        let lastPart = query.match(/[a-zA-Z][#\.][^#\. >]*$/)[0];
>+        value = query.slice(0, -1 * lastPart.length + 1) + value;
>+      }
>+      let item = {
>+        preLabel: query,
>+        label: value,
>+        count: count
>+      };
>+      if (toLowerCase) {
>+        item.label = value.toLowerCase();
>+      }
>+      items.unshift(item);
>+      if (++total > 14) {
>+        break;
>+      }
>+    }
>+    if (total > 0) {
>+      this.searchPopup.setItems(items);
>+      this.searchPopup.openPopup(this.searchBox);
>+    }
>+    else {
>+      this.searchPopup.hidePopup();
>+    }
>+  },
>+
>+  /**
>+   * Suggests classes,ids and tags bsaed on the user input as user types in the

typo: based.

>+   * searchbox.
>+   */
>+  showSuggestions: function Selectors_showSuggestions() {
>+    let query = this.searchBox.value;
>+    if (this._lastValidSearch != "" &&
>+        this._lastToLastValidSearch != this._lastValidSearch) {
>+      this._searchSuggestions = {
>+        ids: new Map(),
>+        classes: new Map(),
>+        tags: new Map(),
>+      };
>+
>+      let nodes = this.doc.querySelectorAll(this._lastValidSearch);

I guess we're sure this won't bail (no need to try{}catch{})?

Comment 33

[Paul Rouget [:paul]]

Comment on attachment 712116 [details] [diff] [review]
patch v0.7

>diff --git a/browser/devtools/webconsole/AutocompletePopup.jsm b/browser/devtools/shared/AutocompletePopup.jsm
>   _updateSize: function AP__updateSize()
>   {
>-    if (!this._panel) {
>-      return;
>-    }
>-    this._list.width = this._panel.clientWidth +
>-                       this._scrollbarWidth;
>+    // We need the timeout to allow the content to reflow. Attempting to
>+    // update the richlistbox size too early does not work.
>+    this._document.defaultView.setTimeout(function() {
>+      if (!this._panel) {
>+        return;
>+      }
>+      this._list.width = this._panel.clientWidth +
>+                         this._scrollbarWidth;
>+      // Height change is required, otherwise the panel is drawn at an offset
>+      // the first time.
>+      this._list.height = this._panel.clientHeight;
>+      // This brings the panel back at right position.
>+      this._list.top = 0;
>+      // Changing panel height might make the selected item out of view, so
>+      // bring it back to view.
>+      this._list.ensureIndexIsVisible(this._list.selectedIndex);
>+    }.bind(this), 5);


I'm not against this setTimeout(..., 5), but I don't understand:
. why we don't use 0 (If you use setTimeout, it's to jump into the next event loop, not to actually wait 5ms). If 5 works but not 0, you'll run into some bug with slow configurations.
. if I'm not mistaken, you can flush the reflow operations by accessing node.style.width (instead of clientWidth)

>   /**
>    * Append an item into the autocomplete list.
>    *
>    * @param object aItem
>-   *        The item you want appended to the list. The object must have a
>-   *        "label" property which is used as the displayed value.
>+   *        The item you want appended to the list.
>+   *        The item object can have the following properties:
>+   *        - label {String} Property which is used as the displayed value.
>+   *        - preLabel {String} [Optional] The String that will be displayed
>+   *                   before the label indicating that this is the already
>+   *                   present text in the input box, and label is the text
>+   *                   that will be auto completed. When this property is
>+   *                   present, |preLabel.length| starting characters will be
>+   *                   removed from label.
>+   *        - count {Number} [Optional] The number to represent the count of
>+   *                autocompleted label.
>    */
>   appendItem: function AP_appendItem(aItem)
>   {
>-    let description = this._document.createElementNS(XUL_NS, "description");
>-    description.textContent = aItem.label;
>-
>     let listItem = this._document.createElementNS(XUL_NS, "richlistitem");
>-    listItem.appendChild(description);
>+    let label = this._document.createElementNS(XUL_NS, "label");
>+    label.setAttribute("value", aItem.label);
>+    label.setAttribute("class", "autocomplete-value");
>+    if (aItem.preLabel) {
>+      let preDesc = this._document.createElementNS(XUL_NS, "label");
>+      preDesc.setAttribute("value", aItem.preLabel);
>+      preDesc.setAttribute("class", "initial-value");
>+      listItem.appendChild(preDesc);
>+      label.setAttribute("value", aItem.label.slice(aItem.preLabel.length));
>+    }

Does that work well in RTL?

Comment 34

[Paul Rouget [:paul]]

Comment on attachment 712252 [details] [diff] [review]
tests v1.1

>+  function checkState(event) {
>+    // Using setTimout as the "command" event fires at delay after keypress
>+    window.setTimeout(function() {
>+      let [key, query] = keyStates[state];
>+
>+      is(searchBox.value, query, "The suggestion at " + state + "th step on " +
>+         "pressing " + key + " key is correct.")
>+      checkStateAndMoveOn(state + 1);
>+    }, 200);
>+  }

Can you listen to the command event?

setTimeout === intermittent oranges

Comment 35

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #34)
> Comment on attachment 712252 [details] [diff] [review]
> tests v1.1
> 
> >+  function checkState(event) {
> >+    // Using setTimout as the "command" event fires at delay after keypress
> >+    window.setTimeout(function() {
> >+      let [key, query] = keyStates[state];
> >+
> >+      is(searchBox.value, query, "The suggestion at " + state + "th step on " +
> >+         "pressing " + key + " key is correct.")
> >+      checkStateAndMoveOn(state + 1);
> >+    }, 200);
> >+  }
> 
> Can you listen to the command event?
> 
> setTimeout === intermittent oranges

Sadly the VK_UP/DOWN do not dispatch command events in textbox. So I have to listen for keypress only.

Also, even after listening for command event for the rest of the keypresses, there were test failures on debug machines. The executeSoon was not enough there, these failures went away with some timeout.

I know timeout is bad :(

Comment 36

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #33)
> Comment on attachment 712116 [details] [diff] [review]
> patch v0.7
> 
> >diff --git a/browser/devtools/webconsole/AutocompletePopup.jsm b/browser/devtools/shared/AutocompletePopup.jsm
> >   _updateSize: function AP__updateSize()
> >   {
> >-    if (!this._panel) {
> >-      return;
> >-    }
> >-    this._list.width = this._panel.clientWidth +
> >-                       this._scrollbarWidth;
> >+    // We need the timeout to allow the content to reflow. Attempting to
> >+    // update the richlistbox size too early does not work.
> >+    this._document.defaultView.setTimeout(function() {
> >+      if (!this._panel) {
> >+        return;
> >+      }
> >+      this._list.width = this._panel.clientWidth +
> >+                         this._scrollbarWidth;
> >+      // Height change is required, otherwise the panel is drawn at an offset
> >+      // the first time.
> >+      this._list.height = this._panel.clientHeight;
> >+      // This brings the panel back at right position.
> >+      this._list.top = 0;
> >+      // Changing panel height might make the selected item out of view, so
> >+      // bring it back to view.
> >+      this._list.ensureIndexIsVisible(this._list.selectedIndex);
> >+    }.bind(this), 5);
> 
> 
> I'm not against this setTimeout(..., 5), but I don't understand:
> . why we don't use 0 (If you use setTimeout, it's to jump into the next
> event loop, not to actually wait 5ms). If 5 works but not 0, you'll run into
> some bug with slow configurations.
> . if I'm not mistaken, you can flush the reflow operations by accessing
> node.style.width (instead of clientWidth)

I actually have not tried style.width, I just moved this part of the code from one place to another. It was already present in the file.

> >   /**
> >    * Append an item into the autocomplete list.
> >    *
> >    * @param object aItem
> >-   *        The item you want appended to the list. The object must have a
> >-   *        "label" property which is used as the displayed value.
> >+   *        The item you want appended to the list.
> >+   *        The item object can have the following properties:
> >+   *        - label {String} Property which is used as the displayed value.
> >+   *        - preLabel {String} [Optional] The String that will be displayed
> >+   *                   before the label indicating that this is the already
> >+   *                   present text in the input box, and label is the text
> >+   *                   that will be auto completed. When this property is
> >+   *                   present, |preLabel.length| starting characters will be
> >+   *                   removed from label.
> >+   *        - count {Number} [Optional] The number to represent the count of
> >+   *                autocompleted label.
> >    */
> >   appendItem: function AP_appendItem(aItem)
> >   {
> >-    let description = this._document.createElementNS(XUL_NS, "description");
> >-    description.textContent = aItem.label;
> >-
> >     let listItem = this._document.createElementNS(XUL_NS, "richlistitem");
> >-    listItem.appendChild(description);
> >+    let label = this._document.createElementNS(XUL_NS, "label");
> >+    label.setAttribute("value", aItem.label);
> >+    label.setAttribute("class", "autocomplete-value");
> >+    if (aItem.preLabel) {
> >+      let preDesc = this._document.createElementNS(XUL_NS, "label");
> >+      preDesc.setAttribute("value", aItem.preLabel);
> >+      preDesc.setAttribute("class", "initial-value");
> >+      listItem.appendChild(preDesc);
> >+      label.setAttribute("value", aItem.label.slice(aItem.preLabel.length));
> >+    }
> 
> Does that work well in RTL?

So the idea here is that the CSS selectors are always LTR, which means that the way our results will appear will always be:

[already existing part of query][suggestion part]

and the same goes for Web console's input like |window.| . That will also always be LTR. Thus there is no need of RTL.

If this is not the case, is there something that I am missing here ?

Comment 37

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #32)
> Comment on attachment 712116 [details] [diff] [review]
> patch v0.7
> 
> Global notes:
> . it rocks
:)
> . this is a first pass review
> . I need more comments. Especially where you use regex

Sure.

> . "Selectors.jsm" and "InspectorPanel.selectors" are too generic names.

I know, I was not able to come up with something good for the time being. Mihai suggested SelectorSearch . What do you think ?

> Maybe you want to split logic and UI code at some point.

Most of the UI code is handled by AutocompeltePopup.jsm, so I think its pretty split up.

> . This doesn't support page navigation

You are right, this needs to be handled from InspectorPanel side. Will fix.

> >+    // Initiate the selectors search object.
> >+    let setNodeFunction = function(node) {
> >+      this.cancelLayoutChange();
> 
> Why is that needed?

you are right, this will eventually get called when while selecting a new node, a "new-node" event will be emitted.

> >+  };
> >+  this.searchPopup = new AutocompletePopup(this.panelDoc, options);
> >+
> >+  // event listeners.
> >+  this.searchBox.addEventListener("command", this._onHTMLSearch, true);
> >+  this.searchBox.addEventListener("keypress", this._onSearchKeypress, true);
> >+}
> >+
> >+this.Selectors.prototype = {
> >+
> >+  // The possible states of the query.
> >+  States: {
> >+    CLASS: "class",
> >+    ID: "id",
> >+    TAG: "tag",
> >+  },
> >+
> >+  // The current state of the query.
> >+  _state: null,
> >+
> >+  // The query corresponding to last state computation.
> >+  _lastStateCheckAt: null,
> >+
> >+  /**
> >+   * Computes the state of the query. State refers to whether the query
> >+   * currently requires a class suggestion, or a tag, or an Id suggestion.
> 
> Can you explain how often this getter is called?

This getter caches the state for query, so the effective times this is called is actually whenever the query in the searchbox changes. But any other option (like regex) to determine the State of the query was failing in some or other normal edge cases.

> >+    if (!this.searchBox ||
> >+        !this.searchBox.value ||
> >+        this.searchBox.value == "") {
> 
> No need to test ` == ""`, the previous line does that already.
> 
> (!!"" is false)

A new thing learned :)

> What the conditions to reach this part of the code (a comment would help).

Assuming you are talking about the part that is below this line, then whenever we have a valid result for the selector, we call the callback method. In this case, this method selects the provided node.

> >+    aEvent.preventDefault();
> >+    aEvent.stopPropagation();
> >+    if (this._searchResults.length > 0) {
> >+      this.callback(this._searchResults[this._searchIndex]);
> >+    }
> >+  },
> 
> >+
> >+  
> >+  /**
> >+   * Populates the suggestions list and show the suggestion popup.
> >+   */
> >+  _showPopup: function Selectors__showPopup(aList, aFirstPart) {
> >+    // Sort alphabetically in increaseing order.
> >+    aList = aList.sort(function([a1,a2], [b1,b2]) {
> >+      return a1 > b1;
> >+    });
> 
> If I'm not mistaken, you can just do 'aList = aList.sort()' (alphabetical
> order by default).

True.

> 
> >+   * searchbox.
> >+   */
> >+  showSuggestions: function Selectors_showSuggestions() {
> >+    let query = this.searchBox.value;
> >+    if (this._lastValidSearch != "" &&
> >+        this._lastToLastValidSearch != this._lastValidSearch) {
> >+      this._searchSuggestions = {
> >+        ids: new Map(),
> >+        classes: new Map(),
> >+        tags: new Map(),
> >+      };
> >+
> >+      let nodes = this.doc.querySelectorAll(this._lastValidSearch);
> 
> I guess we're sure this won't bail (no need to try{}catch{})?

If the DOM tree changes, such that the selector gave result last time, and this time it did not, only then. I will surround it with try catch.



For everything else, I Agree.

Comment 38

[Girish Sharma [:Optimizer]]

Attachment: patch v0.8

Addresses comments.
Renamed to SelectorSearch.jsm
Added inline comments to explain things.
Addressed other comment in the review.

Comment 39

[Girish Sharma [:Optimizer]]

Attachment: tests v1.2

Just a minor renaming of inspector.selectors to inspector.searchSuggestion

Comment 40

[Girish Sharma [:Optimizer]]

Attachment: patch v0.9

Forgot to hg add the new named file.

Comment 41

[Paul Rouget [:paul]]

14 is a magic number. Move it up to the file, as a const:

const MAX_RESULT = 14;

Comment 42

[Paul Rouget [:paul]]

s/  background: transparent;/ background-color: transparent/

s/  -moz-linear-gradient(top/ linear-gradient(to bottom/

s/  background: rgba(0,0,0,0);/ background-color: transparent;/


How does it look in RTL? (I see some text-align:right and margin-right).

Comment 43

[Paul Rouget [:paul]]

This is some solid work. Very good job.

I haven't been through all the regex, because regex. Apparently, it works as expected, but let me be a bit annoying: can I ask you to add a comment _everywhere_ there's a regex?

As soon as we have comments, I'll r+ this patch.

Comment 44

[Girish Sharma [:Optimizer]]

Attachment: patch v1.0

carry forward r+.
Explained every regex.
made margins and alignment respect rtl.
comments addressed.

Comment 45

[Girish Sharma [:Optimizer]]

Comment on attachment 714812 [details] [diff] [review]
patch v1.0

my bad.

Comment 46

[Girish Sharma [:Optimizer]]

Attachment: RTL screenshot

panel in rtl mode.

The margins and everything is ok. The main issue here is that the two parts that should have been like [abc][def] are now [def][abc] which does not make any sense. And css selectors are in English only I guess.

@Paul, what should be done ?

Comment 47

[Girish Sharma [:Optimizer]]

Attachment: patch v1.0 with forced ltr

Since RTL will not work here, I introduced forcing direction support and forced inspector searchbox to always be ltr.

I think this one should be the right approach.

Comment 48

[Mihai Sucan [:msucan]]

Comment on attachment 714819 [details] [diff] [review]
patch v1.0 with forced ltr

Review of attachment 714819 [details] [diff] [review]:
-----------------------------------------------------------------

Some nits below.

::: browser/devtools/webconsole/AutocompletePopup.jsm
@@ +36,5 @@
> +function AutocompletePopup(aDocument,
> +                           aOptions = {fixedWidth: false,
> +                                       autoSelect: false,
> +                                       position: "after_start",
> +                                       panelId: "devtools_autoCompletePopup"})

Please make this aOptions = {}, then in the constructor body put the defaults.

If I provide an empty aOptions argument I would expect that |position| and |panelId| (and the other options) take their default values. However, with the approach of this patch, they don't take any value.

@@ +115,5 @@
>  
> +  // Event handlers.
> +  onSelect: null,
> +  onClick: null,
> +  onKeypress: null,

add direction: null and the other new properties you add in the constructor. (this avoids warnings of undefined properties in debug builds, or when js strict warnings are enabled)

Comment 49

[Girish Sharma [:Optimizer]]

Attachment: patch v1.1

Added the '+' combinator support.

Comment 50

[Girish Sharma [:Optimizer]]

Attachment: tests v1.3

Added test to test descendent selectors involving combinators.

Pushed to try again : https://tbpl.mozilla.org/?tree=Try&rev=22e6dbd651a9

Comment 51

[Girish Sharma [:Optimizer]]

Attachment: Combined patch

Rebased and qfolded the two patches into one.

Comment 52

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/c133a03833b0

Comment 53

[Paul Rouget [:paul]]

A lot of oranges there.

Not sure if they are all cause by this patch or not.

At least:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20066870&tree=Fx-Team

Comment 54

[Girish Sharma [:Optimizer]]

The win debug ones are. But I cannot think of a better solution to fix them other than increasing the timeout :( .

Comment 55

[Paul Rouget [:paul]]

backed out: https://hg.mozilla.org/integration/fx-team/rev/eca9e6636021

Comment 56

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #54)
> The win debug ones are. But I cannot think of a better solution to fix them
> other than increasing the timeout :( .

Sometimes, what we do is this:

function checkSomething(callback) {
  if (something) {
    executeSoon(callback);
  } else {
    setTimeout(function() {checkSomething(callback)}, 500);
  }
}

This is not optimal (if it fails, you get a timeout, not a failure), but it works (feature is tested without any risk of orange).

Comment 57

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #56)
> (In reply to Girish Sharma [:Optimizer] from comment #54)
> > The win debug ones are. But I cannot think of a better solution to fix them
> > other than increasing the timeout :( .
> 
> Sometimes, what we do is this:
> 
> function checkSomething(callback) {
>   if (something) {
>     executeSoon(callback);
>   } else {
>     setTimeout(function() {checkSomething(callback)}, 500);
>   }
> }
> 
> This is not optimal (if it fails, you get a timeout, not a failure), but it
> works (feature is tested without any risk of orange).

I got the real culprit. Its due to the fact that the machine is soo slow that when the input is "div" and I press "." , it has to open the suggestions panel. Now after 200ms of pressing ".", I press "UP" key and even by that time, the panel has not updated its |state|, so the state is still "closed" even though the panel is visible, which causes the logic in the actual code to do nothing (which is correct, as if panel is not open, do not do anything). For tests purposes, I can wait for the "popupshown" event of the panel, but I can reproduce the scenario using hands too. Just quickly pressing "." followed by UP navigation key and see that the focus has not shifted to the suggestions panel. Instead, the cursor has moved to the beginning of the text "div.".

Comment 58

[Girish Sharma [:Optimizer]]

Attachment: tests pass.

Added the approach mentioned by Paul along with the waiting for panel to close and open at required steps.

Tests seem to pass : https://tbpl.mozilla.org/?tree=Try&rev=a51eb151270a

Lets see when it lands in fx-team.

Comment 59

[Girish Sharma [:Optimizer]]

Attachment: rebased.

Rebased on latest fx-team.

The complete series pushed to try again at : https://tbpl.mozilla.org/?tree=Try&rev=cd95ea1c1624

Comment 60

[Girish Sharma [:Optimizer]]

Attachment: rebased again

Rebased again based on latest fx-team.

Carry forward r+ from paul

Comment 61

[Girish Sharma [:Optimizer]]

Attachment: rebased once again

rebased again to fx-team

Comment 62

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/2e3aba92a713

Comment 63

[Paul Rouget [:paul]]

https://hg.mozilla.org/mozilla-central/rev/2e3aba92a713

Comment 64

[Ioana (away)]

Verified as fixed on Windows 7 64bit, Mac OSX 10.7 and Ubuntu 13.04 32bit, using Firefox 22b2 (20130521223249). Autocomplete works like that for the web console, with the difference that results are mainly listed by the number of appearances, and only secondary by their alphabetical order (this might be how the web console autocomplete works too, but there you only have suggestions, so they don't have appearances numbers).