1364093 - After a space character, autocomplete should be shown for all flags

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[Tim Nguyen :ntim]

If you type: "status-code:200 m", I would expect autocomplete to show up for all flags starting with "m".

Comment 1

[Ruturaj Vartak]

So, the options in (In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from comment #0)
> If you type: "status-code:200 m", I would expect autocomplete to show up for
> all flags starting with "m".

So then the options shown would be "method:" and "mime-type:", right ? And if the guy clicks / selects "method" would the query filter change to "method:" or "status-code:200 method:" ?

Comment 2

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #1)
> So, the options in (In reply to Tim Nguyen :ntim (not accepting requests
> until 1st June) from comment #0)
> > If you type: "status-code:200 m", I would expect autocomplete to show up for
> > all flags starting with "m".
> 
> So then the options shown would be "method:" and "mime-type:", right ? And
> if the guy clicks / selects "method" would the query filter change to
> "method:" or "status-code:200 method:" ?

Yes, exactly.

Comment 3

[Ruturaj Vartak]

Attachment: WIP-1364093-1.patch

Hey Tim / Julian,

Its a WIP patch, if the idea is right I'll go ahead add some test-cases as well..

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8868913 [details] [diff] [review]
WIP-1364093-1.patch

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

Thanks for the patch. 

Let's first fix Bug 1364097. I feel like this code should at least leave in the searchbox component.
Maybe check with ntim, he doesn't necessarily have the same opinion.

::: devtools/client/shared/components/autocomplete-popup.js
@@ +32,4 @@
>      }
>    },
>  
> +  computeFilterTokens(filter) {

I don't think this logic should be in the autocomplete popup. Tokenizing the input value seems to be something that should be handled by the searchbox (since it's the one responsible for the input). 

Deciding to show the autocomplete popup again after a space also seems like something that should be optional?

@@ +36,5 @@
> +    let tokens = filter.split(/\s+/);
> +    let lastFilterChar = filter.length > 0 ? filter.slice(-1) : "";
> +    let filterToUse = "";
> +    if (tokens.length > 0) {
> +      if (lastFilterChar !== " ") {

If we implement Bug 1364097 first, this will have an impact on the solution here, since you will only care about non empty tokens.

I would suggest to get fix Bug 1364097 before moving forward here.

@@ +104,5 @@
>     */
>    select() {
>      if (this.refs.selected) {
> +      let { tokens } = this.state;
> +      let selectionItem = "";

FYI, for any tokens.length, you can do 

let selectionItem = [
  ...tokens.slice(0, tokens.length - 1),
  this.refs.selected.textContent
].join(" ")

Note that the current approach doesn't preserve the number of spaces typed by the user.

For instance: "cause:test      m" will become "cause:test method:" if using the autocomplete. Just mentioning it for correctness, but I don't think we need to care about this.

Comment 5

[Ruturaj Vartak]

Attachment: WIP-1364093-2.patch

Another go ...
- changed the logic of tokenization to search-box
- added a prop of tokenizer to search-box

Currently there is a problem, which I've commented as
// Unable to find this.refs.input.value updated on selecting autocomplete item
Whenever an item is clicked / selected using enter/tab, the autocomplete doesn't hide.

Let me know if its correct direction.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8870685 [details] [diff] [review]
WIP-1364093-2.patch

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

If handling tokenizationin the autocomplete popup works, let's revert to that.
I won't have the bandwidth to investigate the issue at the moment, sorry :(

Please make sure to make the behavior optional though, I don't think that's something we want as a default behavior for a shared autocomplete component.

Comment 7

[Tim Nguyen :ntim]

I don't think we need tokenization here.

As I mentioned before, we could simply make autocompleteList a function that accepts a filter as parameter.

Here are some examples of what that function should return:

>> autocompleteList("domain:google.com ")
-> No list

>> autocompleteList("domain:google.com m")
-> ["domain:google.com method:", "domain:google.com mime-type:"]


Another advantage of allowing the list to be specified as a function is that you can also make the popup suggest values:

>> autocompleteList("is:")
-> ["is:cached", "is:running"]

Comment 8

[Tim Nguyen :ntim]

You might need some extra CSS to make the popup items overflow on the left, because you don't want "domain:google.com method:" to overflow like this: "domain:google.com m...", but you want it to overflow like this: "...google.com method:". You can use direction: rtl; for that.


I can help with that if needed.

Comment 9

[Ruturaj Vartak]

Attachment: WIP-1364093-3.patch

Tim,

WIP patch Based on your suggestion.

- the tokenization is duplicated in onItemSelected
- CSS text-overflow: I thought of changing direction: rtl, and applying ellipses, but that wouldn't work for rtl languages, need something that reverses current direction.. :)

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8871178 [details] [diff] [review]
WIP-1364093-3.patch

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

This is a step in the right direction, but I really don't like the tokenization logic because it's not generic enough.

::: devtools/client/shared/components/search-box.js
@@ +183,5 @@
>          onItemSelected: (itemValue) => {
> +          let tokens = value.split(/\s+/);
> +          let newValue = tokens.length == 1 ?
> +            itemValue : tokens.slice(0, tokens.length - 1).join(" ") + " " + itemValue;
> +          this.setState({ value: newValue });

There are 2 ways you can avoid this tokenization logic here:
- Either suggest the full value with the previous flag ("is:cached mime-type:" rather than "mime-type:")
- Either support both value and displayValue in the getAutocompleteList function:

[{value: "is:cached mime-type:", displayValue: "mime-type:"}]

Comment 11

[Ruturaj Vartak]

Attachment: WIP-1364093-4.patch

Tim,

I preferred your second idea, however I've slightly modified it. Please check if thats OK.

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8871223 [details] [diff] [review]
WIP-1364093-4.patch

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

I still think this logic could be more simple using the {value, displayValue} idea I've suggested.
Plus, it makes the logic more generic, and allows the values to be autocompleted easily.

See https://www.irccloud.com/pastebin/T7MgGiiP/ for what I'm thinking. AutocompletePopup simply needs to be modified to support displayValue and value.

Comment 13

[Ruturaj Vartak]

Attachment: WIP-1364093-5.patch

- Have worked on your idea of [{value, displayValue} ,...]
- I had to add another state variable `selected` in search-box, to hide autocomplete on mousedown / tab / enter. Initially the autocomplete list was generated inside autocomplete - and we used its length, now that list is going as a prop, I had to use another variable.

If the gear has slotted in right path.. :)  I'll start with cleanup, test-cases, etc...

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8872016 [details] [diff] [review]
WIP-1364093-5.patch

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

Thanks!

Can you move down the list computation to the autocomplete-popup component ?
The function can be passed in the autocomplete-popup component and the component can use it to compute the list.

autocomplete-popup.js:
render() {
  let { getAutocompleteList, filter } = this.props;
  let list = getAutocompleteList(filter);
  ...
}

The nice thing about doing this is that the autocomplete popup component doesn't need to maintain the list in its internal state: computeState, componentWillReceiveProps can be removed. And getInitialState can simply return the selectedIndex.

That also means that the search-box component doesn't need to maintain the list state, and just needs to pass the function to the autocomplete-popup component.

Other than that, I'm thinking autocompleteProvider may be a better name than getAutocompleteList for the component props.

::: devtools/client/netmonitor/src/components/toolbar.js
@@ +98,3 @@
>        .map((item) => `${item}:`);
>  
> +    function getAutocompleteList(filter) {

Can you move this to devtools/client/netmonitor/src/utils/filter-text-utils.js ?

@@ +98,5 @@
>        .map((item) => `${item}:`);
>  
> +    function getAutocompleteList(filter) {
> +      let emptyObject = [];
> +      if (filter === "" || typeof filter === "undefined") {

!filter is the same as typeof filter === "undefined"

@@ +99,5 @@
>  
> +    function getAutocompleteList(filter) {
> +      let emptyObject = [];
> +      if (filter === "" || typeof filter === "undefined") {
> +        return emptyObject;

No need for the extra emptyObject variable, you can just return []

@@ +106,5 @@
> +      let tokens = filter.split(/\s+/g);
> +      let lastToken = tokens[tokens.length - 1];
> +      let previousTokens = tokens.slice(0, tokens.length - 1);
> +
> +      let autocompleteFilter = lastToken !== "" ? lastToken : "";

What is this handling ?

Can't we simply do:

if (lastToken === "" || !lastToken) {
  return [];
}
?

Comment 15

[Ruturaj Vartak]

Attachment: WIP-1364093-6.patch

- autocompleteProvider (renamed) moved to filter-text-utils.js
- passed autocompleteProvider all the way to AutocompletePopup
- I had too keep the list in the state, since cycling through the list would've needed recurring computation.
- added focused: true in onChange (I think previous bug, while typing, if we press Esc key, the autocomplete never returns, till we blur and focus into the input box)

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8872027 [details] [diff] [review]
WIP-1364093-6.patch

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

Looks good! Works well to me, can you update the tests and maybe add some ?

Let's see what Julian thinks as well.

::: devtools/client/netmonitor/src/components/toolbar.js
@@ +22,2 @@
>  
>  const { L10N } = require("../utils/l10n");

nit: newline placement

...
} = require("../selectors/index");

const { autocompleteProvider } = require("...");
const { L10N } = require("../utils/l10n");

::: devtools/client/netmonitor/src/utils/filter-text-utils.js
@@ +254,5 @@
> +  let tokens = filter.split(/\s+/g);
> +  let lastToken = tokens[tokens.length - 1];
> +  let previousTokens = tokens.slice(0, tokens.length - 1);
> +
> +  if (lastToken === "" || !lastToken) {

Actually, an empty string is falsy (sorry I didn't look carefully before), so !lastToken is enough.

::: devtools/client/shared/components/search-box.js
@@ +26,5 @@
>    },
>  
>    getDefaultProps() {
>      return {
> +      autocompleteProvider: function () {

You can shorten this to:

return {
  autocompleteProvider: () => [],
};

@@ +108,2 @@
>      let { autocomplete } = this.refs;
> +    if (!autocomplete || autocomplete.state.list.length < 1) {

nit: use <= 0 instead of < 1

@@ +149,5 @@
>      } = this.props;
>      let { value } = this.state;
>      let divClassList = ["devtools-searchbox", "has-clear-btn"];
>      let inputClassList = [`devtools-${type}input`];
> +    let showAutocomplete = this.state.focused && value !== "";

You want to make sure autocompleteProvider is supplied as well here.

let showAutocomplete = autocompleteProvider && this.state.focused && !!value;

Comment 17

[Ruturaj Vartak]

Attachment: fix-1364093-1.patch

- Updated and added test cases. Let me know if anything should be added.
- Skipped checking for showAutocomplete = autocompleteProvider ... since a function reference is truthy and it'll always return true.
- worked on your suggestions.

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8872064 [details] [diff] [review]
fix-1364093-1.patch

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

Thanks! Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0eccfc9c6edfa76c420e6a76922b3c9f77a2f57

Julian, do you have anything to add ?

::: devtools/client/shared/components/search-box.js
@@ +147,5 @@
>      } = this.props;
>      let { value } = this.state;
>      let divClassList = ["devtools-searchbox", "has-clear-btn"];
>      let inputClassList = [`devtools-${type}input`];
> +    let showAutocomplete = this.state.focused && value !== "";

I think it would be better to remove getDefaultProps(), then add autocompleteProvider to the condition so we don't load the autocomplete component when autocompleteProvider isn't specified (because autocompleteProvider would be undefined instead of a function reference).

Comment 19

[Ruturaj Vartak]

Attachment: fix-1364093-2.patch

- removed getDefaultProps from search-box.js, and used autocompleteProvider in computation of showAutocomplete

Comment 20

[Ruturaj Vartak]

Attachment: fix-1364093-3.patch

Tim,

I've added some test cases for the autocompleteProvider from filter-text-utils.js. However I don't know if that's the right place to add it.

Thanks

Comment 21

[Ruturaj Vartak]

Attachment: fix-1364093-2.patch

Marked the 2nd patch as obsolute by mistake.

Comment 22

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #20)
> Created attachment 8872163 [details] [diff] [review]
> fix-1364093-3.patch
> 
> Tim,
> 
> I've added some test cases for the autocompleteProvider from
> filter-text-utils.js. However I don't know if that's the right place to add
> it.
> 
> Thanks

No, the new test should be in devtools/client/netmonitor/test (probably called browser_net_filter-autocomplete.js).

Comment 23

[Ruturaj Vartak]

Attachment: fix-1364093-4.patch

Hey Tim,

I've reverted the incorrectly placed test.

And, I've added browser_net_filter-autocomplete.js. Please have a look, I had set browser_net_filter-01.js as the template to work on.

Even though test cases seem to be working, I don't really see the popup coming, the strings going into the text-box. Or is it just too quick?

Thanks

Comment 24

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #23)
> Created attachment 8872241 [details] [diff] [review]
> fix-1364093-4.patch
> 
> Hey Tim,
> 
> I've reverted the incorrectly placed test.
> 
> And, I've added browser_net_filter-autocomplete.js. Please have a look, I
> had set browser_net_filter-01.js as the template to work on.
> 
> Even though test cases seem to be working, I don't really see the popup
> coming, the strings going into the text-box. Or is it just too quick?

If the test passes, it simply means that it's running too fast for you to see.

For debugging purposes, you can pause the test if needed though: await wait(10000).

Comment 25

[Tim Nguyen :ntim]

Comment on attachment 8872241 [details] [diff] [review]
fix-1364093-4.patch

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

The new test looks good, but I think you should separate it from this patch and move this to another bug to make review easier (the network monitor test should be reviewed by Honza), and to get this landed sooner.

I filed bug 1368431 to get that done.


If you don't mind I'm going to make the revision without the netmonitor test the last version.

::: devtools/client/netmonitor/test/browser_net_filter-autocomplete.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +add_task(function* () {

nit: use add_task(async function() {

and then below in the test:

await instead of yield

(We're starting to move away from task.js)

@@ +15,5 @@
> +  EventUtils.synthesizeMouseAtCenter(document.querySelector(".devtools-filterinput"), {}, window)
> +  EventUtils.synthesizeKey("s", {});
> +  // Expecting to see a dropdown now...
> +  ok(document.querySelector(".devtools-autocomplete-popup"), "Autocomplete Popup Created");
> +  let sExpectation = [

nit: please don't use hungarian style naming, use something like let expected

@@ +25,5 @@
> +    "status-code:",
> +  ];
> +  sExpectation.map(function (item, i) {
> +    is(
> +      document

You might want to put this into a helper method in case you add more tests:

testAutocompleteContents(expected) where expected is your array of items.

Comment 26

[Ruturaj Vartak]

Thanks Tim.

Comment 27

[Julian Descottes [:jdescottes]]

Comment on attachment 8872114 [details] [diff] [review]
fix-1364093-2.patch

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

Looks good, thanks! 

Can you add comments & jsdoc in the spots I mentioned?

::: devtools/client/netmonitor/src/utils/filter-text-utils.js
@@ +241,5 @@
>  
>    return match;
>  }
>  
> +function autocompleteProvider(filter) {

Add a JSDoc

@@ +246,5 @@
> +  let negativeAutocompleteList = FILTER_FLAGS.map((item) => `-${item}`);
> +  let baseList = [...FILTER_FLAGS, ...negativeAutocompleteList]
> +    .map((item) => `${item}:`);
> +
> +  if (!filter) {

move this to the top of the function

@@ +250,5 @@
> +  if (!filter) {
> +    return [];
> +  }
> +
> +  let tokens = filter.split(/\s+/g);

Should add a comment explaining we always autocomplete based on the last token (implied, we are not trying to match what the user is currently editing based on the caret position etc...)

@@ +254,5 @@
> +  let tokens = filter.split(/\s+/g);
> +  let lastToken = tokens[tokens.length - 1];
> +  let previousTokens = tokens.slice(0, tokens.length - 1);
> +
> +  if (!lastToken) {

Add a comment explaining why we are bailing out.

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

We should add a doc that explains what are the arguments and return value of this autocompleteProvider, otherwise you have to read the implementation to know what the contract is.

Comment 28

[Tim Nguyen :ntim]

Feel free to mark the updated patch as review+ and add the checkin-needed keyword to the bug.

Comment 29

[Ruturaj Vartak]

Attachment: fix-1364093-5.patch

Hey Julian,
I've added the comments.

Tim,
I haven't yet added the checkin-needed flag, Not sure if my comments will pass :)

Comment 30

[Ruturaj Vartak]

Comment on attachment 8875230 [details] [diff] [review]
fix-1364093-5.patch

Please ignore this fix !!! I'm sorry its from a wrong branch fix.

Comment 31

[Ruturaj Vartak]

Attachment: fix-1364093-5.patch

- The correct patch with comments as requested by Julian

Comment 32

[Julian Descottes [:jdescottes]]

Comment on attachment 8875234 [details] [diff] [review]
fix-1364093-5.patch

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

Looks good to me, thanks!

::: devtools/client/netmonitor/src/utils/filter-text-utils.js
@@ +247,5 @@
> + *
> + * It expects an entire string of the searchbox ie "is:cached pr".
> + * The string is then tokenized into "is:cached" and "pr"
> + *
> + * The output is an array of objects

This should follow a @return annotation, below the @param

Comment 33

[Ruturaj Vartak]

Attachment: fix-1364093-6.patch

Added @return annotation.

Comment 34

[Tim Nguyen :ntim]

Thanks! Can you undo the netmonitor/test changes ? These belong in bug 1368431.

Comment 35

[Ruturaj Vartak]

Attachment: fix-1364093-7.patch

- Removed browser_net_filter-autocomplete.js

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82ffae610a03
After a space and character, autocomplete should be shown for all flags. r=ntim, jdescottes.

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/82ffae610a03

Comment 38

[Ruturaj Vartak]

Thanks a lot guys

Comment 39

[Md. Rahimul Islam]

I have successfully reproduced this bug with Nightly 55.0a1 (2017-05-11)  on windows 10(32bit)

this bug is verified fix with  latest beta 55.0b2 (32-bit)

Build ID: 20170615063713
Mozilla/5.0 (Windows NT 10.0; rv:55.0) Gecko/20100101 Firefox/55.0