Cristian Fogel [:cfogel] Reporter
	Description • 6 years ago

[Affected versions]:
- Firefox 60.0.1, 61.0b10, 62.0a1 

[Affected platforms]:
- Windows 10x64, Ubuntu 16.04LTS, MacOS10.13.4

[Steps to reproduce]:
1. Launch Firefox;
2. Navigate to any website;
3. Press the F12 key;
4. Click on the Network monitor tab;
5. For any method right click in the right-most part of the row
*(in the area where the Request Details tab would appear)

[Expected result]:
- Context menu for method is displayed. 

[Actual result]:
- Request tab is shown in the right section.

[Additional notes]:
- Attached screenshot with the issue; 
- on MacOS and Ubuntu the context menu is not displayed if right-click(ed) in the area where the tab would be;
- on Windows the first right-click brings up the tab, closing and then repeating the steps also displays the context menu over the tab

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 1 • 6 years ago

Thanks for the report!

I can confirm the issue on Win10, Nightly.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 2 • 6 years ago

More information for anybody interested in fixing this bug:

There are two (UX) things that need to be changed

1) Right clicking on a request in the Network panel should *not* change the selection. This fixes the reported problem, since the side-bar won't open.

2) Context menu actions should be wired to the clicked request, *not* the current selection. 

---

add #1) Clicking a request in the list is handled by `RequestListContent` component:
https://searchfox.org/mozilla-central/rev/3737701cfab93ccea04c0e9cab211ad10f931d87/devtools/client/netmonitor/src/components/RequestListContent.js#335

The new implementation should be something like as follows:

```js
if (event.button == 0) {
  event.preventDefault();
  event.stopPropagation();
  dispatch(Actions.selectRequest(id));
}
```

add #2) Context menu is opened in `RequestListContent` component as well:
https://searchfox.org/mozilla-central/rev/3737701cfab93ccea04c0e9cab211ad10f931d87/devtools/client/netmonitor/src/components/RequestListContent.js#238

Note the `selectedRequest` props that is passed to `this.contextMenu.open` it needs to be changed. The method needs to figure out the *clicked* request and use it instead.

Every request item in the list is represented by <div> element:
<div class="request-list-item odd" data-id="server2.conn2.child1/netEvent73">...</div>

The <div> has `data-id` attribute that links to the corresponding request object. The <div> should be figured out from clicked `event.target` (it's a parent).

Further, there is an existing selector `getDisplayedRequestById` that returns request object by id:
https://searchfox.org/mozilla-central/rev/3737701cfab93ccea04c0e9cab211ad10f931d87/devtools/client/netmonitor/src/selectors/requests.js#143

We also need to have/update test coverage for this.

Honza

	Fabien Casters [:vaga]
	Comment 3 • 6 years ago

Hi,

I would like to work on it.

Fabien

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 4 • 6 years ago

Excellent, assigned to you!
Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 6 • 6 years ago
mozreview-review

Comment on attachment 8985870 [details]
Bug 1466598 - [Network-Monitor] Right click on method enables request details tab

https://reviewboard.mozilla.org/r/251372/#review258434

Thanks for working on this!

The patch looks good to me, but please see my inline comments.

Two additional comments:

1) Edit and Resend context menu action is broken now (the form doesn't open)
2) We also need a test covering this new functionality.

Honza

::: devtools/client/netmonitor/src/components/RequestListContent.js:251
(Diff revision 1)
> +
>      evt.preventDefault();
> -    const { selectedRequest, displayedRequests } = this.props;
>  
> +    const { displayedRequests } = this.props;
> +    const selectedRequest = displayedRequests.find(r => r.id === itemId);

The same code (to get request from target) is used in `onHover` method above. Can you please extract it and introduce new `getRequestFromTarget(evt.target)` method?

::: devtools/client/netmonitor/src/components/RequestListContent.js:262
(Diff revision 1)
> -        cloneSelectedRequest,
> +        selectedRequest,
>          openStatistics,
>        });
>      }
>  
>      this.contextMenu.open(evt, selectedRequest, displayedRequests);

The method maset return a value to pass consistent-return ESlint check.

::: devtools/client/netmonitor/test/browser_net_copy_as_curl.js
(Diff revision 1)
>      const { document } = monitor.panelWin;
>  
>      const items = document.querySelectorAll(".request-list-item");
> -    EventUtils.sendMouseEvent({ type: "mousedown" }, items[items.length - 1]);
> -    EventUtils.sendMouseEvent({ type: "contextmenu" },
> +    EventUtils.sendMouseEvent({ type: "mousedown" }, items[0]);
> +    EventUtils.sendMouseEvent({ type: "contextmenu" }, items[items.length - 1]);
> -      document.querySelectorAll(".request-list-item")[0]);

Why is this needed?

	Fabien Casters [:vaga]
	Comment 7 • 6 years ago
mozreview-review-reply

Comment on attachment 8985870 [details]
Bug 1466598 - [Network-Monitor] Right click on method enables request details tab

https://reviewboard.mozilla.org/r/251372/#review258434

Thanks!

Today, I work on it.

> The same code (to get request from target) is used in `onHover` method above. Can you please extract it and introduce new `getRequestFromTarget(evt.target)` method?

Ok but I need this.props.requests to retrieve request.

I should create function who returns an another like
`getRequestFromTarget = (state) => (target) => // Do stuff`

Is it ok ?

> Why is this needed?

Because, now, the request object is selected by the contextmenu event. Before, it was with the last click event.

I can remove click event in this test and write this behaviour in the new test.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 8 • 6 years ago

(In reply to Fabien Casters from comment #7)
> > The same code (to get request from target) is used in `onHover` method above. Can you please extract it and introduce new `getRequestFromTarget(evt.target)` method?
> 
> Ok but I need this.props.requests to retrieve request.
This is what I had in mind:

getRequestFromTarget(target, requests) {
  const element = target.closest(".request-list-item");
  if (!element) {
    return null;
  }

  const id = element.dataset.id;
  if (!id) {
    return null;
  }

  return requests.find(r => r.id === id);
}

And consequently you can use it in onContextMenu as follows:

const selectedRequest = this.getRequestFromTarget(evt.target, this.props.displayedRequests);
if (!requestItem) {
  return;
}

...and in onHover:

const requestItem = this.getRequestFromTarget(target, this.props.displayedRequests);
if (!requestItem) {
  return false;
}

Honza

	Fabien Casters [:vaga]
	Comment 10 • 6 years ago
mozreview-review-reply

Comment on attachment 8985870 [details]
Bug 1466598 - [Network-Monitor] Right click on method enables request details tab

https://reviewboard.mozilla.org/r/251372/#review258434

Edit and Resend context menu action works again. It was not easy to solve this but it works.

Maybe, I should:
 - rename `cloneSelectedRequest()` function to `cloneRequest()`
 - rename `CLONE_SELECTED_REQUEST` to `CLONE_REQUEST` because now the action takes an id parameter
 - remove `cloneSelectedRequest` prop and call `dispatch(Actions.cloneRequest(id))` just in button events. (I'm not sure about that)

I'm working on test covering.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 11 • 6 years ago

(In reply to Fabien Casters from comment #10)
> Comment on attachment 8985870 [details]
> Bug 1466598 - [Network-Monitor] Right click on method enables request
> details tab
> 
> https://reviewboard.mozilla.org/r/251372/#review258434
> 
> Edit and Resend context menu action works again. It was not easy to solve
> this but it works.
When I right click on a request and pick "edit and resend" a new request
is created & selected, but the side-bar doesn't open.

(I am on win10)

Is this working for you?

Honza

	Fabien Casters [:vaga]
	Comment 13 • 6 years ago

Oops. I forgot to change the UI reducer.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 15 • 6 years ago

Thanks for the update!

Unfortunately, edit and resend still doesn't work for me.

Here is my STR:

0) Open the Toolbox select the Network panel
1) Load google.com
2) Make sure no request is selected
3) Right click on the first document `/` request
4) Execute "edit and resend"
5) The request is cloned, but the side bar won't open

The important thing is that there is no selection in the panel.

Honza

	Fabien Casters [:vaga]
	Comment 16 • 6 years ago

It works for me. :/ I'm on MacOS.

I found an another bug. The panel is opened but the request header field is empty.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 17 • 6 years ago

(In reply to Fabien Casters from comment #16)
> It works for me. :/ I'm on MacOS.
Hm... I retested and it works for me now.
Maybe I used the old patch, not sure.
Sorry, for the false alarm here!

> I found an another bug. The panel is opened but the request header field is
> empty.
Yes, I can see the same problem.

Also, when pressing the Send button I am seeing the following error in the Browser Console:
TypeError: response.eventActor is undefined: sendCustomRequest/</<@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/actions/requests.js:74:9

It might be related.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 18 • 6 years ago
mozreview-review

Comment on attachment 8985870 [details]
Bug 1466598 - [Network-Monitor] Right click on method enables request details tab

https://reviewboard.mozilla.org/r/251372/#review262478

Just unassigning myself from the review, till there is new version.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 19 • 6 years ago

@stav: any progress on this bug? Can I help somehow?

Honza

	stavbarak
	Comment 20 • 6 years ago

I'm attaching the diff file so you'll see the changes we've made.
Please provide me with some feedback: are there any best (or better) practices? Did we do anything that we shouldn't have? Are there any tests to run?
Any information that might help me move forward with this will be appreciated.

Say all changes are okay, here are the errors left to fix after making those:

1. when right clicking and choosing "edit and resend" if the sidebar is not already opened, it does not open (it should)
2. context menu does not open with the keyboard context menu button

So, please review the changes and reply with some feedback.

	stavbarak
	Comment 21 • 6 years ago

Hi, sorry for the delay.
I attached the diff file so you'll see the changes made.
Please review attached file diff_22_10 provide me with some feedback.

Say all changes are okay, here are the errors left to fix after making those:

1. when right clicking and choosing "edit and resend" if the sidebar is not already opened, it does not open (it should)
2. context menu does not open with the keyboard context menu button

Let me know how you'd like me to move on from here.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 22 • 6 years ago

Thanks for the update!

I am seeing conflicts when applying the patch on latest m-c HEAD

Zapplying right-click-side-panel.patch
patching file devtools/client/netmonitor/src/actions/requests.js
Hunk #2 FAILED at 40
1 out of 3 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/actions/requests.js.rej
patching file devtools/client/netmonitor/src/reducers/requests.js
Hunk #2 FAILED at 118
1 out of 3 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/reducers/requests.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh right-click-side-panel.patch

Please fix and I'll review.

Honza

	stavbarak
	Comment 23 • 6 years ago

Hi,
This time I'm having difficulties creating the diff so I'll link you those two changed files, after merging and fixing the conflicts.
I still have the two small issues mentioned above, but please review the current changes and let me know if it's okay so far.

Changed files:
https://github.com/tomklino/gecko-dev/blob/1466598/devtools/client/netmonitor/src/actions/requests.js
https://github.com/tomklino/gecko-dev/blob/1466598/devtools/client/netmonitor/src/reducers/requests.js

I understand the rest didn't break?

Let me know.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 24 • 6 years ago

There is a conflict since Bug 1340100 modified the Requests reducer and actions. You need to make sure you are working with latest m-c, apply your patch on top and resolve the conflicts mentioned in comment #22.

For example this line causes a conflict:
https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/devtools/client/netmonitor/src/reducers/requests.js#135


In order to create a diff you can:

# export the latest commit (tip) from Mercurial
hg export tip > my-patch.patch

# export the latest patch in HG queues (qtip)
hg export qtip > my-patch.patch

# export specific revision
hg export 443077:5a123d345104 > my-patch.patch

You can use:
hg log 
.. to see commits and revision numbers

If you do the following in the current dir:
hg diff > my-patch.patch
...it stores all uncommitted changes.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 25 • 6 years ago

Comment on attachment 9019411 [details] [diff] [review]
diff_22_10

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

Please see my inline comments.

Thanks!
Honza

::: devtools/client/netmonitor/src/actions/requests.js
@@ +44,5 @@
> +    type: CLONE_SELECTED_REQUEST,
> +  }
> +}
> +
> +function cloneRequest(id) {

It would be great to have a comment explaining why we have this new action

@@ +52,5 @@
> +  };
> +}
> +
> +
> +function rightClickRequest(id) {

Same as above, a comment would be great.

::: devtools/client/netmonitor/src/components/RequestListContent.js
@@ +207,5 @@
> +  onMouseDown(e, id) {
> +    if (e.button === LEFT_MOUSE_BUTTON) {
> +      this.props.selectRequest(id)
> +    } else if(e.button === RIGHT_MOUSE_BUTTON) {
> +      console.log("RIGHT_MOUSE_BUTTON clicked")

This looks like forgotten log.

::: devtools/client/netmonitor/src/reducers/requests.js
@@ +194,5 @@
> +  }
> +
> +  const newRequest = {
> +    id: clonedRequest.id + "-clone",
> +    method: clonedRequest.method,

This cloning code is missing the `cause` field...

	stavbarak
	Comment 26 • 6 years ago

Here's the last diff with the issues fixed:
https://github.com/mozilla/gecko-dev/compare/master...tomklino:1466598

please review.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 27 • 6 years ago

Thanks for working on this!

Can you please:

1) Attach a patch as a file, so I can perform a review and comment on it. Or better, you might use Phabricator 

See also:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

2) I am seeing several conflicts when applying the patch. Please get the latest source (m-c) and rebase

Conflicts:

applying right-click-details.patch
patching file devtools/client/netmonitor/src/actions/requests.js
Hunk #2 FAILED at 40
1 out of 3 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/actions/requests.js.rej
patching file devtools/client/netmonitor/src/reducers/requests.js
Hunk #2 FAILED at 118
1 out of 3 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/reducers/requests.js.rej
patching file devtools/client/netmonitor/src/actions/requests.js
Hunk #1 FAILED at 44
Hunk #2 FAILED at 51
2 out of 2 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/actions/requests.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh right-click-details.patch


Thanks!
Honza

	stavbarak
	Comment 28 • 6 years ago

Please review the following diff.
Thank you!

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 29 • 6 years ago

Thanks for the update!

The patch-collision is fixed, but the "Edit and Resend" feature is broken now.

STR:
1) Right click a request
2) Pick "edit and resend" action
3) Click Send in the form that opens at the right side

There is an exception in the Browser console and the request is not resent.

TypeError: response.eventActor is undefined; can't access its "actor" property: sendCustomRequest/</<@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/actions/requests.js:98:9

TypeError: response.eventActor is undefined; can't access its "actor" property: sendCustomRequest/</<@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/actions/requests.js:98:9

Can you reproduce the issue on your machine?

Honza

	stavbarak
	Comment 30 • 6 years ago

This is what we discovered so far:
- clone request works okay, the problem is that before a request is selected, its object in redux does not contain the object "requestHeaders".
- we placed an error in updateRequest to see (from its stacktrace) from where it is being called with "requestHeaders"
- we found in request-utils(line 480) a call to connector.requestData(id, "requestHeaders") which eventually updates the request
- the above is called from the MonitorPanel component immediately when the request is selected

Here is how we think to proceed:
1st option is to call connector.requestData(id, "requestHeaders") when a user right clicks on a request, thus preparing it to be properly cloned if they continue to click on "Edit and Resend"
2nd option  is to call it right before cloning, when the user clicks on on "Edit and Resend"
3rd option is to continue to reverse engineer how and why "requestHeaders" is being added to the request in redux and replicate that in our process (eventually that might be identical to the 1st option)

for now we will continue with the 1st option, please tell us what you think.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 31 • 6 years ago

(In reply to stavbarak from comment #30)
> This is what we discovered so far:
> - clone request works okay, the problem is that before a request is
> selected, its object in redux does not contain the object "requestHeaders".
> - we placed an error in updateRequest to see (from its stacktrace) from
> where it is being called with "requestHeaders"
> - we found in request-utils(line 480) a call to connector.requestData(id,
> "requestHeaders") which eventually updates the request
> - the above is called from the MonitorPanel component immediately when the
> request is selected

Thanks for the analysis!

> Here is how we think to proceed:
> 1st option is to call connector.requestData(id, "requestHeaders") when a
> user right clicks on a request, thus preparing it to be properly cloned if
> they continue to click on "Edit and Resend"
> 2nd option  is to call it right before cloning, when the user clicks on on
> "Edit and Resend"
> 3rd option is to continue to reverse engineer how and why "requestHeaders"
> is being added to the request in redux and replicate that in our process
> (eventually that might be identical to the 1st option)
When the Headers sid-panel is opened (and the corresponding HeadersPanel component mounted),
all headers (request+response) are explicitly fetched here:

https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/devtools/client/netmonitor/src/components/HeadersPanel.js#91

You can also see how headers are explicitly fetched when they the user executes e.g. "Copy Request Headers" (from the context menu):

https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/devtools/client/netmonitor/src/widgets/RequestListContextMenu.js#335-336

So, as you said, this API is what you need:
this.props.connector.requestData(id, "requestHeaders");
or perhaps even better:
`fetchNetworkUpdatePacket`


> for now we will continue with the 1st option, please tell us what you think.
Why not to fetch the headers only when it's necessary? I.e. when the user clicks on on "Edit and Resend"?
Just like we do for 'Copy Headers'. I think that 2nd option is more effective.

Here is the menu item logic:
https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/devtools/client/netmonitor/src/widgets/RequestListContextMenu.js#97-105

Honza

	stavbarak
	Comment 32 • 6 years ago

We changed fetchNetworkUpdatePacket from

```js
function fetchNetworkUpdatePacket(requestData, request, updateTypes) {
  updateTypes.forEach((updateType) => {
    // Only stackTrace will be handled differently
    if (updateType === "stackTrace") {
      if (request.cause.stacktraceAvailable && !request.stacktrace) {
        requestData(request.id, updateType);
      }
      return;
    }

    if (request[`${updateType}Available`] && !request[updateType]) {
      requestData(request.id, updateType);
    }
  });
}
```


to 
```js
function fetchNetworkUpdatePacket(requestData, request, updateTypes) {
  return Promise.all(updateTypes.map((updateType) => {
    // Only stackTrace will be handled differently
    if (updateType === "stackTrace") {
      if (request.cause.stacktraceAvailable && !request.stacktrace) {
        return requestData(request.id, updateType);
      }
      return;
    }

    if (request[`${updateType}Available`] && !request[updateType]) {
      return requestData(request.id, updateType);
    }
  }));
}
```

It fixed the problem above.
However, then some tests failed. We think it's because the tests don't wait until the selector they need appears.
What do you think?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 33 • 6 years ago

(In reply to stavbarak from comment #32)
> However, then some tests failed. We think it's because the tests don't wait
> until the selector they need appears.
> What do you think?
Yes, it sounds like the reason.

Honza

	Pulsebot
	Comment 45 • 5 years ago

Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5a1f414d631
[Network-Monitor] Right click on method enables request details tab. r=Honza