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

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

(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.

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.

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.

rebased with latest master

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.

- 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 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 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.

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

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?

(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.

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

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 on attachment 8864017 [details] [diff] [review] WIP-fix-1354504-4.patch Hey Ricky, Could you take a look at this ? Thanks.

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"

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)?

(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.

Forgot to add the autocomplete popup file.

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")

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?

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

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

- Negative flag suggestion as mentioned in comment#22

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!

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.

- merged latest master, hopefully no conflicts now :)

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)

(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).

(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?

> 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.

>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 :)

(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; } }

(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.

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

- Implemented your suggestion of computeState() method

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 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.

- 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 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" ?

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

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d5d0e32f5904 Add autocomplete to network monitor search box. r=jdescottes, ntim

Thanks a lot guys for all your help!

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

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

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

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

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`

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.

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.

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

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

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2757a36546c0 Add autocomplete to network monitor search box. r=jdescottes, ntim

Ah! Thanks Tim.

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b1a4643318 Add test cases for SearchBox and AutocompletePopup react components. r=ntim, jdescottes.

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]