Hide network monitor autocomplete when the input is empty

VERIFIED FIXED in Firefox 55


2 years ago
7 months ago


(Reporter: ntim, Assigned: ruturaj)


Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)



(1 attachment, 2 obsolete attachments)



2 years ago
Should autocomplete be shown when the input is empty? if so, do we want to add a header/footer ?
Priority: -- → P3

Comment 1

2 years ago
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.

Flags: needinfo?(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.
Flags: needinfo?(jdescottes)

Comment 3

2 years ago
Created attachment 8869107 [details] [diff] [review]

- checked for input value existence before autocomplete
- updated test cases
Attachment #8869107 - Flags: review?(jdescottes)


2 years ago
Assignee: nobody → ruturaj
Comment on attachment 8869107 [details] [diff] [review]

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?
Attachment #8869107 - Flags: review?(jdescottes)

Comment 5

2 years ago
Created attachment 8869487 [details] [diff] [review]

- used a variable `showAutocomplete`
- added test as suggested
Attachment #8869107 - Attachment is obsolete: true
Attachment #8869487 - Flags: review?(jdescottes)
Comment on attachment 8869487 [details] [diff] [review]

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.
Attachment #8869487 - Flags: review?(jdescottes) → review+

Comment 7

2 years ago
Pushed by ntim.bugs@gmail.com:
Do not show the autocomplete popup when input is empty. r=jdescottes


2 years ago
Summary: Figure out whether network monitor autocomplete should be shown when the input is empty → Hide network monitor autocomplete when the input is empty

Comment 8

2 years ago
Created attachment 8869661 [details] [diff] [review]

- Removed unwanted parentheses.

Thanks Julian for all the help.
Attachment #8869661 - Flags: review+
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Attachment #8869487 - Attachment is obsolete: true
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



2 years ago


2 years ago
status-firefox55: fixed → verified


7 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.