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

RESOLVED FIXED in Firefox 65

Status

enhancement
P3
normal
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: mcroud, Assigned: tanhengyeow, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 65

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: good-first-bug)

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
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.
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
Mentor: odvarko
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: good-first-bug
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
(Assignee)

Comment 3

6 months ago
Edit and Resend: Cancel button should return a user to the Headers panel
Assignee: nobody → E0032242
Status: NEW → ASSIGNED
(Assignee)

Comment 4

6 months ago
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?
```
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
(Assignee)

Comment 6

6 months ago
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?
Flags: needinfo?(odvarko)
(Assignee)

Comment 7

6 months ago
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?
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
Flags: needinfo?(odvarko)
@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:
Flags: needinfo?(E0032242)
(Assignee)

Comment 11

6 months ago
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
Flags: needinfo?(odvarko)
(Assignee)

Updated

6 months ago
Flags: needinfo?(E0032242)
(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
Flags: needinfo?(odvarko)

Comment 13

6 months ago
Pushed by jodvarko@mozilla.com:
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

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1304f235bc67
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in before you can comment on or make changes to this bug.