Closed Bug 1439242 Opened 2 years ago Closed 2 years ago

Headers filter broken

Categories

(DevTools :: Netmonitor, defect, P2)

58 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox58 wontfix, firefox59 fixed, firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: ruturaj, Assigned: ruturaj, Mentored)

References

Details

(Keywords: good-first-bug, regression, Whiteboard: good-first-bug)

Attachments

(2 files, 2 obsolete files)

Attached image Lauchpad screenshot
Filtering of Headers in the Headers panel is broken. Observed in latest master (5536f71c38)

STR:
1. Open http://firefox-dev.tools/debugger-examples/ or any page
2. Click any resource to open the side panel
3. Click Headers tab of side panel
4. Search "server" in the "Filter Headers" search box

Observed: Nothing changes in the headers list.

Expected: To see only "server" header and any other headers having "server" in Header name or its value
The problem is across all components using `shared/components/tree/TreeView.js`

I have a simple patch ready if we can agree if its a bug, Also I plan to add test cases..
Flags: needinfo?(ntim.bugs)
Thanks for the report!
Yes, this is a bug.

Honza
Has STR: --- → yes
Priority: -- → P2
Yep, as Honza said, this looks like a bug. I wonder for how long this was broken though.
Flags: needinfo?(ntim.bugs)
The problem is caused by CSS rule in NetworkDetailsPanel.css

.network-monitor .tree-container .treeTable tr {
  display: block;
  position: relative;
}

https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/devtools/client/netmonitor/src/assets/styles/NetworkDetailsPanel.css#100

.. this rule overrides, the one that is responsible for hiding headers in TreeView.css

/* Filtering */
.treeTable .treeRow.hidden {
  display: none;
}

https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/devtools/client/shared/components/tree/TreeView.css#85

One solution is increasing priority of the rule in TreeView.css

/* Filtering */
.treeTable .treeRow.hidden {
  display: none !important;
}

And we also need a test to avoid regressions in the future.

The following existing test for sorting headers might be used as an inspiration for creating a new one: browser_net_headers_filtered.js

https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/devtools/client/netmonitor/test/browser_net_headers_sorted.js#11


Honza
Mentor: odvarko
Whiteboard: good-first-bug
Assignee: nobody → ruturaj
Attached patch WIP-1439242-1.patch (obsolete) — Splinter Review
WIP Patch

- I'm not able to `await waitForDOM(document, ".treeRow.hidden");` Hence I've used wait for the component to update, not sure why its happening that way, since in previous test cases never had that problem.
- Also it shows 3 test cases passed, while there are only 2 `is`. The code doesn't show anything else, donno if some required/embedded scripts are doing it.
Attachment #8952295 - Flags: review?(ntim.bugs)
Comment on attachment 8952295 [details] [diff] [review]
WIP-1439242-1.patch

I don't have time to review this, but you might want to try waitUntil, or specifying length to waitForDOM. Honza might have time to help you with this.
Attachment #8952295 - Flags: review?(ntim.bugs) → review?(odvarko)
Comment on attachment 8952295 [details] [diff] [review]
WIP-1439242-1.patch

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

Thanks for working on this!

See my inline comments.

Honza

::: devtools/client/netmonitor/test/browser_net_headers_filter.js
@@ +21,5 @@
> +  let wait = waitForNetworkEvents(monitor, 1);
> +  tab.linkedBrowser.reload();
> +  await wait;
> +
> +  wait = waitForDOM(document, ".headers-overview");

Using `waitUntil` is a bit more consistent in this case:
wait = waitUntil(() => document.querySelector(".headers-overview"));

@@ +34,5 @@
> +
> +  document.querySelectorAll(".devtools-filterinput")[1].focus();
> +  EventUtils.synthesizeKey("con", {});
> +  // await waitForDOM(document, ".treeRow.hidden");
> +  await new Promise(r => setTimeout(r, 900));

`waitUntil` should work just fine
await waitUntil(() => document.querySelector(".treeRow.hidden"));
Attachment #8952295 - Flags: review?(odvarko)
Attached patch fix-1439242-1.patch (obsolete) — Splinter Review
- used waitUntil as suggested.
Attachment #8952295 - Attachment is obsolete: true
Attachment #8952676 - Flags: review?(odvarko)
Comment on attachment 8952676 [details] [diff] [review]
fix-1439242-1.patch

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

Looks good to me now!

R+ assuming try is green.

Thanks!
Honza
Attachment #8952676 - Flags: review?(odvarko) → review+
Comment on attachment 8952676 [details] [diff] [review]
fix-1439242-1.patch

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

::: devtools/client/netmonitor/test/browser_net_headers_filter.js
@@ +34,5 @@
> +
> +  document.querySelectorAll(".devtools-filterinput")[1].focus();
> +  EventUtils.synthesizeKey("con", {});
> +  // await waitForDOM(document, ".treeRow.hidden");
> +  // await new Promise(r => setTimeout(r, 900));

nit: Remove these 2 comments
Thanks Tim ☺
- removed unnecessary comment as Tim suggested.
Attachment #8952676 - Attachment is obsolete: true
Attachment #8952950 - Flags: review?(odvarko)
Comment on attachment 8952950 [details] [diff] [review]
fix-1439242-2.patch

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

(In reply to Ruturaj Vartak from comment #13)
> Created attachment 8952950 [details] [diff] [review]
> fix-1439242-2.patch
> 
> - removed unnecessary comment as Tim suggested.
Ah, great.

Honza
Attachment #8952950 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/945321756118
Headers filter in netmonitor broken. r=honza.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/945321756118
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
One-liner patch with an automated test. Seems like a good candidate for uplift to 59 :)
Flags: needinfo?(odvarko)
Flags: in-testsuite+
Comment on attachment 8952950 [details] [diff] [review]
fix-1439242-2.patch

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: HTTP headers filter doesn't work
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: It's only for developers and only one CSS line changed.
[String changes made/needed]: n/a
Flags: needinfo?(odvarko)
Attachment #8952950 - Flags: approval-mozilla-beta?
Comment on attachment 8952950 [details] [diff] [review]
fix-1439242-2.patch

One-liner CSS change with a new automated test for the bug in question. Approved for 59b13.
Attachment #8952950 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.