1227746 - Hide side panels when related request is filtered out

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect)

Description

[Sebastian Zartner [:sebo]]

When the selected request is filtered out by using the search filter or the filter buttons, the side panels should be closed.

Test case:
1. On this page open the Network panel and reload the page
2. Click the 'CSS' filter button
3. Click the first request
   => The side panels open showing detailed request info.
4. Click the 'JS' filter button and click the 'CSS' filter button to deactivate it

=> The side panels are still shown while the request is not listed anymore.

Sebastian

Comment 1

[raibaz]

I'd like to pick this up as my first contribution, if possible :)

Comment 2

[raibaz]

Attachment: bug-1227746.diff

Comment 3

[raibaz]

(It's my first patch submission, sorry in advance for any mistakes i made)

Comment 4

[Luna Phipps-Costin]

I don't think it should be. If I am looking for a new JS file, maybe I'll glance back at the CSS information before I decide which JS to click on. What's the advantage of closing it? Seems like it'd be clunky to make it disappear just because the menu has navigated away from the original URL of the panel.

Comment 5

[Sebastian Zartner [:sebo]]

(In reply to Christopher Phonis-Phine from comment #4)
> I don't think it should be. If I am looking for a new JS file, maybe I'll
> glance back at the CSS information before I decide which JS to click on.
> What's the advantage of closing it? Seems like it'd be clunky to make it
> disappear just because the menu has navigated away from the original URL of
> the panel.

There is no relation between the shown requests and the info in the sidebar, as the selected request to which the information belongs is hidden. So the advantage is that it avoids confusion when the sidebar is closed.

@raibaz, It looks like your patch was missed, so I'm now asking Honza for review.

Sebastian

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8873505 [details] [diff] [review]
bug-1227746.diff

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

Thanks for working on this!

See my inline comment.

However, I think that we should use different approach.

1) see reducers/ui.js and the way how the side bar is closed if the request list is cleared. Specifically see the CLEAR_REQUESTS case in the reducer. 

2) We need to do similar things for the case when a filter (filter button or filter text) changes and the selected request is not visible anymore. I think that we don't have to change the selection, so if the user changes the filter and the selected request appears again the side bar can open automatically.

The point is that the logic should happen at reducer level. The UI will reflect it automatically.

3) It looks like we need something like isSelectedRequestVisible (or more generic isRequestVisible)

4) The check should probably happen when any of the following (filter changed) actions are fired:

ENABLE_REQUEST_FILTER_TYPE_ONLY:
TOGGLE_REQUEST_FILTER_TYPE:
SET_REQUEST_FILTER_TEXT:

(see the reducer/filters.js reducer)

5) See also how to get list of visible requests in selectors/requests.js => `getDisplayedRequests`

Hope this helps.

Honza

::: devtools/client/netmonitor/src/components/toolbar.js
@@ +54,5 @@
>        return;
>      }
> +    if(this.props.networkDetailsOpen) {
> +      this.props.toggleNetworkDetails();
> +    }

This hides the details side-bar after every filter change - even if the selected request is still visible.

Comment 7

[DeanTing]

Hi Honza, Ricky, I would like to pick this bug.

Comment 8

[DeanTing]

Attachment: Bug 1227746 - Hide side panels when related request is filtered out.

Review commit: https://reviewboard.mozilla.org/r/207740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207740/

Comment 9

[DeanTing]

Hi Honza, Ricky,
I have send review request.
Please help me to review my codes.
Thank you.

There are some comments, see as below.

1) Seems we can't modify reducer part to attain the purpose, hide the network detail panel when selected request is filtered out.

The selected information is stored in state.requests.
The control configuration(networkDetailsOpen) is stored in state.ui.
We need state.requests to know selected request is display or not, then change the state.ui.networkDetailIsOpen.
We can't use state.requests and state.ui in reducer layer at the same time, because they are independent when processing reducer layer.

Resolved method:
Modify selector part to check selected request is visible or not.

2) when network detail panel is hidden, the selected request will change to be unselected.

I haven't trace the code to find where is this workflow.
if we want to do the things that user changes the filter and the selected request appears again the side bar can open automatically, we need to modify this part which is processing when network detail panel is hidden, the selected request will change to be unselected.

Dean

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8936996 [details]
Bug 1227746 - Hide side panels when related request is filtered out.

https://reviewboard.mozilla.org/r/207740/#review215046

Hey DeanTing,

Thanks for all the work! I've verfifed this patch works as expected in my local build.

I've some concerns about the approach which I told you eailer, I believe we can do some tweaks and simplification without adding `toggleNetworkDetailsBySelectedRequest()` for handling this edge case. Please see below:

The question is I suspect that `toggleNetworkDetailsBySelectedRequest()` might only use in filtering case. When user starts filtering reqeusts, NetworkDetailsPanel could be closed due to the request result, but there is no other chances to make that open again. Therefore, introducing a `toggle...` API doesn't make sense in this case. We could just focus on closing NetworkDetailsPanel in the right condition.

I'd suggest that keep your new selector `isSelectedRequestVisible` as is, and then use it in components/Toolbar.js to determine whether the filter behavior will close the NetworkDetailsPanel.

It might look like:

toggleRequestFilterType(evt) {
  ...
  const { isSelectedRequestVisible, toggleNetworkDetails } = this.props;
  // If selected request is invisible after filtering, we close NetworkDetailsPanel
  if (!isSelectedRequestVisible) {
    toggleNetworkDetails(false);
  }
}

same as in updateRequestFilterText(text) {...}

Because this is not the high risk issue, I believe it's okay you just set reviewer to me.

::: devtools/client/netmonitor/src/actions/ui.js:20
(Diff revision 1)
>    TOGGLE_COLUMN,
>    WATERFALL_RESIZE,
>  } = require("../constants");
> +const {
> +  isSelectedRequestVisible,
> +}= require("../selectors/index");

nit: missing whitespace before `= require(...)`

::: devtools/client/netmonitor/src/actions/ui.js:172
(Diff revision 1)
>    toggleColumn,
>    toggleNetworkDetails,
>    togglePersistentLogs,
>    toggleBrowserCache,
>    toggleStatistics,
> +  toggleNetworkDetailsBySelectedRequest,

nit: move it below `toggleNetworkDetails` (alphabetical order)

::: devtools/client/netmonitor/src/selectors/requests.js:136
(Diff revision 1)
> +  ({ selectedId }, displayedRequests) => {
> +    let arr = displayedRequests;
> +    return arr.some(r => r.id == selectedId)
> +  }

Let's simplify this code as below:

```
({ selectedId }, displayedRequests) =>
  displayedRequests.some(r => r.id === selectedId)
```

nit: using === instead of == for comparing two items.

::: devtools/client/netmonitor/src/selectors/requests.js:140
(Diff revision 1)
> +  getDisplayedRequests,
> +  ({ selectedId }, displayedRequests) => {
> +    let arr = displayedRequests;
> +    return arr.some(r => r.id == selectedId)
> +  }
> +)

nit: missing tailing colon `;`

Comment 11

[DeanTing]

Comment on attachment 8936996 [details]
Bug 1227746 - Hide side panels when related request is filtered out.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207740/diff/1-2/

Comment 12

[DeanTing]

Comment on attachment 8936996 [details]
Bug 1227746 - Hide side panels when related request is filtered out.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207740/diff/2-3/

Comment 13

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8873505 [details] [diff] [review]
bug-1227746.diff

># HG changeset patch
># User Mattia Tommasone <[e-mail]>
># Date 1496335508 -7200
>#      Thu Jun 01 18:45:08 2017 +0200
># Branch bug-1227746
># Node ID 5fa5c63cb25fef051950b4c6dec87d1b92c1cc02
># Parent  a6df0f5e3000f7992c9cf4e0161887a8f465a906
>Bug 1227746 - Toggled the network details panel if it is open when a new filter is set
>
>diff --git a/devtools/client/netmonitor/src/components/toolbar.js b/devtools/client/netmonitor/src/components/toolbar.js
>--- a/devtools/client/netmonitor/src/components/toolbar.js
>+++ b/devtools/client/netmonitor/src/components/toolbar.js
>@@ -53,6 +53,9 @@ const Toolbar = createClass({
>     if (evt.type === "keydown" && (evt.key !== "" || evt.key !== "Enter")) {
>       return;
>     }
>+    if(this.props.networkDetailsOpen) {
>+      this.props.toggleNetworkDetails();
>+    }
>     this.props.toggleRequestFilterType(evt.target.dataset.key);
>   },
> 
>diff --git a/devtools/client/netmonitor/test/browser.ini b/devtools/client/netmonitor/test/browser.ini
>--- a/devtools/client/netmonitor/test/browser.ini
>+++ b/devtools/client/netmonitor/test/browser.ini
>@@ -168,3 +168,4 @@ skip-if = true # TODO: fix the test
> [browser_net_timing-division.js]
> [browser_net_truncate.js]
> [browser_net_persistent_logs.js]
>+[browser_net_close_request_on_filter.js]
>diff --git a/devtools/client/netmonitor/test/browser_net_close_request_on_filter.js b/devtools/client/netmonitor/test/browser_net_close_request_on_filter.js
>new file mode 100644
>--- /dev/null
>+++ b/devtools/client/netmonitor/test/browser_net_close_request_on_filter.js
>@@ -0,0 +1,35 @@
>+/* Any copyright is dedicated to the Public Domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+"use strict";
>+
>+/**
>+ * Tests if changing the request filter closes the request detail panel
>+ */
>+
>+const REQUESTS = [
>+  { url: "sjs_content-type-test-server.sjs?fmt=html&res=undefined&text=Sample" },
>+  { url: "sjs_content-type-test-server.sjs?fmt=html&res=undefined&text=Sample" +
>+         "&cookies=1" },
>+  { url: "sjs_content-type-test-server.sjs?fmt=css&text=sample" }
>+];
>+
>+add_task(function* () {
>+	let { monitor } = yield initNetMonitor(FILTERING_URL);
>+	let { document, store, windowRequire } = monitor.panelWin;
>+  info("Starting test... ");
>+
>+  let Actions = windowRequire("devtools/client/netmonitor/src/actions/index");
>+
>+  let waitNetwork = waitForNetworkEvents(monitor, REQUESTS.length);
>+  loadCommonFrameScript();
>+  yield performRequestsInContent(REQUESTS);
>+  yield waitNetwork;
>+
>+	let toggleButton = document.querySelector(".network-details-panel-toggle");
>+
>+  EventUtils.sendMouseEvent({ type: "mousedown" }, document.querySelectorAll(".request-list-item")[0]);
>+  store.dispatch(Actions.setRequestFilterText("css"));
>+  	
>+  is(!!document.querySelector(".network-details-panel"), false, "The details pane should be hidden when a filter is applied.");
>+});

Comment 14

[DeanTing]

Comment on attachment 8936996 [details]
Bug 1227746 - Hide side panels when related request is filtered out.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207740/diff/3-4/

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8936996 [details]
Bug 1227746 - Hide side panels when related request is filtered out.

https://reviewboard.mozilla.org/r/207740/#review215118

Thanks! I like this patch. It looks better and concise now. All issues are fixed and I can see there is no new issue after update.

Please address remaining linter (coding style) errors before landing your patch.

Comment 16

[DeanTing]

Comment on attachment 8936996 [details]
Bug 1227746 - Hide side panels when related request is filtered out.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207740/diff/4-5/

Comment 17

[DeanTing]

Comment on attachment 8936996 [details]
Bug 1227746 - Hide side panels when related request is filtered out.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207740/diff/5-6/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9096413092d0
Hide side panels when related request is filtered out.  r=rickychien

Comment 19

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9096413092d0

Comment 20

[Sebastian Zartner [:sebo]]

I can confirm that it's working as expected in Nightly 59.0a1 2017-12-26.

Thank you for the patch, DeanTing!

Sebastian