Closed Bug 1364097 Opened 5 years ago Closed 5 years ago

Hide network monitor autocomplete when the input is empty

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(firefox55 verified)

VERIFIED FIXED
Firefox 55
Tracking Status
firefox55 --- verified

People

(Reporter: ntim, Assigned: ruturaj)

References

Details

Attachments

(1 file, 2 obsolete files)

Should autocomplete be shown when the input is empty? if so, do we want to add a header/footer ?
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
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)
Attached patch fix-1364097-1.patch (obsolete) — Splinter Review
- checked for input value existence before autocomplete
- updated test cases
Attachment #8869107 - Flags: review?(jdescottes)
Assignee: nobody → ruturaj
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?
Attachment #8869107 - Flags: review?(jdescottes)
Attached patch fix-1364097-2.patch (obsolete) — Splinter 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]
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.
Attachment #8869487 - Flags: review?(jdescottes) → review+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2184331d4ac0
Do not show the autocomplete popup when input is empty. r=jdescottes
Summary: Figure out whether network monitor autocomplete should be shown when the input is empty → Hide network monitor autocomplete when the input is empty
- Removed unwanted parentheses.

Thanks Julian for all the help.
Attachment #8869661 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2184331d4ac0
Status: NEW → RESOLVED
Closed: 5 years ago
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

[bugday-20170531]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.