If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Hide network monitor autocomplete when the input is empty

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Netmonitor
P3
normal
VERIFIED FIXED
5 months ago
4 months ago

People

(Reporter: ntim, Assigned: Ruturaj Vartak)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 months ago
Should autocomplete be shown when the input is empty? if so, do we want to add a header/footer ?
Priority: -- → P3
Blocks: 1364092
(Assignee)

Comment 1

4 months 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.

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)
(Assignee)

Comment 3

4 months ago
Created attachment 8869107 [details] [diff] [review]
fix-1364097-1.patch

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

Updated

4 months ago
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)
(Assignee)

Comment 5

4 months ago
Created attachment 8869487 [details] [diff] [review]
fix-1364097-2.patch

- 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+

Comment 7

4 months ago
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
(Reporter)

Updated

4 months 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
(Assignee)

Comment 8

4 months ago
Created attachment 8869661 [details] [diff] [review]
fix-1364097-3.patch

- Removed unwanted parentheses.

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

[bugday-20170531]
(Reporter)

Updated

4 months ago
Status: RESOLVED → VERIFIED
(Reporter)

Updated

4 months ago
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.