1364092 - Add negative filtering in autocomplete

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[Tim Nguyen :ntim]

Negative filtering (all the flags with - in front) results should show up in autocomplete.

I think adding all the flags with - in front to the autocompleteList should be enough

Comment 1

[Ruturaj Vartak]

Attachment: negative-match-suggestion.png

Something like this should be good enough right ?

Comment 2

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #1)
> Created attachment 8866834 [details]
> negative-match-suggestion.png
> 
> Something like this should be good enough right ?

I personally don't think we should show "-transferred" when "tr" is typed.

I would just keep the current behaviour, but with the negative filters added.

Comment 3

[Ruturaj Vartak]

Should the negative filters be shown just below their corresponding positive filters? or else all negative ones will be shown first - thanks to our `filteredList...sort()` in the AutocompletePopup component.

Also - regexp wouldn't have a negative filter right ? Assuming the user can put a negative regex

Comment 4

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #3)
> Should the negative filters be shown just below their corresponding positive
> filters? or else all negative ones will be shown first - thanks to our
> `filteredList...sort()` in the AutocompletePopup component.
I guess it would be best to hide the autocomplete popup when no value is set (bug 1364097).
Then we don't have to worry about the sorting.

Or we could put all the negative filters at the end of the list.

> Also - regexp wouldn't have a negative filter right ? Assuming the user can
> put a negative regex

Well, -regexp:^hello would mean all requests with URLs that *don't* start with hello, so I guess negative filtering still makes sense with regexps

Comment 5

[Ruturaj Vartak]

Attachment: fix-1364092-1.patch

First attempt

- Appneded negative filtering list to FILTER_FLAGS
- Order of the list: Responsibility moved to implementor of Autocomplete (toolbar.js)
- Updated test case

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8868903 [details] [diff] [review]
fix-1364092-1.patch

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

Thanks for the patch Ruturaj! Some comments but in the good direction. 

With this changeset, the autocomplete list displayed by default is twice as big as it used to be. 
This makes an issue spotted during the initial bug even more visible:

> 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 

(I'll attach a screenshot to illustrate.)

I will block this on Bug 1364097, which will prevent showing the autocomplete if no value is set and will make this issue less visible.

::: devtools/client/netmonitor/src/components/toolbar.js
@@ +21,4 @@
>  
>  const { L10N } = require("../utils/l10n");
>  
> +// Setup autocomplete list

The top of the file should be reserved for requiring dependencies and declaring straightforward constants. 

Let's move this to the render. Wrap it in a dedicated method if you prefer. I doubt the performance impact of recomputing this small this is worth keeping it here.

@@ +21,5 @@
>  
>  const { L10N } = require("../utils/l10n");
>  
> +// Setup autocomplete list
> +FILTER_FLAGS.sort();

I don't mind moving the sort out of the autocomplete-popup but in case you didn't know, you can pass your own compare method to sort: 

autocompleteList.sort((a, b) => {
  if (a.startsWith("-") != b.startsWith("-")) {
    // Items starting with "-" have greater index than the others.
    return a.startsWith("-") ? 1 : -1;
  }
  return a.localeCompare(b);
});

(this implementation is too specific to the special case of "-" but we could have something similar that would just give items starting [a-zA-Z] a lower index)

However I'm really not a big fan of calling sort here on FILTER_FLAGS, which is supposed to be constant. I would say either sort it in constants.js from the beginning, or sort a clone of the array.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: netmon_filter_selection.gif

See the GIF (viewport size is usually what I use on my computer).

Navigating with the keyboard easily moves the selected item out of the visible area.

Comment 8

[Ruturaj Vartak]

> Navigating with the keyboard easily moves the selected item out of the visible area.
Was wondering if we could pass classNames as props to AutoCompletePopup, Then we could override the max-height such that only 4-5 items are shown.

> The top of the file should be reserved for requiring dependencies and declaring straightforward constants. 
Agreed, I didn't realize that FILTER_FLAGS was getting mutated for others' use. Your soln. is better.

> (this implementation is too specific to the special case of "-" but we could have something similar that would just give items starting [a-zA-Z] a lower index)
I personally prefer this way, as the user who implements Autocomplete must have control on how things should be rendered for his component. In this case toolbar.js of Netmon wants it the way u've suggested :)

> However I'm really not a big fan of calling sort here on FILTER_FLAGS, which is supposed to be constant. I would say either sort it in constants.js from the beginning, or sort a clone of the array.
Agreed.. FILTER_FLAGS shouldn't get modified.

So, should we override using classNames?

Comment 9

[Tim Nguyen :ntim]

I think it would be much simpler if we simply hide the popup when no value is entered, that seems like the most reasonable thing to do for now.

That way, you won't even have to take care of changing the sorting.

Comment 10

[Julian Descottes [:jdescottes]]

I agree with the comment from ntim. If we don't show the popup for no value, most issues go away. 
Let's address Bug 1364097 first and resume after that.

Comment 11

[Ruturaj Vartak]

Julian, Tim,

Tim, I know you already feel that you don't prefer attachment#8866834 [details], and you wanted user to explicitly to start with "-". Now with bug#1364097 getting resolved, do we have a change of heart :) ?

I think, this question is very synonymous to filtering of autocomplete list - using `.startsWith()` or `.includes()`. Personally I'd prefer .includes() way, hence my bending towards attachment#8866834 [details] :)

I'll go ahead with Tim's preference anyways..

Comment 12

[Ruturaj Vartak]

Attachment: fix-1364092-2.patch

- Applied fix-1364097-3.patch, and updated the code
- updated test cases

This fix has an issue, by just typing "-" all negative flags popup. Since we don't positive matches unless we type in a char, I'm not sure what should be done. I'm adding another patch with ignoring a prefix (ie. considering it as empty value)

I've uploaded this fix, so that both UX patches are available for review.

Comment 13

[Ruturaj Vartak]

Attachment: fix-1364092-3.patch

- this patch has ignorePrefixForAutocomplete as a prop
- also updated test-cases for ignorePrefix

Comment 14

[Tim Nguyen :ntim]

(In reply to Ruturaj Vartak from comment #11)
> Julian, Tim,
> 
> Tim, I know you already feel that you don't prefer attachment#8866834 [details]
> [details], and you wanted user to explicitly to start with "-". Now with
> bug#1364097 getting resolved, do we have a change of heart :) ?
> 
> I think, this question is very synonymous to filtering of autocomplete list
> - using `.startsWith()` or `.includes()`. Personally I'd prefer .includes()
> way, hence my bending towards attachment#8866834 [details] :)
> 
> I'll go ahead with Tim's preference anyways..

I would match Chrome's behaviour:

Typing "-" displays all negative filtering items.

Comment 15

[Ruturaj Vartak]

Tim / Julian,

attachment#8869676 [details] [diff] [review] addresses Tim's suggestion / Chrome behavior.

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8869676 [details] [diff] [review]
fix-1364092-2.patch

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

::: devtools/client/netmonitor/src/components/toolbar.js
@@ +95,5 @@
> +    // Setup autocomplete list
> +    let negativeAutocompleteList = FILTER_FLAGS.map((item) => `-${item}`);
> +    let autocompleteList = [...FILTER_FLAGS, ...negativeAutocompleteList]
> +      .map((item) => `${item}:`)
> +      .sort((a, b) => {

This extra sorting should really be unnecessary, since negative filters will never appear in combination with normal filters.

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8869676 [details] [diff] [review]
fix-1364092-2.patch

Don't hesitate to set earlier versions of your patches as obsolete when you upload new versions :)

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8869676 [details] [diff] [review]
fix-1364092-2.patch

ok my bad, these patches are just two different proposals.

Comment 19

[Julian Descottes [:jdescottes]]

Could you rebase your patches on a recent changeset of central, I can't apply them here.

I agree with ntim about show all negative filters when the user types "-", I don't think we need the extra complexity here. Let's stick with the patch version 2?

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8869676 [details] [diff] [review]
fix-1364092-2.patch

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

As mentioned in my previous comment, let's go for this patch, and roll back the sort() changes for now.

::: devtools/client/netmonitor/src/components/toolbar.js
@@ +95,5 @@
> +    // Setup autocomplete list
> +    let negativeAutocompleteList = FILTER_FLAGS.map((item) => `-${item}`);
> +    let autocompleteList = [...FILTER_FLAGS, ...negativeAutocompleteList]
> +      .map((item) => `${item}:`)
> +      .sort((a, b) => {

After thinking a bit more about this, since we can afford it now, I would keep the sort() in the autocomplete-popup. 

My reasoning for this is that most of the time you want suggestions to be sorted alphabetically when you use an autocomplete. So if the dedicated widget (autocomplete-popup) can do it by default, it should. 

If at some point we need a more complex sorting, we could make the sort function a Prop of the autocomplete popup. But no need to do this for now.

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8869677 [details] [diff] [review]
fix-1364092-3.patch

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

Let's go for version 2.

Comment 22

[Ruturaj Vartak]

Attachment: fix-1364092-4.patch

- Merged latest master
- Simple negative filtering
- sort remains in automcomplete
- no changes to test cases

Comment 23

[Julian Descottes [:jdescottes]]

Comment on attachment 8869895 [details] [diff] [review]
fix-1364092-4.patch

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

Great! Thanks for updating the patch, and sorry we went a bit back and forth with this one.
I will push the patch to inbound once try is green (https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6852fc40e0383598843d0c4699ad0a1854dd259)

One small comment, even though ntim gives feedback on the patch, since he is not flagged for review I'm not sure he wants to be in the r= list.
Saying that because I see he removed himself manually from the patch that landed for Bug 1364097.

Comment 24

[Tim Nguyen :ntim]

(In reply to Julian Descottes [:jdescottes] from comment #23)
> One small comment, even though ntim gives feedback on the patch, since he is
> not flagged for review I'm not sure he wants to be in the r= list.
> Saying that because I see he removed himself manually from the patch that
> landed for Bug 1364097.

I don't mind being in the list, but "r=XX" means patch has been r+ed by XX, so I'm just respecting notations here :) There's a f= flag for feedback.

Comment 25

[Ruturaj Vartak]

Thanks a lot guys.

Julian, its ok to go back and forth, I get to use some git commands :)

I simply add all guys who've actively suggested / commented on the bug for r= values. Please let me know if there is anything that I can lookup for such notations, conventions, etc.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec318d4524d5
Added negative filtering for Network Monitor autocomplete. r=jdescottes

Comment 27

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

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

Comment 28

[Md. Majedul islam]

I have reproduced this bug with Nightly 55.0a1 (2017-05-17) on Ubuntu 16.04, 64 bit!

The fix is now verified on Latest Nightly.

Build ID 	20170601100220
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170531]