1364097 - Hide network monitor autocomplete when the input is empty

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[Tim Nguyen :ntim]

Should autocomplete be shown when the input is empty? if so, do we want to add a header/footer ?

Comment 1

[Ruturaj Vartak]

Tim, Julian,

- Should we invoke the autocomplete if atleast one char is typed-in ? (based on some comments in bug#1354504
- Header/Footer - Tim if you give an idea what it looks like.. Did you suggest something like optgroup, options in select box? - Perhaps we could just show Main bucket items, ie. "Headers", "URL", something like that? I'm not sure how that'd help for user to understand what headers are filterable even more so what prefix strings to use.

Thanks

Comment 2

[Julian Descottes [:jdescottes]]

Let's simply go for the simple solution: the autocomplete should only be shown if there is one character at least. In this case, no header/footer is needed.

Comment 3

[Ruturaj Vartak]

Attachment: fix-1364097-1.patch

- checked for input value existence before autocomplete
- updated test cases

Comment 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8869107 [details] [diff] [review]
fix-1364097-1.patch

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

Thanks for the patch, Ruturaj! 
A few minor comments, let's see another version addressing them.

The commit message should not just repeat the bug summary, it should explain what the code change is.
So here "Bug 1364097 - do not show the autocomplete popup when input is empty. r=..."

::: devtools/client/shared/components/search-box.js
@@ +170,4 @@
>          hidden: value == "",
>          onClick: this.onClearButtonClick
>        }),
> +      autocompleteList.length > 0 && this.state.focused && value &&

Would extract this to a variable as it starts to grow a bit complicated. Not a big fan of simply checking "value" which is a string. Maybe `value == ""` (same as what is used above for show the clear icon.

::: devtools/client/shared/components/test/mochitest/test_searchbox-with-autocomplete.html
@@ +66,3 @@
>  
>      $(".devtools-searchinput").focus();
>      await forceRender(component); // Wait for state update

for completeness, can we check here that the list is still not rendered?

Comment 5

[Ruturaj Vartak]

Attachment: fix-1364097-2.patch

- used a variable `showAutocomplete`
- added test as suggested

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8869487 [details] [diff] [review]
fix-1364097-2.patch

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

Looks good to me thanks!

Pushed your changeset to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aaf54ade4ec2bba4e03e024298133342200c626
If you want to address the nit, you can set r+ on the new patch directly.

Let's wait for try before adding the checkin-needed keyword on the bug.
Feel free to resume your patches on the other bugs blocked by this one :) (using this patch as a starting point)

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

nit: parentheses not needed here.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2184331d4ac0
Do not show the autocomplete popup when input is empty. r=jdescottes

Comment 8

[Ruturaj Vartak]

Attachment: fix-1364097-3.patch

- Removed unwanted parentheses.

Thanks Julian for all the help.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2184331d4ac0

Comment 10

[Mahfuza Humayra Mohona]

I have reproduced this bug on Nightly 55.0a1 (2017-05-17)on Windows 10 (64 bit).

This bug's fix is verified on Latest Nightly 55.0a1.  


Build ID 	: 20170601030206 
User Agent 	: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170531]