Closed Bug 1359682 Opened 3 years ago Closed 3 years ago

tap thumbnail to open response sidebar panel

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: gasolin, Assigned: brennan.brisad, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 3 obsolete files)

When user tap the `js` icon in the `cause` column, he/she can see the `stack-trace `sidebar panel opened by default.

As we know the response sidebar panel contain the image preview, we can add the similar thing for file column. 


in request-list-file.js, hook onclick event in img element.
Then define `onThumbnailClick` in request-list-content.js
Thanks for the bug report, I like that idea!

Honza
Assignee: nobody → brennan.brisad
Attached patch bug1359682.patch (obsolete) — Splinter Review
Attachment #8862370 - Flags: review?(gasolin)
Comment on attachment 8862370 [details] [diff] [review]
bug1359682.patch

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

Thanks for the patch and adding the test! The main flow looks good, some comment about improve the test.

::: devtools/client/netmonitor/src/components/request-list-content.js
@@ +291,5 @@
>          dispatch(Actions.selectDetailsPanelTab("security"));
>        }
>      },
>      onSelectDelta: (delta) => dispatch(Actions.selectDelta(delta)),
> +    onThumbnailClick: () => {

We can add some comment as onCauseBadgeClick/onSecurityIconClick does

```
/**
 * A handler that opens the response tab in the details view if the thumbnail is clicked.
 */
```

::: devtools/client/netmonitor/test/browser_net_thumbnail-click.js
@@ +6,5 @@
> +/**
> + * Test that clicking on the file thumbnail opens the response details tab.
> + */
> +
> +add_task(function* () {

+1, A test is helpful to check if that function really works!

@@ +12,5 @@
> +  let { document } = monitor.panelWin;
> +
> +  yield performRequestsAndWait();
> +  yield openHeadersPanel();
> +

Declare extra wait for response-panel here, so we can just remove openHeadersPanel when related bug fixed

`let wait = waitForDOM(document, "#response-panel");`

@@ +18,5 @@
> +  let icon = request.querySelector(".requests-list-icon");
> +
> +  info("Clicking thumbnail of the sixth request.");
> +  EventUtils.synthesizeMouseAtCenter(icon, {}, monitor.panelWin);
> +

do wait here to make sure the component is rendered
`yield wait;`

@@ +21,5 @@
> +  EventUtils.synthesizeMouseAtCenter(icon, {}, monitor.panelWin);
> +
> +  ok(document.querySelector("#response-tab[aria-selected=true]"),
> +     "Response tab is selected.");
> +

Can add one more test to make sure the image preview field is there
```
  ok(document.querySelector(".response-image-box"),
     "Response image preview is shown.");
```

@@ +32,5 @@
> +    });
> +    yield wait;
> +  }
> +
> +  function* openHeadersPanel() {

As this test shows, I saw an issue that the select panel function will not work before we opened the sidebar. But its not caused by this patch. Please file a bug and add comment before this function: 
`// FIXME: the select panel function will not work before we opened the sidebar.
This function can be removed after bug xxxxxx fixed`
Attachment #8862370 - Flags: review?(gasolin) → feedback+
(The above issue is caused by event is propagated to both request-item mousedown / column onclick) It will be nice if it could be fixed at same patch, but its ok to file a bug and add comment there.
Attached patch bug1359682.patch (obsolete) — Splinter Review
Thanks for the review!

I've addressed your comments in the new patch.

I solved the side-panel issue by using onMouseDown instead, like the security icon code did.  For consistency, I also made changes to the other handlers.  I hope they are minor enough that they don't need to be handled in separate bugs.
Attachment #8862800 - Flags: review?(gasolin)
Attachment #8862370 - Attachment is obsolete: true
Thanks for the update! I saw the patch changed all onClick event to onMouseDown event. 

onMouseDown will be also triggered by right click or the middle click. I'm not sure if we use that feature to select the line in the request list and show the proper context menu with the right click. If that's the case, we could use onMouseDown event for all onClick event to fix event propagation issue (by react) for free.

I haven't applied the patch yet but I think we should use onClick instead of onMouseDown if both way can solve the issue (since onClick looks more intuitive in our use case).

Michael, how do you think?
Flags: needinfo?(brennan.brisad)
(In reply to Fred Lin [:gasolin] from comment #6)
> Thanks for the update! I saw the patch changed all onClick event to
> onMouseDown event. 

Yes.  More specifically, in two cases I changed the actual event from onClick to onMouseDown: for the cause badge and the new file thumbnail.  For the security badge, the handler was already bound to onMouseDown, and there I only changed the handler name from `onSecurityIconClick` to `onSecurityIconMouseDown` for consistency and clarity.

The handler for selecting a row item in the requests list is called `onItemMouseDown`, and it was already bound to onMouseDown.  That one I didn't touch.

> onMouseDown will be also triggered by right click or the middle click. I'm
> not sure if we use that feature to select the line in the request list and
> show the proper context menu with the right click. If that's the case, we
> could use onMouseDown event for all onClick event to fix event propagation
> issue (by react) for free.

I see.  Well, with my change the context menu still works, as is expected since I didn't change the event for the actual list item.  But if I right-click a badge or thumbnail it will both open the appropriate tab and show the context menu, at the same time.  I don't think that should be a problem.  For instance, the security badge already did it like that.

> I haven't applied the patch yet but I think we should use onClick instead of
> onMouseDown if both way can solve the issue (since onClick looks more
> intuitive in our use case).

I experimented changing the event from onMouseDown to onClick for selecting list items, but then I need to release the mouse button before the item is selected and that felt very unnatural.

> Michael, how do you think?

I do agree onClick sounds more intuitive, but since some of the code already used onMouseDown I think using it for all badges and thumbnail shouldn't pose any problems.  It seems to solve the issue in a simple way.

I might be misunderstanding your points though and I am open for suggestions.
Flags: needinfo?(brennan.brisad) → needinfo?(gasolin)
Thank you for do the experiment! lets keep using onMouseDown event.

Sorry for the late review, I'll complete the review and push to try server for auto test tomorrow.
Flags: needinfo?(gasolin)
(In reply to Fred Lin [:gasolin] from comment #8)
> Thank you for do the experiment! lets keep using onMouseDown event.
> 
> Sorry for the late review, I'll complete the review and push to try server
> for auto test tomorrow.

Thanks!  No worries, I have no hurry. :-)
Comment on attachment 8862800 [details] [diff] [review]
bug1359682.patch

Looks good, thanks!

Though I met an issue to apply the .patch with hg, might need rebase the patch to the up-to-date branch
Attachment #8862800 - Flags: review?(gasolin) → review+
I don't know whether you already rebased the patch youself.  But if not, here's a new version.
Attachment #8864496 - Flags: review?(gasolin)
Attachment #8864373 - Attachment is obsolete: true
Attachment #8862800 - Attachment is obsolete: true
Comment on attachment 8864496 [details] [diff] [review]
Rebased bug1359682.patch

thanks!
Attachment #8864496 - Flags: review?(gasolin) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9bb619755c4
Open Netmonitor response panel when clicking on file thumbnail. r=gasolin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e9bb619755c4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
I have reproduced this bug with Nightly 55.0a1 (2017-04-25) (64-bit) on Windows 7, 64 Bit!

This bug's fix is verified with latest Nightly!

Build ID   : 20170510030301
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170510]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.