650804 - Implement a search mechanism for the highlighter

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

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

Need a way to search for and highlight multiple elements in the highlighter, probably using a querySelector for the search mechanism.

Comment 1

[Kevin Dangoor]

We may just want a GCLI command to start with and add UI when we start adding in search throughout. (Or, we could add the UI here to begin with and augment with global search later on.)

Comment 4

[Paul Rouget [:paul]]

Bug triage, filter on PINKISBEAUTIFUL

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: WIP 0.01

First iteration. Just adds a searchbox, a key (Accel F) and selects the first node that matches the query searched.

Next step: Form a list of all the nodes that matched and show it. Almost like the Debugger's global Search Results.

Next to next step: Handle Tab/Enter/Keyboard navigation in the search box to highlight next/previous matches.

Finally: Tests.

Comment 6

[Paul Rouget [:paul]]

> Next step: Form a list of all the nodes that matched and show it. Almost like the Debugger's global Search Results.

I don't think you should support the list of matches for now. Because it will increase significantly the complexity of this patch as there are no ways to show/select/highlight multiple nodes yet (bug 653545). I think you should focus on just showing one node (basically, select the first result), and maybe have prev/next buttons to cycle through the results.

Comment 7

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #6)
> > Next step: Form a list of all the nodes that matched and show it. Almost like the Debugger's global Search Results.
> 
> I don't think you should support the list of matches for now. Because it
> will increase significantly the complexity of this patch as there are no
> ways to show/select/highlight multiple nodes yet (bug 653545). I think you
> should focus on just showing one node (basically, select the first result),
> and maybe have prev/next buttons to cycle through the results.

I am not aiming for multiple nodes selected at a time. Just showing the results, like debugger's global search lists out the matching lines.

The use case for this would be that user would directly come to know of all the HTML elements that his style rule is applying to. This will be handy to debug styles.

Comment 8

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #7)
> I am not aiming for multiple nodes selected at a time. Just showing the
> results, like debugger's global search lists out the matching lines.

What would you show exactly? If I type "div" in the search box, what exactly would you show in the list?

Comment 9

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #8)
> (In reply to Girish Sharma [:Optimizer] from comment #7)
> > I am not aiming for multiple nodes selected at a time. Just showing the
> > results, like debugger's global search lists out the matching lines.
> 
> What would you show exactly? If I type "div" in the search box, what exactly
> would you show in the list?

May be the lines those divs belong to as seen in the Mark up panel. haven't figured that out. Will do tomorrow.

Comment 10

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #9)
> (In reply to Paul Rouget [:paul] from comment #8)
> > (In reply to Girish Sharma [:Optimizer] from comment #7)
> > > I am not aiming for multiple nodes selected at a time. Just showing the
> > > results, like debugger's global search lists out the matching lines.
> > 
> > What would you show exactly? If I type "div" in the search box, what exactly
> > would you show in the list?
> 
> May be the lines those divs belong to as seen in the Mark up panel. haven't
> figured that out. Will do tomorrow.

like:
  
  <div id="foo" class="bar">
  <div class="foobar">
  <div randomattribute="">

?

I think you'd expect selectors because you're writing a selector.

  div#foo.bar
  div.foobar
  div

But that might be a bit tricky as you have to build a selector for all the matching nodes.

I'm not sure this is a good idea.

Comment 11

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #10)
> > May be the lines those divs belong to as seen in the Mark up panel. haven't
> > figured that out. Will do tomorrow.
> 
> like:
>   
>   <div id="foo" class="bar">
>   <div class="foobar">
>   <div randomattribute="">
> 
> ?

Almost exactly.

> I think you'd expect selectors because you're writing a selector.
> 
>   div#foo.bar
>   div.foobar
>   div
> 
> But that might be a bit tricky as you have to build a selector for all the
> matching nodes.
> 
> I'm not sure this is a good idea.

It would be tricky, but it would not be the search result then. It would be more like search suggestion, isn't it ? just like Google provides as you type your query in the search box. While this will lead him to a correct query, as in we will tell him whether a query is possible or not using his input text, but it will not tell him in one go, all the matching html elements of his current query.

Both ways he is getting some useful information, and not one some other equally useful information. I was inclined towards the first approach earlier, but now that you've mentioned the second one, I am confused.

And its true that the second one will be computationally more heavy, if not coding wise difficult.

Comment 12

[Paul Rouget [:paul]]

So we need to decide what would the popup do: completion or showing result?
I'd prefer completion.

This is what I would suggest: we start simple. Don't implement the popup yet, and we will then experiment different approaches.

I'm also interested to hear you about how we could let the user jump to the next/previous match in term of UI. And should we show the number of matches?

Comment 13

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #12)
> So we need to decide what would the popup do: completion or showing result?
> I'd prefer completion.

Hey, speaking of popups, we can do both:

1) As you type in the box, a popup shows completion, that appears above the search box.
2) When the user presses Enter, or even as he types, the result list appears below in a UI similar to debugger's global results UI.

Showing just the completion will not be helpful as he would have no idea what that query selector will lead to. Suppose he is writing 'div.foo' and we show completion results :

'div.foobar'
'div.foo#bar'
'div.foo.bar'

Now unless he has a good knowledge on the DOM of the page, he would not really understand which one he needs to see result of.

> This is what I would suggest: we start simple. Don't implement the popup
> yet, and we will then experiment different approaches.

You mean forget popup and/or results UI in this bug ?

> I'm also interested to hear you about how we could let the user jump to the
> next/previous match in term of UI. And should we show the number of matches?

Without any popup or results UI there is no extra space to show the total number of matches. About jumping to next/prev match, he can use Enter/Up/Down to navigate between matches.

Comment 14

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

My 2c:

(In reply to Girish Sharma [:Optimizer] from comment #13)
> (In reply to Paul Rouget [:paul] from comment #12)
> > So we need to decide what would the popup do: completion or showing result?
> > I'd prefer completion.
> 
> Hey, speaking of popups, we can do both:
> 
> 1) As you type in the box, a popup shows completion, that appears above the
> search box.
> 2) When the user presses Enter, or even as he types, the result list appears
> below in a UI similar to debugger's global results UI.
> 

I really don't think having any kind of UI for this functionality is a good idea at this point. There's no guarantee that the extra displayed information is in any way useful. Suppose you're searching for "div", you may end up with search results containing just
<div>
<div>
...
<div>
There's no guarantee of a uniform distribution of ids, classes (or even attributes for extra differentiation) in a webpage's DOM for a list of results to be useful. Having both a popup and extra results afterwards is even more confusing.

> Showing just the completion will not be helpful as he would have no idea
> what that query selector will lead to. Suppose he is writing 'div.foo' and
> we show completion results :
> 
> 'div.foobar'
> 'div.foo#bar'
> 'div.foo.bar'
> 
> Now unless he has a good knowledge on the DOM of the page, he would not
> really understand which one he needs to see result of.
> 

Exactly. There's no need for extra UI. The amount of new (or helpful) information given by such a popup or list is rather low.

I assume that by "completion", Paul means no popup, but a gray textual continuation of the first match, similar to what the webconsole, gcli, zsh, fish etc. does.

> > This is what I would suggest: we start simple. Don't implement the popup
> > yet, and we will then experiment different approaches.
> 
> You mean forget popup and/or results UI in this bug ?
> 

In the debugger, pressing UP goes to a previous match, DOWN/RETURN/ENTER goes to the next. We can keep the same shortcuts for consistency.

> > I'm also interested to hear you about how we could let the user jump to the
> > next/previous match in term of UI. And should we show the number of matches?
> 
> Without any popup or results UI there is no extra space to show the total
> number of matches. About jumping to next/prev match, he can use
> Enter/Up/Down to navigate between matches.

An exact number of matches is probably not relevant.

IMHO, the easiest way of using this search mechanism would be typing a selector (or a part of it), then pressing UP/DOWN to jump to the next/previous match. If there are no matches, some sort of feedback would suffice ("No matches" in red somewhere, a sound "blip" etc.).

Comment 15

[Girish Sharma [:Optimizer]]

Attachment: WIP 0.1

* Adds Result navigation via ENTER/RETURN/UP/DOWN keys.
* Provides a feedback when no match. It makes the searchbox reddish, with red background and border. I tested on Windows 7 and it looked good. Please provide feedback on the color values and look on Mac and Linux (may be a screenshot).

I am not sure about not showing any UI. Atleast one of the two should be shown (i.e. either results or completion suggestion).


Regarding what Victor said, that completion would be just grey text (like gcli) representing the first result's selector:

How is that useful ? suppose he is looking for 'div.foo.bar' and currently he has written only 'div.fo'. Now lets say that for 'div.fo' there are 10 total node matches and 3 unique selectors :
'div.foo'
'div.foo.bar'
'div.foo.baz'

Now since he is at 'div.fo' , he would already have the first element with 'div.foo' selector selected in the breadcrumbs, markup view, and infobar. So how will displaying 'o' in grey text help him ?

Since this search is search as you type, and not search when you press Enter/Return, the grey auto-completion suggestion would not be of that use.

But what if the user got a popup displaying all the unique three selectors in the popup. That way he can directly selected the second one, that he actually wanted. Now this case has one more problem, until the popup is open, pressing UP/DOWN will navigate the popup's suggestions and select the first matching element corresponding to that suggestion. Only when he presses Enter/Return, his UP/DOWN/Enter/Return keys will navigate the results.

Comment 16

[Girish Sharma [:Optimizer]]

BTW, how would we even get the selector from the element ? I mean like the user entered 'div.fo' . Now how to find all or even one of the completion suggestion to 'div.fo' ?

Comment 17

[Paul Rouget [:paul]]

I agree with what Victor said.

> You mean forget popup and/or results UI in this bug ?

Yes. I would very much like to keep the UI as clean and simple as possible. Not extra popup, not result list. We could complete on ids and classes easily I guess, but I'd prefer this to happen in a different bug.

For cycling through the result, keybindings are fine. For showing "no match", a red border might be fine too.

Let's implement what we are sure about, and then experiment new bit of UIs in follow-up bugs.

Comment 18

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #17)
> I agree with what Victor said.
> 
> > You mean forget popup and/or results UI in this bug ?
> 
> Yes. I would very much like to keep the UI as clean and simple as possible.
> Not extra popup, not result list. We could complete on ids and classes
> easily I guess, but I'd prefer this to happen in a different bug.
> 
> For cycling through the result, keybindings are fine. For showing "no
> match", a red border might be fine too.
> 
> Let's implement what we are sure about, and then experiment new bit of UIs
> in follow-up bugs.

Please see the latest patch.

Comment 19

[Girish Sharma [:Optimizer]]

Attachment: patch with audio feedback for noo results

This is just as WIP 0.1 but with an audio feedback to tell you that there are no matching results.

Comment 20

[Paul Rouget [:paul]]

Comment on attachment 701565 [details] [diff] [review]
patch with audio feedback for noo results

OMG THIS IS AWESOME!

Comment 21

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #20)
> Comment on attachment 701565 [details] [diff] [review]
> patch with audio feedback for noo results
> 
> OMG THIS IS AWESOME!

Who asked for your review ? :P

Comment 22

[Paul Rouget [:paul]]

s/reasult/result/

Comment 23

[Paul Rouget [:paul]]

> Search HTML

I don't really like this string, anything better?

Comment 24

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #23)
> > Search HTML
> 
> I don't really like this string, anything better?

Even I don't like it. Its not final. But do we have any other options ?

Comment 25

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

(In reply to Paul Rouget [:paul] from comment #20)

Can the searchbox be placed on the right? Webconsole has it on the right, and after bug 802546 debugger will also have it on the right.

Comment 26

[Girish Sharma [:Optimizer]]

Attachment: WIP 0.2

* Shows only the search icon when not focused.
* Search box moved to right end.
* I think its ready for feedback.

Comment 27

[Girish Sharma [:Optimizer]]

Attachment: patch v1.0

I think this is ready for review. Tests coming in a separate patch.

Comment 28

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

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

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

The search functionality behaves as expected, modulo the following bug.

1. Open inspector on a page (e.g. http://news.ycombinator.com/) and lock the inspector on any node.
2. Type a matching selector in the searchbox (e.g. "body").
3. Node is selected  (good).
4. Press backspace.
5. Searchbox is red (good).
6. Type the last deleted letter (e.g. "y" from body).
No matches are found (bad).
Also:
7. Press ENTER.
Breadcrumbs disappear (!).

I'll r+ this with the above bug fixed, a test and the comments below addressed.

Paul's idea regarding the expandable searchbox works exceptionally well, I think I'm going to steal, since the debugger's going to get a breadcrumbs bar soon.

::: browser/devtools/inspector/InspectorPanel.jsm
@@ +65,5 @@
> +    this.searchBox = this.panelDoc.getElementById("inspector-searchbox");
> +    this._lastSearched = null;
> +    this._searchResults = null;
> +    this._searchIndex = 0;
> +    this._focusSearchBox = this._focusSearchBox.bind(this);

No need to bind |this| onto _focusSearchBox.

Even more, I only see it used once, in inspector.xul. I'm not a big fan of functions only used once, especially if they're one-liners.

@@ +397,5 @@
>      return Promise.resolve(null);
>    },
>  
> +  /*
> +   * Returns the matchign nodes based on the query.

Typo: matchign.

This is also a one-liner function, called only once. Inline it.

@@ +408,5 @@
> +   * Focus the Searchbox.
> +   */
> +  _focusSearchBox: function InspectorPanel__focusSearchBox() {
> +    this.searchBox.focus();
> +  },

One-liner function, called only once. Inline it.

@@ +411,5 @@
> +    this.searchBox.focus();
> +  },
> +
> +  /*
> +   * Search for the search box value as a query selector.

It seems the function documentation in this file follows the /** * */ convention. Add another * on the first comment line here, and everywhere else.

"Search for the search box value as a query selector" sounds misleading to me (and is a bit hard to parse). How about simply "The command callback for the HTML search box. This function is automatically invoked as the user is typing".

@@ +416,5 @@
> +   */
> +  _onHTMLSearch: function InspectorPanel__onHTMLSearch() {
> +    let query = this.searchBox.value;
> +    if (query.length == 0) {
> +      this.searchBox.setAttribute("filled", false);

Use removeAttribute here and only match [filled] in the css, not [filled=true].

@@ +418,5 @@
> +    let query = this.searchBox.value;
> +    if (query.length == 0) {
> +      this.searchBox.setAttribute("filled", false);
> +      this.searchBox.classList.remove("devtools-no-search-result");
> +      return;

You should move this check in onSearchKeypress.

Since this callback is invoked after a certain delay (because the textbox's type="search" attribute), then after clearing the search query (via backspace for example), then it takes a second to revert the "devtools-no-search-result" class if it was already present.

@@ +427,5 @@
> +    }
> +
> +    this.searchBox.setAttribute("filled", true);
> +    this._searchResults = this._getMatchingNodes(query);
> +    if (this._searchResults != null && this._searchResults.length > 0) {

querySelectorAll can't return null, only an empty NodeList.

@@ +434,5 @@
> +      this.searchBox.classList.remove("devtools-no-search-result");
> +      this.cancelLayoutChange();
> +      this.selection.setNode(this._searchResults[0]);
> +    }
> +    else {

Nit: it's usually } else { throughout the file.

@@ +446,5 @@
> +  _onSearchKeypress: function InspectorPanel__onSearchKeypress(aEvent) {
> +    let query = this.searchBox.value;
> +    switch(aEvent.keyCode) {
> +      case aEvent.DOM_VK_ENTER:
> +      case aEvent.DOM_VK_RETURN:

Why are you special casing ENTER and RETURN? Searching should happen automatically because you're listening to the "command" event. No need to rush things here.

@@ +463,5 @@
> +        }
> +        break;
> +
> +      case aEvent.DOM_VK_DOWN:
> +        this._searchIndex = (this._searchIndex + 1)%this._searchResults.length;

Nit: add space around % operator.

::: browser/devtools/inspector/inspector.xul
@@ +17,5 @@
>            src="chrome://global/content/viewSourceUtils.js"/>
>  
> +  <commandset>
> +    <command id="nodeSearchCommand"
> +      oncommand="inspector._focusSearchBox()"/>

oncommand="inspector.searchBox.focus();"

@@ +24,5 @@
> +  <keyset>
> +    <key id="nodeSearchKey"
> +      key="&inspectorSearchHTML.key;"
> +      modifiers="accel"
> +      command="nodeSearchCommand"/>

Where are the inspector keys defined? I'm not entirely confident if this should be here or somewhere else, please ask Paul.

::: browser/themes/gnomestripe/devtools/common.css
@@ +170,5 @@
>  
> +.devtools-no-search-result {
> +  transition-property: box-shadow, border-color, background-image;
> +  transition-duration: 150ms;
> +  transition-timing-function: ease;

The transition-timing-function's default value is already "ease", no need to add this here.

@@ +173,5 @@
> +  transition-duration: 150ms;
> +  transition-timing-function: ease;
> +  box-shadow: inset 0 0 0 1px hsla(0,68%,6%,.35);
> +  border-color: hsl(10,70%,40%) hsl(10,75%,37%) hsl(10,80%,35%) !important;
> +  background-image: url(magnifying-glass.png), -moz-linear-gradient(hsla(1,16%,76%,.45), hsla(1,16%,76%,.75));

The color looks fine to me, but I'm generally bad at this stuff, so please ask Paul if it's in line with the toolbox theme :)

::: browser/themes/gnomestripe/devtools/inspector.css
@@ +225,5 @@
> +
> +#inspector-searchbox:not([focused]):not([filled=true]) {
> +  max-width: 20px !important;
> +  -moz-padding-end: 6px;
> +  -moz-padding-start: 22px;

How did you calculate these paddings? I almost feel like there are too many pixels on the right, but I may be wrong.

@@ +229,5 @@
> +  -moz-padding-start: 22px;
> +  background-position: 8px center, top left, top left;
> +}
> +
> +#inspector-searchbox[focused], #inspector-searchbox[filled=true] {

Nit: newline after ,

::: browser/themes/winstripe/devtools/inspector.css
@@ +69,5 @@
>    overflow: hidden;
>    min-height: 25px;
>    margin: 0 -11px 0 0;
>    padding: 0 9px;
> +  }

Unnecessarily added whitespace.

Comment 29

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #28)
> Comment on attachment 701921 [details] [diff] [review]
> patch v1.0
> 
> Review of attachment 701921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The search functionality behaves as expected, modulo the following bug.
> 
> 1. Open inspector on a page (e.g. http://news.ycombinator.com/) and lock the
> inspector on any node.
> 2. Type a matching selector in the searchbox (e.g. "body").
> 3. Node is selected  (good).
> 4. Press backspace.
> 5. Searchbox is red (good).
> 6. Type the last deleted letter (e.g. "y" from body).
> No matches are found (bad).
> Also:
> 7. Press ENTER.
> Breadcrumbs disappear (!).

This is mainly because the command is fired after 500 ms, and by that time, you have made two changes, due to which some race condition happens. I have reduced the timeout to 50ms. Cannot see this now.

> @@ +416,5 @@
> > +   */
> > +  _onHTMLSearch: function InspectorPanel__onHTMLSearch() {
> > +    let query = this.searchBox.value;
> > +    if (query.length == 0) {
> > +      this.searchBox.setAttribute("filled", false);
> 
> Use removeAttribute here and only match [filled] in the css, not
> [filled=true].

removeAttribute will require an extra check of has attribute, as there are some cases when I set "filled" to false when its already false.

> @@ +418,5 @@
> > +    let query = this.searchBox.value;
> > +    if (query.length == 0) {
> > +      this.searchBox.setAttribute("filled", false);
> > +      this.searchBox.classList.remove("devtools-no-search-result");
> > +      return;
> 
> You should move this check in onSearchKeypress.
> 
> Since this callback is invoked after a certain delay (because the textbox's
> type="search" attribute), then after clearing the search query (via
> backspace for example), then it takes a second to revert the
> "devtools-no-search-result" class if it was already present.

when a keypress event is occurred, the textbox is not empty yet. So there is no way to tell whether it will get empty or not. Thus adding the check of 

if (this.searchBox.value.length == 0) (remove class)

will not work out. I have reduced the command firing timeout for the textbox, so the delay should reduce now.

> @@ +446,5 @@
> > +  _onSearchKeypress: function InspectorPanel__onSearchKeypress(aEvent) {
> > +    let query = this.searchBox.value;
> > +    switch(aEvent.keyCode) {
> > +      case aEvent.DOM_VK_ENTER:
> > +      case aEvent.DOM_VK_RETURN:
> 
> Why are you special casing ENTER and RETURN? Searching should happen
> automatically because you're listening to the "command" event. No need to
> rush things here.

Cannot do that. I do not want to move the next/prev logic to the onHTMLSearch function as what can happen then is :
'div' -> first div gets selected.
then quickly 'di' -> 'div' -> second div get selected.
quickly in the sense that not allowing a command event to fire at the 'di' stage.

Also, when user presses Enter, he expects search when I press enter behavior, as to search now. Not after some timeout.

> @@ +24,5 @@
> > +  <keyset>
> > +    <key id="nodeSearchKey"
> > +      key="&inspectorSearchHTML.key;"
> > +      modifiers="accel"
> > +      command="nodeSearchCommand"/>
> 
> Where are the inspector keys defined? I'm not entirely confident if this
> should be here or somewhere else, please ask Paul.

There are no keys used in the inspector tool as of now. So this is the first one.

> ::: browser/themes/gnomestripe/devtools/inspector.css
> @@ +225,5 @@
> > +
> > +#inspector-searchbox:not([focused]):not([filled=true]) {
> > +  max-width: 20px !important;
> > +  -moz-padding-end: 6px;
> > +  -moz-padding-start: 22px;
> 
> How did you calculate these paddings? I almost feel like there are too many
> pixels on the right, but I may be wrong.

If you can send me a screenshot, I will be more clear on how much is required :)


Rest all comments fixed.

@Paul, what do you think of the colors (the red background and border) ?

Comment 30

[Girish Sharma [:Optimizer]]

Attachment: patch v2.0 with tests

Patch with comments addressed.

Comment 31

[Girish Sharma [:Optimizer]]

Attachment: patch v2.1 with tests

Addressed comments. Added Tests.

Try push at : https://tbpl.mozilla.org/?tree=Try&rev=fc0fcd0039ac

(no idea why that first changeset is coming to my push. Whatever I do, that comes.)

Comment 32

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

(In reply to Girish Sharma [:Optimizer] from comment #29)

> > Breadcrumbs disappear (!).
> 
> This is mainly because the command is fired after 500 ms, and by that time,
> you have made two changes, due to which some race condition happens. I have
> reduced the timeout to 50ms. Cannot see this now.
> 

Press return/enter while the search box is red (i.e. no matches are found). The breadcrumbs disappear.

Also, the devtools-no-search-result class should be removed when the searchbox is empty. Not searching doesn't mean "no matches found".

> > Use removeAttribute here and only match [filled] in the css, not
> > [filled=true].
> 
> removeAttribute will require an extra check of has attribute, as there are
> some cases when I set "filled" to false when its already false.
> 

Nope, you can safely removeAttribute even if the attribute is not present. Please use it.

> > Why are you special casing ENTER and RETURN? Searching should happen
> > automatically because you're listening to the "command" event. No need to
> > rush things here.
> 
> I do not want to move the next/prev logic to the
> onHTMLSearch function as what can happen then is :
> 'div' -> first div gets selected.
> then quickly 'di' -> 'div' -> second div get selected.
> quickly in the sense that not allowing a command event to fire at the 'di'
> stage.

I didn't say you should do that :)

> 
> Also, when user presses Enter, he expects search when I press enter
> behavior, as to search now. Not after some timeout.
> 

Since the timeout is so much smaller now, then this is almost redundant. Anyway, not that important.

Comment 33

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

Comment on attachment 702992 [details] [diff] [review]
patch v2.1 with tests

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

This is looking pretty good, but r- mainly because the breadcrumbs disappearing when pressing ENTER while no matches are found. Please see the comments above and below as well.

::: browser/devtools/inspector/test/browser_inspector_bug_650804_search.js
@@ +58,5 @@
> +    inspector = aInspector;
> +    inspector.selection.setNode($("b1"));
> +    focusSearchBoxUsingShortcut(inspector.panelWin);
> +    inspector.panelWin.document.getElementById("inspector-searchbox")
> +             .setAttribute("timeout", "20");

Why are you changing the timeout here? Leave it as defined in inspector.xul.

@@ +62,5 @@
> +             .setAttribute("timeout", "20");
> +
> +    executeWithDelay(function() {
> +      checkStateAndMoveOn(0);
> +    }, 200);

This is pretty much a prettified way of setting a timeout, I wouldn't trust this at all. Can't you just add event listeners to the searchbox itself?

@@ +72,5 @@
> +      return;
> +    }
> +
> +    let key = keypressStatesVsSelectedIds[index][0];
> +    let id = keypressStatesVsSelectedIds[index][1];

Uber-nit: let [key, id] = keypressStatesVsSelectedIds[index];

::: browser/devtools/inspector/test/head.js
@@ +113,5 @@
>  
>    EventUtils.synthesizeKey(name, modifiers);
>  }
> +
> +function focusSearchBoxUsingShortcut(panelWin) {

Why have this in head js? You're using it in only one test. If it'll ever be necessary to reuse it, it can be moved then.

@@ +123,5 @@
> +
> +  let name = null;
> +
> +  if (key.getAttribute("keycode"))
> +    name = key.getAttribute("keycode");

Nit: use curly braces even for single line statements.

@@ +140,5 @@
> +
> +  EventUtils.synthesizeKey(name, modifiers);
> +}
> +
> +function executeWithDelay(aCallback, aDelay) {

I'm not comfortable with this. Try using event listeners.

Comment 34

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #33)
> Comment on attachment 702992 [details] [diff] [review]
> patch v2.1 with tests
> 
> Review of attachment 702992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking pretty good, but r- mainly because the breadcrumbs
> disappearing when pressing ENTER while no matches are found. Please see the
> comments above and below as well.

Agreed to the above comments.

> ::: browser/devtools/inspector/test/browser_inspector_bug_650804_search.js
> @@ +58,5 @@
> > +    inspector = aInspector;
> > +    inspector.selection.setNode($("b1"));
> > +    focusSearchBoxUsingShortcut(inspector.panelWin);
> > +    inspector.panelWin.document.getElementById("inspector-searchbox")
> > +             .setAttribute("timeout", "20");
> 
> Why are you changing the timeout here? Leave it as defined in inspector.xul.

To further reduce the total tiem taken by the test. I have to simulate 25 keypresses, so if timeout is 50ms, it will at minimum take 2.5 seconds to finish the test. While that should not be an issue. In my local setup, I have seen cases when the test will simply stop at 20th or 18th step sometimes.


> @@ +62,5 @@
> > +             .setAttribute("timeout", "20");
> > +
> > +    executeWithDelay(function() {
> > +      checkStateAndMoveOn(0);
> > +    }, 200);
> 
> This is pretty much a prettified way of setting a timeout, I wouldn't trust
> this at all. Can't you just add event listeners to the searchbox itself?

Wouldn't trust as in ? Anyways, a focus listener should do here :)

> @@ +72,5 @@
> > +      return;
> > +    }
> > +
> > +    let key = keypressStatesVsSelectedIds[index][0];
> > +    let id = keypressStatesVsSelectedIds[index][1];
> 
> Uber-nit: let [key, id] = keypressStatesVsSelectedIds[index];

\o/

> ::: browser/devtools/inspector/test/head.js
> @@ +113,5 @@
> >  
> >    EventUtils.synthesizeKey(name, modifiers);
> >  }
> > +
> > +function focusSearchBoxUsingShortcut(panelWin) {
> 
> Why have this in head js? You're using it in only one test. If it'll ever be
> necessary to reuse it, it can be moved then.

If there are any plans to further improve this search behavior (which personally I have), then this function will be required.

> @@ +123,5 @@
> > +
> > +  let name = null;
> > +
> > +  if (key.getAttribute("keycode"))
> > +    name = key.getAttribute("keycode");
> 
> Nit: use curly braces even for single line statements.

I just copy pasted this one from above in the file :). Will do.

> @@ +140,5 @@
> > +
> > +  EventUtils.synthesizeKey(name, modifiers);
> > +}
> > +
> > +function executeWithDelay(aCallback, aDelay) {
> 
> I'm not comfortable with this. Try using event listeners.

Event listener will increase the complexity of checking each state, as sometimes a command is fired and sometimes a keypress. Having both the listeners and then toggling between them at the correct place might not turn out well.

Comment 35

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

(In reply to Girish Sharma [:Optimizer] from comment #34)

> To further reduce the total tiem taken by the test. I have to simulate 25
> keypresses, so if timeout is 50ms, it will at minimum take 2.5 seconds to
> finish the test. While that should not be an issue. In my local setup, I
> have seen cases when the test will simply stop at 20th or 18th step
> sometimes.
> 

That's a clear indication of a race condition and executeWithDelay (timeouts in general) misbehaving. If the test itself would timeout, then it'll self close, and you could avoid that by a requestLongerTimeout call. But that's not the case here.

> 
> > @@ +62,5 @@
> > > +             .setAttribute("timeout", "20");
> > > +
> > > +    executeWithDelay(function() {
> > > +      checkStateAndMoveOn(0);
> > > +    }, 200);
> > 
> > This is pretty much a prettified way of setting a timeout, I wouldn't trust
> > this at all. Can't you just add event listeners to the searchbox itself?
> 
> Wouldn't trust as in ? Anyways, a focus listener should do here :)
> 

See above comment :) Timeouts are generally evil.

Comment 36

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #35)
> (In reply to Girish Sharma [:Optimizer] from comment #34)
> 
> > To further reduce the total tiem taken by the test. I have to simulate 25
> > keypresses, so if timeout is 50ms, it will at minimum take 2.5 seconds to
> > finish the test. While that should not be an issue. In my local setup, I
> > have seen cases when the test will simply stop at 20th or 18th step
> > sometimes.
> > 
> 
> That's a clear indication of a race condition and executeWithDelay (timeouts
> in general) misbehaving. If the test itself would timeout, then it'll self
> close, and you could avoid that by a requestLongerTimeout call. But that's
> not the case here.

Then what do you suggest I do. In the list of key that I synthesize, first few will fire command, then next few will fire keypress, then again command and so on. Managing them will be a pain. Let me try that once though.

> > 
> > > @@ +62,5 @@
> > > > +             .setAttribute("timeout", "20");
> > > > +
> > > > +    executeWithDelay(function() {
> > > > +      checkStateAndMoveOn(0);
> > > > +    }, 200);
> > > 
> > > This is pretty much a prettified way of setting a timeout, I wouldn't trust
> > > this at all. Can't you just add event listeners to the searchbox itself?
> > 
> > Wouldn't trust as in ? Anyways, a focus listener should do here :)
> > 
> 
> See above comment :) Timeouts are generally evil.

This is done.

Comment 37

[Girish Sharma [:Optimizer]]

Attachment: patch v2.2 with tests

Only thing remains is the removal of timeouts from the test. I am testing it out. Until then, this patch has the remaining comments addressed.

Comment 38

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

(In reply to Girish Sharma [:Optimizer] from comment #36)
> 
> Then what do you suggest I do. In the list of key that I synthesize, first
> few will fire command, then next few will fire keypress, then again command
> and so on. Managing them will be a pain. Let me try that once though.
> 

Either add event listeners to the searchbox, or fire your own events for what you want to follow. I'd say the former approach is better.

Comment 39

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

Found another bug: 

1. Open inspector on http://news.ycombinator.com/ and lock the inspector on any node.
2. Type "t"
3. Searchbox is red (good).
4. Press backspace.
5. Type "t" again.

No matches are found, but the searchbox doesn't turn red. Should test this behavior as well.

Comment 40

[Girish Sharma [:Optimizer]]

Attachment: patch v3.0 with tests

Fixed everything.

checking for the red border on searchbox too in the tests.
Not using timeout in the test.

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

Comment 41

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

Comment on attachment 703229 [details] [diff] [review]
patch v3.0 with tests

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

Excellent.

::: browser/devtools/inspector/InspectorPanel.jsm
@@ +435,5 @@
> +      case aEvent.DOM_VK_RETURN:
> +        if (query == this._lastSearched) {
> +          this._searchIndex = (this._searchIndex + 1) % this._searchResults.length;
> +        }
> +        else {

Nit: } else {

Comment 42

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

Attachment: one pixel too wide

(In reply to Girish Sharma [:Optimizer] from comment #29)
> 
> If you can send me a screenshot, I will be more clear on how much is
> required :)
> 

Here's what I'm talking about. I think the searchbox is one pixel too wide on the right, at least on OS X.

Comment 43

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

Attachment: low resolution, explained

Here's a low resolution (non-retina) version. The magnifying glass is not centered. Photoshop tells me there is 1 extra pixel on the right.

-moz-padding-end: 5px; centers it perfectly.

Comment 44

[Girish Sharma [:Optimizer]]

Attachment: patch v3.1 with tests

carry forward r+.

Fixed magnifying glass position on Mac . Assuming that Linux has it correct already. This is good to go :)

Comment 45

[Girish Sharma [:Optimizer]]

We can fix the alignment for Linux (or any other style change) in a followup.

Comment 46

[Paul Rouget [:paul]]

(looks good on Linux)

Comment 47

[Paul Rouget [:paul]]

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

Comment 48

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

I'm only just noticing this, but the devtools-no-search-result class has
> transition-property: box-shadow, border-color, background-image;
> transition-duration: 150ms;
however I don't see these transitions at all (OS X). Increasing the duration doesn't have any effect. The code itself looks good to me, but I may have missed something. Paul, can you please give this a look if/when you have the time?

Comment 49

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #48)
> I'm only just noticing this, but the devtools-no-search-result class has
> > transition-property: box-shadow, border-color, background-image;
> > transition-duration: 150ms;
> however I don't see these transitions at all (OS X). Increasing the duration
> doesn't have any effect. The code itself looks good to me, but I may have
> missed something. Paul, can you please give this a look if/when you have the
> time?

Can you see if a !important helps ?

Comment 50

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/5b02a4ee6c39

Comment 51

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #48)
> I'm only just noticing this, but the devtools-no-search-result class has
> > transition-property: box-shadow, border-color, background-image;
> > transition-duration: 150ms;
> however I don't see these transitions at all (OS X). Increasing the duration
> doesn't have any effect. The code itself looks good to me, but I may have
> missed something. Paul, can you please give this a look if/when you have the
> time?

okay that is because the transition-property is getting overwritten by this line on windstripe:

http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/devtools/inspector.css#221

and similar lines on other stripes.

I can fix this in bug 831693's patch.

Comment 52

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

(In reply to Girish Sharma [:Optimizer] from comment #51)
> 
> and similar lines on other stripes.
> 
> I can fix this in bug 831693's patch.

No, file a new bug for it.

Comment 53

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #52)
> (In reply to Girish Sharma [:Optimizer] from comment #51)
> > 
> > and similar lines on other stripes.
> > 
> > I can fix this in bug 831693's patch.
> 
> No, file a new bug for it.

okay, but apparently background-image cannot be transitioned. So having only a border transitioned along with a sudden changing background does not seem well. Removing the whole transition from .devtools-no-search-result seems to be a better option.

Filed bug 833885