1354504 - Autocomplete Network Monitor filters

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement)

Description

[Sebastian Zartner [:sebo]]

An autocompletion popup should be shown when entering the first few letters of filter names and values within the Network Monitor like it's done in the Chrome DevTools.

Sebastian

Comment 1

[Ruturaj Vartak]

Attachment: Screenshot from 2017-04-29 23-02-51.png

Hey Tim,

Do you know why the search-box input has 2 nested DIVs inside it?

Thanks,

Comment 2

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #1)
> Created attachment 8863152 [details]
> Screenshot from 2017-04-29 23-02-51.png
> 
> Hey Tim,
> 
> Do you know why the search-box input has 2 nested DIVs inside it?
That's just the shadow dom the platform code generates, only the Browser Toolbox displays it, but it's not actually there. You can safely ignore it.

Comment 3

[Ruturaj Vartak]

Attachment: autocomplete-preview.png

Hey Tim,

This is what I've got working on devtools-core/standalone way. Strangely it doesn't work in Mozilla build (./mach build faster). I inspected the DOM, the DOM is rendered the way I wanted it.

My next attachment is the way I've tried manage the autocomplete.

Comment 4

[Ruturaj Vartak]

Attachment: WIP-fix-1354504-1.patch

As mentioned in comment#3, this patch works fine for devtools-core / standalone somehow doesn't in the Firefox build (./mach build faster) :(

Please check if its the right way.

Thanks

Note: Patch isn't formatted in anyway for checkin.

Comment 5

[Ruturaj Vartak]

Attachment: WIP-fix-1354504-2.patch

rebased with latest master

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8863181 [details] [diff] [review]
WIP-fix-1354504-2.patch

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

You should be able to try something like this:

https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Autocomplete

The popup should be added in toolbox.xul, and the attribute on the toolbox iframes. I can't guarantee it will work, since the snippet uses a browser element.


But in this case, I would really really prefer using a custom <ul>, because it provides much better flexibility.

Comment 7

[Ruturaj Vartak]

Attachment: WIP-fix-1354504-3.patch

- Got autocomplete working without native HTML5, And it works in `./mach build faster` route as well.
- the click Handler of the UL element isn't working as expected. Please check that, donno what's wrong
- implemented focus, arrow keys, enter, tab handlers

- I'm new to React so might've done 1000s of things wrong, please verify.
- I personally didn't like the ArrowDown, ArrowUp code (though it works), I'd like to see a better/elegant approach.

- Phew ! Has taken away the whole of my day! :D

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8863335 [details] [diff] [review]
WIP-fix-1354504-3.patch

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

Nice work!

Some general feedback:

- Autocomplete should only be enabled when autocompleteList is present in the props
- All the autocompletion logic should be in SearchBoxAutocomplete

Here's how this could possibly work:

in the render function your SearchBox component:

return dom.div(
  {className: (...)},
    (...)
    autocompleteList && this.state.focused && AutocompletePopup({
      autocompleteList, // from this.props
      inputValue: value, // from this.state.value
      ref: "autocomplete",
      onItemSelected: () => { this.state.value = value },
    });
);

Then in onKeydown() of SearchBox:

if (!this.refs.autocomplete) {
  return;
}

switch (e.key) {
  case "ArrowDown":
    this.refs.autocomplete.cycleDown();
    break;
  case "ArrowUp":
    (...)
  case "Tab":
  case "Enter":
    this.refs.autocomplete.select();
    break;
}


Then you would set this.state.focused as appropriate in onBlur() and onFocus().
  
And finally, you'll want to implement the relevant methods in AutocompletePopup.

Let me know if you need any clarification.

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8863335 [details] [diff] [review]
WIP-fix-1354504-3.patch

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

::: devtools/client/shared/components/search-box.js
@@ +200,5 @@
> +  },
> +
> +  onAutoCompleteClick(e) {
> +    console.log(e.target);
> +    console.log("This works randomly like 1/20 times");

This is because the blur event happens before the click event in most cases.

You can use the mousedown event in this case.

Comment 10

[Ruturaj Vartak]

Thanks for the quick review Tim, lemme work on your guidelines.

Comment 11

[Ruturaj Vartak]

Hey Tim,

I was looking to split the Components..
The Events of the SearchBox have to be passed to the SearchBoxAutoComplete Component, in which case, something like below has to be set

SEARCHBOX 
=========
[State]
	value: "",
	isAutoCompleteListOpen: false,
	searchBoxEvent: "",

SearchBoxAutoComplete
=====================
[Properties]
	autoCompleteList,
	isAutoCompleteListOpen,
	filter: value,
	searchBoxEvent: eventForAutoCompleteBox,
[State]
	autoCompleteList: this.props.autoCompleteList,
	autoCompleteSelectionIndex: -1

 
In this case the SearchBoxAutoComplete doesn't really do anything, since every event we send will update Auto component, the state variable autoCompleteSelectionIndex can't really be kept in SearchBoxAutoComplete since it gets reInitialized everytime.


Suggestions?

Comment 12

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #11)
> Hey Tim,
> 
> I was looking to split the Components..
> The Events of the SearchBox have to be passed to the SearchBoxAutoComplete
> Component, in which case, something like below has to be set
> 
> SEARCHBOX 
> =========
> [State]
> 	value: "",
> 	isAutoCompleteListOpen: false,
> 	searchBoxEvent: "",
> 
> SearchBoxAutoComplete
> =====================
> [Properties]
> 	autoCompleteList,
> 	isAutoCompleteListOpen,
> 	filter: value,
> 	searchBoxEvent: eventForAutoCompleteBox,
> [State]
> 	autoCompleteList: this.props.autoCompleteList,
> 	autoCompleteSelectionIndex: -1
> 
>  
> In this case the SearchBoxAutoComplete doesn't really do anything, since
> every event we send will update Auto component, the state variable
> autoCompleteSelectionIndex can't really be kept in SearchBoxAutoComplete
> since it gets reInitialized everytime.
> 
> 
> Suggestions?

So event handling stays in SearchBox, which calls methods from SearchBoxAutoComplete.


SearchBoxAutoComplete
=====================
[Methods]:
- cycleDown
- cycleUp
- select (which calls the onItemSelected() prop that's passed in from the SearchBox component).

SearchBox does the event handling:

onKeydown(e):

if (!this.refs.autocomplete) {
  return;
}

switch (e.key) {
  case "ArrowDown":
    this.refs.autocomplete.cycleDown();
    break;
  case "ArrowUp":
    (...)
  case "Tab":
  case "Enter":
    this.refs.autocomplete.select();
    break;
}


onBlur():
  this.state.focused = false;

onFocus():
  this.state.focused = true;

Then in the render method of SearchBox:

    autocompleteList && this.state.focused && AutocompletePopup({
      autocompleteList, // from this.props
      inputValue: value, // from this.state.value
      ref: "autocomplete",
      onItemSelected: () => { this.state.value = value },
    });


this.state.focused && AutocompletePopup will allow the AutocompletePopup to be hidden when the input isn't focused. So there's no need for the isAutocompleteOpen prop you're suggesting.


Also, there's no need for searchBoxEvent, because the SearchBox component is supposed to handle event handling in this case.

Comment 13

[Ruturaj Vartak]

Thanks for the react tutorial Tim !! This indeed is much better way to do it.

Comment 14

[Ruturaj Vartak]

Attachment: WIP-fix-1354504-4.patch

Hey Tim,

Here is another go at it. WIP, so no Formatting of commit message.

Questions
- should the AutoCompletePopup component be moved outside the search-box.js? I prefer it within Searchbox since its not an individually usable component.
- Should the patches be added separately for bug#1354504 and bug#1354490
- I want to add test[s] for the UI component of auto complete, but its intimidating. :P

Thanks

Comment 15

[Ruturaj Vartak]

Comment on attachment 8864017 [details] [diff] [review]
WIP-fix-1354504-4.patch

Hey Ricky,

Could you take a look at this ?

Thanks.

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8864017 [details] [diff] [review]
WIP-fix-1354504-4.patch

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

Overall comments:
- We usually don't capitalize the "C" in autocomplete in the source code
- It would be nice if the style matched the other autocomplete popups in devtools


> - should the AutoCompletePopup component be moved outside the search-box.js? I prefer it within Searchbox since its not an individually usable component.
Yes, it would be preferable. I've suggested tweaks to make both components more independent. 

> - Should the patches be added separately for bug#1354504 and bug#1354490
If it's more convenient for you to do everything in one bug, that's fine to keep everything in one patch.


:jdescottes would be a good reviewer for this bug since he works on the non-react based autocomplete popup.

::: devtools/client/shared/components/search-box.js
@@ +12,5 @@
> +let AutoCompletePopup = createFactory(createClass({
> +  displayName: "AutoCompletePopup",
> +
> +  propTypes: {
> +    autoCompleteList: PropTypes.array,

Can you use "list" instead of "autoCompleteList" (only in AutocompletePopup) ? The component is already about autocomplete, so no point in mentioning it again.

@@ +18,5 @@
> +    onItemSelected: PropTypes.func,
> +  },
> +
> +  getInitialState() {
> +    return this.setupAutoComplete();

You can probably put the contents of setAutoComplete directly in this function.

@@ +22,5 @@
> +    return this.setupAutoComplete();
> +  },
> +
> +  componentWillReceiveProps() {
> +    this.setState(this.setupAutoComplete());

You don't need this, since getInitialState() is supposed to do the job.

@@ +27,5 @@
> +  },
> +
> +  componentDidUpdate() {
> +    if (this.state.autoCompleteSelectionIndex !== -1) {
> +      this.refs["autocomplete-selected"].scrollIntoView(false);

this.refs.selected

@@ +36,5 @@
> +    let autoCompleteList = this.props.autoCompleteList.filter((item) => {
> +      return item.toLowerCase().includes(this.props.filter.toLowerCase());
> +    });
> +
> +    return {autoCompleteList, autoCompleteSelectionIndex: -1};

Similarly here, can you name those:

filteredList (to avoid the confusion between this.props.list and this.state.filteredList) and selectedIndex

@@ +46,5 @@
> +        autoCompleteSelectionIndex: this.state.autoCompleteSelectionIndex + 1
> +      });
> +    } else {
> +      this.setState({autoCompleteSelectionIndex: 0});
> +    }

This could be simplified:

let { filteredList } = this.state;
let nextIndex = selectedIndex + 1 == filteredList.length ? 0 : selectedIndex + 1;


this.setState({selectedIndex: nextIndex});

@@ +50,5 @@
> +    }
> +  },
> +
> +  cycleUp() {
> +    if (this.state.autoCompleteList[this.state.autoCompleteSelectionIndex - 1]) {

Same for cycleUp()

@@ +72,5 @@
> +
> +  onMouseDown(e) {
> +    // To prevent Blur event happening on SearchBox component
> +    e.preventDefault();
> +    e.stopPropagation();

e.preventDefault and e.stopPropagation shouldn't be needed, since mousedown will happen before blur anyway.

@@ +74,5 @@
> +    // To prevent Blur event happening on SearchBox component
> +    e.preventDefault();
> +    e.stopPropagation();
> +
> +    if (e.target.nodeName === "LI") {

So instead of this check, I would put onMouseDown directly on each dom.li()

@@ +84,5 @@
> +  render() {
> +    let {autoCompleteList} = this.state;
> +
> +    return dom.ul(
> +      { id: "search-box-autocomplete-list",

It's better to avoid IDs.

@@ +88,5 @@
> +      { id: "search-box-autocomplete-list",
> +        className: "open",
> +        onMouseDown: this.onMouseDown
> +      },
> +      autoCompleteList.map((item, ix) => {

We usually use i over ix

@@ +92,5 @@
> +      autoCompleteList.map((item, ix) => {
> +        let autoCompleteItemClass =
> +          (this.state.autoCompleteSelectionIndex == ix) ? "autocomplete-selected" : "";
> +        return dom.li({
> +          key: item,

What is this key property for ?

@@ +125,5 @@
>  
>    getInitialState() {
>      return {
> +      value: "",
> +      isFocused: false,

I find isFocused a bit verbose, I would prefer "focused"

@@ +156,5 @@
>    },
>  
>    onChange() {
> +    // This is to handle Tab, Enter keyCode which sets isFocused to false
> +    this.setState({isFocused: true});

onChange should never be called when the input isn't focused.

@@ +188,5 @@
>      this.onChange();
>    },
>  
> +  onFocus() {
> +    this.setState({isFocused: true, autoCompleteSelectionIndex: -1});

nit: remove autoCompleteSelectionIndex

@@ +195,5 @@
> +  onBlur() {
> +    this.setState({isFocused: false});
> +  },
> +
> +  onKeyDown(e) {

Please early return in case autocompleteList isn't set as prop.

@@ +210,5 @@
> +        if (this.state.isFocused
> +          && this.refs.autoCompletePopup.isItemSelected()) {
> +          this.setState({
> +            value: this.refs.autoCompletePopup.getSelectedItem(),
> +            isFocused: false,

So, you're preventing the blur event to happen, to finally make it happen here ? I think the e.preventDefault() can be removed from onMouseDown().

@@ +212,5 @@
> +          this.setState({
> +            value: this.refs.autoCompletePopup.getSelectedItem(),
> +            isFocused: false,
> +          });
> +          this.refs.input.focus();

So, to make both components independent, I would create a select() method on AutocompletePopup which would look like this:

if (this.isItemSelected()) {
  this.props.onItemSelected(this.getSelectedItem());
}

and then in SearchBox:

case "Enter":
case "Tab":
  this.refs.autocomplete.select();
  break;

@@ +251,5 @@
> +      }),
> +      autoCompleteList.length > 0 && this.state.isFocused && AutoCompletePopup({
> +        autoCompleteList: this.props.autoCompleteList,
> +        filter: value,
> +        ref: "autoCompletePopup",

This ref can simply be named "autocomplete"

Comment 17

[Ruturaj Vartak]

Attachment: WIP-fix-1354504-5.patch

Hey Tim,

Worked on your suggestions.. Also,

- componentWillReceiveProps() is required:  The list prop isn't directly used  in render(), filteredList is in state (which is used), onChange filter needs to update the filterdList.
- hence I had to keep this.setupAutoComplete();

- e.preventDefault and e.stopPropagation: If I don't use it, the blur event gets queued, which leads to loss of cursor focus on the textbox for the user to type the flag value

- mouseDown Binding - Isn't it better to have a single listener instead of multiple ?

- key property - I never used it, but React throws warning "Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `AutocompletePopup`. See https://fb.me/react-warning-keys for more information."

- KeyDown, Tab key: preventDefault is still needed, or focus changes to cross button.

I tried to move Autocomplete outside the SearchBox component by doing something like this 
const AutocompletePopup = createFactory(require("devtools/client/shared/components/auto-complete"));
I also made entry in the webpack.config.js

However I kept getting this error
Warning: React.createElement: type should not be null, undefined, boolean, or number. It should be a string (for DOM elements) or a ReactClass (for composite components)
Error: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object. Check the render method of `SearchBox`.

Regarding the autocomplete look-and-feel: I haven't worked on it yet, but do you want it similar to the console autoComplete (with small font and gray hover color)?

Comment 18

[Tim Nguyen :ntim]

Attachment: WIP-fix-1354504-6.patch

(In reply to Ruturaj Vartak from comment #17)
> Created attachment 8864437 [details] [diff] [review]
> WIP-fix-1354504-5.patch
> 
> Hey Tim,
> 
> Worked on your suggestions.. Also,
> 
> - componentWillReceiveProps() is required:  The list prop isn't directly
> used  in render(), filteredList is in state (which is used), onChange filter
> needs to update the filterdList.
> - hence I had to keep this.setupAutoComplete();
Sounds fine.

> - e.preventDefault and e.stopPropagation: If I don't use it, the blur event
> gets queued, which leads to loss of cursor focus on the textbox for the user
> to type the flag value
e.stopPropagation shouldn't be needed, but e.preventDefault makes sense.

> - mouseDown Binding - Isn't it better to have a single listener instead of
> multiple ?
In this case, it makes the logic more explicit to have multiple listeners.

> - key property - I never used it, but React throws warning "Warning: Each
> child in an array or iterator should have a unique "key" prop. Check the
> render method of `AutocompletePopup`. See https://fb.me/react-warning-keys
> for more information."
Sounds good.
> - KeyDown, Tab key: preventDefault is still needed, or focus changes to
> cross button.
Sounds good.
> I tried to move Autocomplete outside the SearchBox component by doing
> something like this 
> const AutocompletePopup =
> createFactory(require("devtools/client/shared/components/auto-complete"));
> I also made entry in the webpack.config.js
> 
> However I kept getting this error
> Warning: React.createElement: type should not be null, undefined, boolean,
> or number. It should be a string (for DOM elements) or a ReactClass (for
> composite components)
> Error: Element type is invalid: expected a string (for built-in components)
> or a class/function (for composite components) but got: object. Check the
> render method of `SearchBox`.
> 
> Regarding the autocomplete look-and-feel: I haven't worked on it yet, but do
> you want it similar to the console autoComplete (with small font and gray
> hover color)?

I've attached a version fixing the two last issues.
Feel free to make any tweaks, and ask for :jdescottes for review, he'll have more useful tips than I have since he's worked on the non-react autocomplete widget.

Comment 19

[Tim Nguyen :ntim]

Attachment: WIP-fix-1354504-6.patch

Forgot to add the autocomplete popup file.

Comment 20

[Tim Nguyen :ntim]

Attachment: WIP-fix-1354504-6.patch

Comment 21

[Tim Nguyen :ntim]

A few UX comments:

- Negative filtering is missing ("-status-code:" for example, which means status code not matching)
- The suggestions that are *exactly* the same as the input value should be filtered-out (if someone typed "status-code:", "status-code:" should no longer display in the autocomplete popup)
- autocomplete popup should only display values that start with the current input value (eg. typing "s" should not match "transferred")

Comment 22

[Ruturaj Vartak]

Attachment: fix-1354504-1.patch

Hey jdescottes, Tim,

- fixed suggestions, now they match using startsWith
- Tab, Enter, Click, ensures the autocomplete isn't shown.
- The negative, I have a local patch, which does something like this..
if a single option is available for suggestions ie. for filter "sta", option available is "status-code:" in such a case, it adds a negative option to autocomplete ie. the filteredList then has 2 items "status-code:" and "-status-code:" That makes sense?

Comment 23

[Ruturaj Vartak]

oh.. and Tim.. thanks a lot, you always manage to rewrite a lot of my code to fit in smoothly.

Comment 24

[Tim Nguyen :ntim]

Comment on attachment 8864845 [details] [diff] [review]
fix-1354504-1.patch

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

Ricky, can you look at the netmonitor parts as well ? Thanks

Comment 25

[Ruturaj Vartak]

Attachment: negative-match-suggestion.png

- Negative flag suggestion as mentioned in comment#22

Comment 26

[Ricky Chien [:rickychien] (inactive)]

Great work @Ruturaj Vartak! This Autocomplete is not an easy work but you did it :)

I saw a few conflicts when applying your patch, could you take a look again? Thanks!

Comment 27

[Ruturaj Vartak]

Hey thanks,

Tim has done most of the hard work, real thanks should go to him for finding common css code, applying similar styles in CSS.

Ill try to rebase against latest master. And resend the patch.

Is the code good enough to run in standalone mode as well? I'm currently running in that mode, but i found that weback.config.js got missed in commit / patch it seems.

Comment 28

[Ruturaj Vartak]

Attachment: fix-1354504-2.patch

- merged latest master, hopefully no conflicts now :)

Comment 29

[Julian Descottes [:jdescottes]]

Comment on attachment 8865124 [details] [diff] [review]
fix-1354504-2.patch

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

Thanks for working on a react version of the autocomplete :) 

Some overall UX comments first:

- 1. the selected item resets as soon as a new request is displayed. On websites that frequently send new requests, that makes the autocomplete very hard to use. This is linked to the fact that componentWillReceiveProps resets the state every time.
- 2. all the other autocompletes in devtools automatically select the first item of the list when pressing TAB. Here you have to either click on the item or select it with the keyboard. If I already type "reg", I only see "regexp:" in the list and I would like to autocomplete directly using TAB.
- 3. autocomplete lists are usually alphabetically sorted. I understand the current sort order follows the headers order, but here I think alphabetical is more logical
- 4. we probably need a follow up for accessibility here. We can do it in a second step, you can ping :yzen to review this.
- 5. the height of the autocomplete list should be constrained to the document. On small-ish viewports, the autocomplete can very easily overflow, making some suggestions invisible even when cycling on them with the keyboard
- 6. more of a general comment, but seeing the list of header filters as soon as I clicked on the field made me think these were the only accepted values for the field. 
Maybe the autocomplete popup should have a kind of header mentioning that you _can_ filter with these but it's not mandatory (alternatively, Chrome is using an inline autocomplete that only displays one suggestion as the placeholder for the input ... was this option discussed?)
- 7. some keys usually supported in our autocompletes have no effect here (home, end, pageup, pagedown, escape)

I would like to see another patch addressing comments 1 to 3 before giving an r+ here. The other points can be addressed as follow ups.

::: devtools/client/netmonitor/webpack.config.js
@@ +44,4 @@
>        "devtools/client/locales": path.join(__dirname, "../../client/locales/en-US"),
>        "devtools/client/shared/components/reps/reps": path.join(__dirname, "../../client/shared/components/reps/reps"),
>        "devtools/client/shared/components/search-box": path.join(__dirname, "../../client/shared/components/search-box"),
> +      "devtools/client/shared/components/autocomplete-popup": path.join(__dirname, "../../client/shared/components/autocomplete-popup"),

The list is alphabetically sorted, move this line before "devtools/client/shared/components/reps/reps" [...]

::: devtools/client/shared/components/autocomplete-popup.js
@@ +9,5 @@
> +module.exports = createClass({
> +  displayName: "AutocompletePopup",
> +
> +  propTypes: {
> +    list: PropTypes.array,

nit: all 3 props are required, add ".isRequired" to each one of them

::: devtools/client/shared/components/search-box.js
@@ +110,5 @@
> +    }
> +
> +    switch (e.key) {
> +      case "ArrowDown":
> +        autocomplete.cycleDown();

Having to call cycleDown, cycleUp & select on the autocomplete doesn't feel very react friendly. Have you tried to store the filtered list and the selected index in the searchbox state and pass them as props to the autocomplete-popup?

The contract of a react component is usually in the props, not the methods, I think?

@@ +120,5 @@
> +      case "Tab":
> +        e.preventDefault();
> +        autocomplete.select();
> +        break;
> +    }

Our autocompletes usually also support :
- pageup, pagedown, home, end (to easily navigate to the beginning/end of the autocomplete list)
- escape to close the autocomplete list (here it also closes the autocomplete list, but only as a side effect because the splitconsole is opened, blurring the filter field)

Comment 30

[Tim Nguyen :ntim]

(In reply to Julian Descottes [:jdescottes] from comment #29)
> ::: devtools/client/shared/components/search-box.js
> @@ +110,5 @@
> > +    }
> > +
> > +    switch (e.key) {
> > +      case "ArrowDown":
> > +        autocomplete.cycleDown();
> 
> Having to call cycleDown, cycleUp & select on the autocomplete doesn't feel
> very react friendly. Have you tried to store the filtered list and the
> selected index in the searchbox state and pass them as props to the
> autocomplete-popup?
> 
> The contract of a react component is usually in the props, not the methods,
> I think?

The AutocompletePopup component is meant to be for general use (for example, the console might use it), and as independent as possible from other components.

Having all this logic:
+    let { filteredList, selectedIndex } = this.state;
+    let nextIndex = selectedIndex + 1 === filteredList.length ? 0 : selectedIndex + 1;
+    this.setState({selectedIndex: nextIndex});

inside the searchbox component means that all future components that want to use the autocomplete component will need to duplicate this logic (autocomplete.cycleDown() gets rid of that duplication).

> - 2. all the other autocompletes in devtools automatically select the first item of the list when pressing TAB. Here you have to either click on the item or select it with the keyboard. If I already type "reg", I only see "regexp:" in the list and I would like to autocomplete directly using TAB.

I would say this can be a follow up, since this feature doesn't really make sense without the placeholder implemented.

> (alternatively, Chrome is using an inline autocomplete that only displays one suggestion as the placeholder for the input ... was this option discussed?)
Not really, chrome only uses that when there's only one suggestion. If there are multiple suggestions, chrome displays a list (try the letter "s" for example).

Comment 31

[Julian Descottes [:jdescottes]]

(In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from comment #30)
> (In reply to Julian Descottes [:jdescottes] from comment #29)
> > ::: devtools/client/shared/components/search-box.js
> > @@ +110,5 @@
> > > +    }
> > > +
> > > +    switch (e.key) {
> > > +      case "ArrowDown":
> > > +        autocomplete.cycleDown();
> > 
> > Having to call cycleDown, cycleUp & select on the autocomplete doesn't feel
> > very react friendly. Have you tried to store the filtered list and the
> > selected index in the searchbox state and pass them as props to the
> > autocomplete-popup?
> > 
> > The contract of a react component is usually in the props, not the methods,
> > I think?
> 
> The AutocompletePopup component is meant to be for general use (for example,
> the console might use it), and as independent as possible from other
> components.
> 
> Having all this logic:
> +    let { filteredList, selectedIndex } = this.state;
> +    let nextIndex = selectedIndex + 1 === filteredList.length ? 0 :
> selectedIndex + 1;
> +    this.setState({selectedIndex: nextIndex});
> 
> inside the searchbox component means that all future components that want to
> use the autocomplete component will need to duplicate this logic
> (autocomplete.cycleDown() gets rid of that duplication).
> 

If cycleDown(), cycleUp() and select() are a public API for the autocomplete-popup component, let's document them clearly as such.

> > - 2. all the other autocompletes in devtools automatically select the first item of the list when pressing TAB. Here you have to either click on the item or select it with the keyboard. If I already type "reg", I only see "regexp:" in the list and I would like to autocomplete directly using TAB.
> 
> I would say this can be a follow up, since this feature doesn't really make
> sense without the placeholder implemented.
> 

Would be better with a placeholder, but IMO it still makes sense without it :/
I think consistency is important, as I said all our autocompletes behave this way, even if they don't show a placeholder (inspector html search for instance). If you go for a follow up, can you block this bug on it? Hopefully both bugs will make it in the same release.

> > (alternatively, Chrome is using an inline autocomplete that only displays one suggestion as the placeholder for the input ... was this option discussed?)
> Not really, chrome only uses that when there's only one suggestion. If there
> are multiple suggestions, chrome displays a list (try the letter "s" for
> example).

Indeed, they just don't display it directly on click. Do we really want to open the popup when the input is empty then?

Comment 32

[Tim Nguyen :ntim]

> Do we really want to open the popup when the input is empty then?

This is a question for UX.

I would say no for consistency, but on the other hand, it's better for discovery, so I'm not sure.

Comment 33

[Ruturaj Vartak]

Attachment: fix-1354504-3.patch

>1. the selected item resets as soon as a new request is displayed. On websites that frequently send new requests, that makes the autocomplete very hard to use. This is linked to the fact that componentWillReceiveProps resets the state every time.
Is it a really property of the Autocomplete? I'm not sure how to solve this one.

>2. all the other autocompletes in devtools automatically select the first item of the list when pressing TAB. Here you have to either click on the item or select it with the keyboard. If I already type "reg", I only see "regexp:" in the list and I would like to autocomplete directly using TAB.
fixed, now if there is a single autocomplete item, it'll be "selected" in dropdown, a simple "Tab" or "Enter" will write it to the input box

>- 3. autocomplete lists are usually alphabetically sorted. I understand the current sort order follows the headers order, but here I think alphabetical is more logical
fixed

>- 7. some keys usually supported in our autocompletes have no effect here (home, end, pageup, pagedown, escape)
pagedown = +5 movements
pageup = -5 movements
home = 0th selection
end = last selection
implemented generic jumpBy

>The list is alphabetically sorted, move this line before "devtools/client/shared/components/reps/reps" [...]
fixed

>nit: all 3 props are required, add ".isRequired" to each one of them
fixed

- fixed "scheme:" was coming in twice, the sort helped to notice it :)

Comment 34

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #33)
> Created attachment 8866238 [details] [diff] [review]
> fix-1354504-3.patch
> 
> >1. the selected item resets as soon as a new request is displayed. On websites that frequently send new requests, that makes the autocomplete very hard to use. This is linked to the fact that componentWillReceiveProps resets the state every time.
> Is it a really property of the Autocomplete? I'm not sure how to solve this
> one.

You can probably do something like this:

componentWillReceiveProps(nextProps) {
  if (nextProps.filter === this.props.filter) {
    return;
  }
}

Comment 35

[Julian Descottes [:jdescottes]]

(In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from comment #34)
> (In reply to Ruturaj Vartak from comment #33)
> > Created attachment 8866238 [details] [diff] [review]
> > fix-1354504-3.patch
> > 
> > >1. the selected item resets as soon as a new request is displayed. On websites that frequently send new requests, that makes the autocomplete very hard to use. This is linked to the fact that componentWillReceiveProps resets the state every time.
> > Is it a really property of the Autocomplete? I'm not sure how to solve this
> > one.
> 
> You can probably do something like this:
> 
> componentWillReceiveProps(nextProps) {
>   if (nextProps.filter === this.props.filter) {
>     return;
>   }
> }

That's what I tried when I did my first review, and unfortunately it doesn't work, the list seems to be filtered for the "previous" value of the input. (which is actually why I suggested making the component simpler by extracting the filtering logic etc...).

I looked a bit more into this, and here's what happens. When user is typing in the searchbox, the component will render twice. 

First, in onChange with 
> if (this.state.value !== this.refs.input.value) {
>   this.setState({
>     value: this.refs.input.value,
>     focused: true
>   });
> }

And then in the searchTimeout after dispatching `Actions.setRequestFilterText(text)`.
So componentWillReceiveProps is always called twice with the same nextProps.

The first time componentWillReceiveProps is called, we recompute the filteredList, but the `filter` prop has not been updated yet, so the filteredList is incorrect. The second time, `filter` is still the same (so we would return with the change proposed in the previous comment) but this time if we call getInitialState, this.props.filter has already been updated so the filtering will work as expected.

One way to fix this is to have a function to compute the state:

> computeState({filter, list}) {
>  let filteredList = list.filter((item) => {
>     return item.toLowerCase().startsWith(filter.toLowerCase())
>       && item.toLowerCase() !== filter.toLowerCase();
>   }).sort();
>   let selectedIndex = filteredList.length == 1 ? 0 : -1;
> 
>   return { filteredList, selectedIndex };
> }

Then you can call it from both getInitialState & componentWillReceiveProps:

> getInitialState() {
>   return this.computeState(this.props);
> },
> 
> componentWillReceiveProps(nextProps) {
>   if (this.props.filter === nextProps.filter) {
>     return;
>   }
>
>   this.setState(this.computeState(nextProps));
> },

There might be a more React-friendly way to do it, but the above should work.

Comment 36

[Ruturaj Vartak]

Hey Julian,

Thanks.

Originally setState for focussed = true wasn't there in onChange event, but I added in last commit to ensure that escape hides the AutcompletePopup. And the component gets invoked again when somebody types-in (onChange).

Let me try your suggestion

Comment 37

[Ruturaj Vartak]

Attachment: fix-1354504-4.patch

- Implemented your suggestion of computeState() method

Comment 38

[Julian Descottes [:jdescottes]]

Comment on attachment 8866352 [details] [diff] [review]
fix-1354504-4.patch

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

Sorry I totally missed the css changes during my first review! I have a few comments on it, cleanup mostly. 

Otherwise the patch looks good and works great, thanks for addressing my previous comments.

I think you removed all the code that was relying on the "${theme}-theme" classname set on the autocomplete popup. Unless I missed something, this means we should cleanup the existing autocomplete-popup class from the code that sets this classname. We should open a follow-up bug to do that.

Setting r+. Despite my lengthy comments in the css section, I believe the changes should be really straightforward :)

The last thing I want to mention is that we are missing tests here. Both on the shared component side and on the netmonitor side. I don't want to block landing on this, but let's make sure we log a follow up for adding tests.

I pushed your current changeset to try: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e06b8778a0fd0af0cbed23327653c91a2b3697e

::: devtools/client/netmonitor/src/components/toolbar.js
@@ +109,4 @@
>              placeholder: SEARCH_PLACE_HOLDER,
>              type: "filter",
>              onChange: setRequestFilterText,
> +            autocompleteList: FILTER_FLAGS.map((item) => `${item}:`)

nit: add a "," at the end

::: devtools/client/shared/components/autocomplete-popup.js
@@ +41,5 @@
> +
> +    return { filteredList, selectedIndex };
> +  },
> +
> +  // Use this method to getSelection of the topmost item

Can you mention that this method is called from outside of this class?
JSDoc comments for methods are usually using /* */ comment blocks:
 
/**
 * Use this method to getSelection of the topmost item.
 * This method is a public API, called outside of the autocomplete-popup component.
 */
 
Same comment for jumptToBottom, jumpBy and select

::: devtools/client/themes/common.css
@@ +48,4 @@
>    border-radius: 3px;
>    overflow-x: hidden;
>    max-height: 20rem;
> +  z-index: 999;

This is only useful for this implementation of the autocomplete-popup. Our other autocomplete popups are hosted in a tooltip which is handling the z-index.

You should use an additional different classname (searchbox-autocomplete-popup ?) and set the z-index for this class only. Or you can move it to your `.devtools-searchbox .devtools-autocomplete-popup` selector ?

@@ +83,4 @@
>    background-color: transparent;
>    border-radius: 4px;
>    padding: 1px 0;
> +  cursor: default;

if you are going to set cursor: default here, it can most likely be removed in the next selector

> .devtools-autocomplete-listbox .autocomplete-item > .initial-value,
> .devtools-autocomplete-listbox .autocomplete-item > .autocomplete-value

@@ +105,3 @@
>    border: 1px solid hsl(210,24%,90%);
>    background-image: linear-gradient(to bottom, hsla(209,18%,100%,0.9), hsl(210,24%,95%));
> +  box-shadow: 0 1px 0 hsla(209,29%,90%,.25) inset;

1 - We already have a box-shadow defined for .devtools-autocomplete-popup on line 45 (with a slightly different value 0 1px 0 hsla(209,29%,72%,.25) inset;)

2 - I think this particular box-shadow here was only set for .devtools-autocomplete-popup in light-theme. Is there a good reason to also apply it to CodeMirror-hints & CodeMirror-Tern-tooltip? Both these classes already have box-shadows defined in light-theme.css and dark-theme.css

3 - In case you need to target .devtools-autocomplete-popup in the light-theme only, you should know that the light-theme classname is also added to the container when we are using the Firebug theme (an easy way for the firebug theme to inherit from the light theme). That means that you either need to have a very specific .light-theme:not(.firebug-theme) in your selector, or to override the value for firebug-theme.

Now, I'm being very nit-picky here, for the sake of explaining the impact these kind of changes can have. We're talking about an almost invisible inset box shadow here, I don't think anybody would notice the difference between before and after your change :) So I don't really ask you to keep the colors 100% as they were before, let's just make sure we don't redefine the box-shadow property several times for no reason. To be honest I would just get rid of this box shadow: it's almost not noticeable, and when it is it feels dated. But it is easier if we do this kind of changes in a separate changeset.

@@ +111,5 @@
> +.theme-dark .CodeMirror-hints,
> +.theme-dark .CodeMirror-Tern-tooltip {
> +  border: 1px solid hsl(210,11%,10%);
> +  background-image: linear-gradient(to bottom, hsla(209,18%,18%,0.9), hsl(210,11%,16%));
> +  box-shadow: none;

Cf previous comment, we used to have a box-shadow, even in dark theme, coming from the declaration at line 45.

Comment 39

[Tim Nguyen :ntim]

Comment on attachment 8866352 [details] [diff] [review]
fix-1354504-4.patch

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

::: devtools/client/shared/components/autocomplete-popup.js
@@ +41,5 @@
> +
> +    return { filteredList, selectedIndex };
> +  },
> +
> +  // Use this method to getSelection of the topmost item

I think a more accurate comment would be:

/**
 * Use this method to select the top-most item
 * This method is a public API, called outside of the autocomplete-popup component.
 */

@@ +46,5 @@
> +  jumpToTop() {
> +    this.setState({ selectedIndex: 0 });
> +  },
> +
> +  // Use this method to getSelection of the bottommost item

Same for jumpToBottom

@@ +53,5 @@
> +    this.setState({ selectedIndex });
> +  },
> +
> +  // Takes incremental jumps, if it exceeds it hops to bottom or top based on direction
> +  jumpBy(increment = 1) {

Might be nice to comment the parameter in the JSDoc.

Comment 40

[Ruturaj Vartak]

Attachment: fix-1354504-5.patch

- Added "," in invoking SearchBox component
- I've added JSDoc comments
- Moved z-index to .devtools-searchbox .devtools-autocomplete-popup
- Removed duplicate cursor: default

- I Agree to the requirement of tests, in one of my earlier comments, I had mentioned I wanted
to write test-cases, but its very intimidating for me to begin with scratch,. In the 
patches I've submitted before, I've mostly copy-pasted, or tweaked expectation o/p.

> 1 - We already have a box-shadow defined for .devtools-autocomplete-popup on line 45 (with a slightly different value 0 1px 0 hsla(209,29%,72%,.25) inset;)
>  
> 2 - I think this particular box-shadow here was only set for .devtools-autocomplete-popup in light-theme. Is there a good reason to also apply it to CodeMirror-hints & CodeMirror-Tern-tooltip? Both these classes already have box-shadows defined in light-theme.css and dark-theme.css
>  
> 3 - In case you need to target .devtools-autocomplete-popup in the light-theme only, you should know that the light-theme classname is also added to the container when we are using the Firebug theme (an easy way for the firebug theme to inherit from the light theme). That means that you either need to have a very specific .light-theme:not(.firebug-theme) in your selector, or to override the value for firebug-theme.
>  
> Now, I'm being very nit-picky here, for the sake of explaining the impact these kind of changes can have. We're talking about an almost invisible inset box shadow here, I don't think anybody would notice the difference between before and after your change :) So I don't really ask you to keep the colors 100% as they were before, let's just make sure we don't redefine the box-shadow property several times for no reason. To be honest I would just get rid of this box shadow: it's almost not noticeable, and when it is it feels dated. But it is easier if we do this kind of changes in a separate changeset.

The Sync of UI (css) was done by :ntim, I'm not sure what his thoughts / context was. But I agree
to your comment that the changes are so subtle, they're hardly visible, I'm not touching anything
in this patch, perhaps :ntim can fill in with his comments, then I'll rework on it.

Comment 41

[Julian Descottes [:jdescottes]]

Comment on attachment 8866637 [details] [diff] [review]
fix-1354504-5.patch

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

Looks good to me, thanks. Some nits about the comments. 
When you reupload your patch, you can directly set r+ on it, no need to go for a new review round.

Regarding the CSS changes, that's fine. We'll see with ntim about doing a cleanup after this has 

While testing, I have noticed another behavior which I think is annoying. 
When you clear the filter using the "X" icon, the autocomplete shows up. I think in most cases if you cleared the filters, you don't expect that.
This is linked to the discussion we had about showing the autocomplete popup at all if the filter is empty. 

Let's log a follow-up for this too. Someone working on the netmonitor should take a decision about that.

Submitted to try again, my previous run was sitting on a bad commit apparently. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6baebea7802d1360d04b3fb97e9fc89a8f58a09 (but it looks like again it sits on bad changeset from mozilla central, so I'll push again later today ...)

::: devtools/client/shared/components/autocomplete-popup.js
@@ +43,5 @@
> +  },
> +
> +  /**
> +   * Use this method to select the top-most item
> +   * @public

We don't use @public anywhere in DevTools, let's describe it in plain english.
Same comment for the other methods.

@@ +63,5 @@
> +   * Takes incremental jumps moving the selection of item in up or down direction.
> +   * Use positive incements ie "1" for downward direction and "-1" for upward motion
> +   * Can be given bigger leaps ie. -5 , +5 for PageUp, PageDown
> +   * If the hops exceeds bottom or top it rolls over to begining or end of the list.
> +   *

Maybe we can avoid going into too much details. What about 
"Increment the selected index with the provided increment value.  Will cycle to the 
 beginning/end of the list if the index exceeds the list boundaries." ?

@@ +81,5 @@
> +    this.setState({selectedIndex: nextIndex});
> +  },
> +
> +  /**
> +   * Selects the item (key cycled or clicked) and executes the callback bound

The wording is pretty confusing because we are mixing selecting -> setting the selected index to some value, and selecting -> submitting the item at the selected index to the onItemSelected callback.

Maybe "Submit the currently selected item to the onItemSelected callback" ?

Comment 42

[Ruturaj Vartak]

Attachment: fix-1354504-6.patch

Thanks Julian, I've updated the patch with your suggested comments.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5d0e32f5904
Add autocomplete to network monitor search box. r=jdescottes, ntim

Comment 45

[Ruturaj Vartak]

Thanks a lot guys for all your help!

Comment 46

[Ruturaj Vartak]

Hey Tim, I guess we need the test-cases bug filed as well.

Comment 47

[Julian Descottes [:jdescottes]]

Thanks for working on this Ruturaj. 
And thanks for filing all the follow-up bugs Tim, that helps a lot!

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d5d0e32f5904

Comment 49

[Iris Hsiao [:ihsiao]]

backed out for devtool perma-failing in browser_net_filter-flags.js like https://treeherder.mozilla.org/logviewer.html#?job_id=98565104&repo=mozilla-central&lineNumber=4244

Comment 50

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/ad45e59c7683
Backed out changeset d5d0e32f5904 for devtool perma-failing in browser_net_filter-flags.js. a=backout

Comment 51

[Ruturaj Vartak]

Its strange, I ran `./mach test devtools/client/netmonitor/test/browser_net_filter-flags.js`
`
...
leakcheck | refcount logging is off, so leaks can't be detected!
runtests.py | Running tests: end.
Buffered messages finished
TEST-INFO | checking window state
Browser Chrome Test Summary
	Passed: 1251
	Failed: 0
	Todo: 0
	Mode: e10s
*** End BrowserChrome Test Results ***
Buffered messages finished
SUITE-END | took 11s
`
Same is the case when run with `./mach mochitest devtools/client/netmonitor/test/browser_net_filter-flags.js`

Here is the output at my end https://pastebin.com/M6PENFRg for `./mach test devtools/client/netmonitor/test/browser_net_filter-flags.js`

Comment 52

[Julian Descottes [:jdescottes]]

This test also never failed in the try runs I submitted. I checked out the exact changeset that got backed out on central (ie d5d0e32f5904) and the test is still successful locally. Even ran it with the rest of the netmonitor tests to be sure. 

I pushed it to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8abd33bedfe87903065822c471dd47367ea92e9
Let's see if this is a failure that only happens with the mozilla-central configuration.

Comment 53

[Tim Nguyen :ntim]

I can actually experience the failure locally by pulling the latest code.

I'm guessing another patch on autoland + this patch caused the test to fail.

Comment 54

[Tim Nguyen :ntim]

Yep, if I remove the changes in bug 1360473, the test just passes fine.

Comment 55

[Tim Nguyen :ntim]

Oh, I see what's happening here, this patch accidentally removed the set-cookie-* flags while moving the flag list to the constants.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2757a36546c0
Add autocomplete to network monitor search box. r=jdescottes, ntim

Comment 57

[Ruturaj Vartak]

Ah! Thanks Tim.

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2757a36546c0

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b1a4643318
Add test cases for SearchBox and AutocompletePopup react components. r=ntim, jdescottes.

Comment 60

[Almas Hossain[:mAlmas]]

I have reproduce this bug with Nightly 55.0a1 (2017-04-07) (64-bit) in Windows 10.

This bug's fix is verified with latest Nightly 55.0a1 (64-bit).
 
Build ID   :    20170517030204
User Agent :	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0


[bugday-20170517]