1499390 - Edit and Resend. Cancel button should return a user to the Headers panel

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[mcroud]

In order to access the New Request form, one must click the Edit and Resend button which appears in the Headers panel.

If a user has done the following:
1) Clicked on a request in the network panel to reveal the Headers panel.
2) Clicked on the Edit and Resend button to reveal the New Request form.
3) Clicked the cancel button.

It is perhaps understandable that the user may expect to be taken a single step back to the Headers panel, their previous location.

What happens is that the Headers/details panel is closed, taking the user back to the Network Panel.

A better user journey is likely had if clicking the cancel button takes the user back to the Headers panel from which they originated.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me) PTO July 8 - 26]

Thanks Matt for the report, I've been also thinking about this.

Some instructions for anyone interested to fix this bug:

1) This is the place where the "Edit and Resend" form is opened from the Headers panel:
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/devtools/client/netmonitor/src/components/HeadersPanel.js#240

The code is executing a function (prop) `cloneSelectedRequest` passed into the HeadersPanel component.

2) This is the place where `cloneSelectedRequest` is passed into the HeadersPanel:
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/devtools/client/netmonitor/src/components/TabboxPanel.js#73

3) Here is the place where `cloneSelectedRequest` is passed into the TabboxPanel:
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/devtools/client/netmonitor/src/components/NetworkDetailsPanel.js#46

4) Here is the place where the `cloneSelectedRequest` function is constructed
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/devtools/client/netmonitor/src/components/NetworkDetailsPanel.js#82

We need to pass an argument to it. Something like:
cloneSelectedRequest: (openedFromHeadersPanel) => dispatch(Actions.cloneSelectedRequest(openedFromHeadersPanel)),

The flag needs to make it through the action to the Requests reducer:

Action is defined here:
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/devtools/client/netmonitor/src/actions/requests.js#40

Reducer here:
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/devtools/client/netmonitor/src/reducers/requests.js#121

5) The flag can be stored in the `request` structure:
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/devtools/client/netmonitor/src/reducers/requests.js#141

requestPostDataAvailable: clonedRequest.requestPostDataAvailable,
isCustom: true,
openedFromHeadersPanel: action.openedFromHeadersPanel,
};

6) Finally, when the Cancel button in Edit and Resend form is pressed (or Send button btw.), the `removeSelectedCustomRequest` is executed
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/devtools/client/netmonitor/src/components/CustomRequestPanel.js#286

Again, the flag needs to be carried through the action to the reducer:
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/devtools/client/netmonitor/src/actions/requests.js#83

Something like:
function removeSelectedCustomRequest(openedFromHeadersPanel) {
  return {
    type: REMOVE_SELECTED_CUSTOM_REQUEST,
    openedFromHeadersPanel: openedFromHeadersPanel,
  };
}

https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/devtools/client/netmonitor/src/reducers/ui.js#158


case REMOVE_SELECTED_CUSTOM_REQUEST:
  return openNetworkDetails(state, { open: action.openedFromHeadersPanel });
case SEND_CUSTOM_REQUEST:
  return openNetworkDetails(state, { open: false });


7) Not sure if `openedFromHeadersPanel` any better suggestions?

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me) PTO July 8 - 26]

8) this bug also needs a test

You might look at the following tests to get some inspiration:

devtools\client\netmonitor\test\browser_net_telemetry_edit_resend.js
devtools\client\netmonitor\test\browser_net_telemetry_edit_resend.js
devtools\client\netmonitor\test\browser_net_edit_resend_caret.js

Honza

Comment 3

[Hengyeow (:tanhengyeow)]

Attachment: Bug 1499390 - Edit and Resend: Cancel button should return a user to the Headers panel. r=Honza

Edit and Resend: Cancel button should return a user to the Headers panel

Comment 4

[Hengyeow (:tanhengyeow)]

Hi :Honza

Progress on this bug as follows:
I need some help understanding what is being interpreted as the variable `openedFromHeadersPanel`. Debugging through the files, 
- `openedFromHeadersPanel` is being interpreted as the `edit-and-resend` button when the action `cloneSelectedRequest` is invoked
- `openedFromHeadersPanel` is being interpreted as the `cancel` button when the action `removeSelectedCustomRequest ` is invoked

I think demystifying this example would help in identifying how to restore to the original state when the cancel button is clicked:

```
Say for this function: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/components/CustomRequestPanel.js#281, it accepts 3 parameters and returns dispatching the action `updateRequest(id, data, batch)`

The action is defined here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/actions/requests.js#27, with type `UPDATE_REQUEST` and the following 3 parameters as payload which can be accessed via `action.<parameter>` by the reducer.

When it reaches the reducer that handles the action type: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/reducers/requests.js#83, it is able to get the `action.<parameter>` as shown.

So the question is, who is passing these parameters to the action as payload such that the reducer can access it?
```

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me) PTO July 8 - 26]

(In reply to Heng Yeow (:tanhengyeow) from comment #4)
> So the question is, who is passing these parameters to the action as payload
> such that the reducer can access it?

So, in case of the `Actions.updateRequest` action, it's the caller of the `updateRequest` property (a function).

The function property is defined here (just like you pointed out):
https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/components/CustomRequestPanel.js#281

and the execution happens here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/components/CustomRequestPanel.js#152

This place passes arguments in it and these arguments are carried all the way to the reducer.

Honza

Comment 6

[Hengyeow (:tanhengyeow)]

Hi :Honza

Thanks for the explanation! I realized the current workflow doesn't work if I apply the suggested comments. 

In /devtools/client/netmonitor/src/reducers/ui.js, even when `open` is always set to true (example below to illustrate):

```
    case REMOVE_SELECTED_CUSTOM_REQUEST:
      return openNetworkDetails(state,
        { open: true });
```

The panel will still be closed and not reopened after clicking cancel. Following the stack trace in the debugger, the panel is only "closed" after all the functions in the stack trace has resolved. So I suspect triggering `openNetworkDetails` and giving it value of true would not open the panel again within the existing function stack trace.

I think we need a different approach for this. What do you think?

Comment 7

[Hengyeow (:tanhengyeow)]

Hi :Honza

Thanks for the explanation! I realized the current workflow doesn't work if I apply the suggested comments. 

In /devtools/client/netmonitor/src/reducers/ui.js, even when `open` is always set to true (example below to illustrate):

```
    case REMOVE_SELECTED_CUSTOM_REQUEST:
      return openNetworkDetails(state,
        { open: true });
```

The panel will still be closed and not reopened after clicking cancel. Following the stack trace in the debugger, the panel is only "closed" after all the functions in the stack trace has resolved. So I suspect triggering `openNetworkDetails` and giving it value of true would not open the panel again within the existing function stack trace.

I think we need a different approach for this. What do you think?

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me) PTO July 8 - 26]

There is also this place:

https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/devtools/client/netmonitor/src/components/MonitorPanel.js#85

componentDidUpdate() {
  const { selectedRequestVisible, openNetworkDetails } = this.props;
  if (!selectedRequestVisible) {
    openNetworkDetails(false);
  }
}

... it closes the Details panel again if there is no selection (or selection not visible)

The issue is the after canceling the edit & resend form the current selected request is unselected. This happens because the temporary request (cloned request) is created & selected when the Custom form is opened. And removed when it's cancelled.
We need to make sure that the original request is selected again.

You need to explore this method:
https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/devtools/client/netmonitor/src/reducers/requests.js#199

Honza

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me) PTO July 8 - 26]

@Heng: I am puzzled by the patch in Phabricator, it has just two changes now?
Anything unexpected happened?

Honza

diff --git a/devtools/client/netmonitor/src/reducers/requests.js b/devtools/client/netmonitor/src/reducers/requests.js
--- a/devtools/client/netmonitor/src/reducers/requests.js
+++ b/devtools/client/netmonitor/src/reducers/requests.js
@@ -145,6 +145,7 @@
         ...state,
         requests: mapSet(requests, newRequest.id, newRequest),
         selectedId: newRequest.id,
+        preselectedId: selectedId,
       };
     }
 
diff --git a/devtools/client/netmonitor/src/reducers/ui.js b/devtools/client/netmonitor/src/reducers/ui.js
--- a/devtools/client/netmonitor/src/reducers/ui.js
+++ b/devtools/client/netmonitor/src/reducers/ui.js
@@ -156,6 +156,7 @@
     case RESET_COLUMNS:
       return resetColumns(state);
     case REMOVE_SELECTED_CUSTOM_REQUEST:
+      return openNetworkDetails(state, { open: true });
     case SEND_CUSTOM_REQUEST:
       return openNetworkDetails(state, { open: false });
     case SELECT_DETAILS_PANEL_TAB:

Comment 11

[Hengyeow (:tanhengyeow)]

Hi :Honza

I found out that we do not need to pass any arguments as suggested in Comment 1. 

Returning the `preselectedId` (value of original request id) would be enough as it would be parsed in https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/devtools/client/netmonitor/src/components/MonitorPanel.js#84 (the selected request would be still visible).

I've updated the patch and also wrote a mochitest for it.

Try results here for reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4e49ed7e8b5e4ac34d259f5bca4868152394073

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me) PTO July 8 - 26]

(In reply to Heng Yeow (:tanhengyeow) from comment #11)
> Hi :Honza
> 
> I found out that we do not need to pass any arguments as suggested in
> Comment 1. 
> 
> Returning the `preselectedId` (value of original request id) would be enough
> as it would be parsed in
> https://searchfox.org/mozilla-central/rev/
> a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/devtools/client/netmonitor/src/
> components/MonitorPanel.js#84 (the selected request would be still visible).
> 
> I've updated the patch and also wrote a mochitest for it.
> 
> Try results here for reference:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d4e49ed7e8b5e4ac34d259f5bca4868152394073
Awesome job here!

Honza

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1304f235bc67
Edit and Resend: Cancel button should return a user to the Headers panel. r=Honza

Comment 14

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/1304f235bc67