Tap timeline column to open the timing sidebar panel

VERIFIED FIXED in Firefox 55

Status

enhancement
P3
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: gasolin, Assigned: gabrielle.singhcadieux, Mentored)

Tracking

({dev-doc-needed, good-first-bug})

unspecified
Firefox 55

Firefox Tracking Flags

(firefox55 verified)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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

We can add the similar thing for timing column. 


in request-list-waterfall.js, hook onclick event in div with .requests-list-timings class.
Then define `onWaterfallClick` in request-list-content.js
I would be interested in working on this! 
Would you be able to give me a little more information? I'm not sure how to replicate this event (i.e. I'm not sure what the stack-trace sidebar looks like because I'm not sure when `js` appears under `cause`).
Flags: needinfo?(gasolin)
Hi gabrielle,

Thanks for interesting to contribute!
You can refer the Bug 1359682 which the task is very similar. If you are familiar with React, you can also join to help us decide if we should use onClick or onMouseDown event.

For the example case, the js icon(badge) is only shown when the request is a js file. You can spot that in most of web site.
Assignee: nobody → gabrielle.singhcadieux
Flags: needinfo?(gasolin)
And welcome to go to slack #netmonitor channel to discuss anything related https://firefox-team.slack.com/messages/devtools/
(In reply to Fred Lin [:gasolin] from comment #0)
> When user tap the `js` icon in the `cause` column, he/she can see the
> `stack-trace `sidebar panel opened by default.
> 
> We can add the similar thing for timing column. 
> 
> 
> in request-list-waterfall.js, hook onclick event in div with
> .requests-list-timings class.

I can't find request-list-waterfall.js - I'm wondering if this was meant to be request-list-item.js?
(In reply to gabrielle.singhcadieux from comment #4)
> (In reply to Fred Lin [:gasolin] from comment #0)
> > When user tap the `js` icon in the `cause` column, he/she can see the
> > `stack-trace `sidebar panel opened by default.
> > 
> > We can add the similar thing for timing column. 
> > 
> > 
> > in request-list-waterfall.js, hook onclick event in div with
> > .requests-list-timings class.
> 
> I can't find request-list-waterfall.js - I'm wondering if this was meant to
> be request-list-item.js?

I just pulled the latest changes and now I see request-list-column-waterfall.js?
Posted patch 1359681.patch (obsolete) — Splinter Review
Based on visual inspection, I believe that this patch produces the desired behaviour.
Attachment #8867229 - Flags: review?(gasolin)
Comment on attachment 8867229 [details] [diff] [review]
1359681.patch

the patch looks good, thanks!


I will push the PR for auto test at next tuesday(cause I'm in travel now)

Though its optional, It will be nice if you can add a test case for this change.
Posted patch 1359681_v2.patch (obsolete) — Splinter Review
I've now added a test for this based on devtools/client/netmonitor/test/browser_net_security-icon-click.js

However, the test always times out when I run it. I believe that this is based on
```
let wait = waitForDOM(document, "#response-panel");
...
yield wait;
```
I am not sure why - the test appears to be performing correctly based on visual inspection?

I'm also not sure whether I added the test to devtools/client/netmonitor/test/browser.ini in the correct place.
Attachment #8867229 - Attachment is obsolete: true
Attachment #8867229 - Flags: review?(gasolin)
Attachment #8867514 - Flags: review?(gasolin)
Comment on attachment 8867514 [details] [diff] [review]
1359681_v2.patch

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

The main code works well! Just need to fix the test part. 
(It's fine to just remove the test in this case.)

::: devtools/client/netmonitor/test/browser_net_waterfall-click.js
@@ +12,5 @@
> +  let { document } = monitor.panelWin;
> +
> +  yield performRequestsAndWait();
> +
> +  let wait = waitForDOM(document, "#response-panel");

what we want to show is the "#timings-panel". You can use the browser toolbox to inspect the correct element
https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

@@ +13,5 @@
> +
> +  yield performRequestsAndWait();
> +
> +  let wait = waitForDOM(document, "#response-panel");
> +  let icon = document.querySelectorAll(".requests-list-timings")[0];

querySelectorAll(".requests-list-timings")[0] will select the column title but not the first row. You can use the browser toolbox to find the proper selector as well.

and `icon` is not a proper name for this selector, I suggest to use `timing`

@@ +20,5 @@
> +  EventUtils.synthesizeMouseAtCenter(icon, {}, monitor.panelWin);
> +
> +  yield wait;
> +
> +  let tabEl = document.querySelectorAll("#details-pane tab")[4];

You can check with "#timings-tab" directly

```
ok(document.querySelector("#timings-tab[aria-selected=true]"),
     "Timings tab is selected.");
```
Attachment #8867514 - Flags: review?(gasolin)
(In reply to Fred Lin [:gasolin] from comment #9)
> Comment on attachment 8867514 [details] [diff] [review]
> 1359681_v2.patch
> 
> Review of attachment 8867514 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The main code works well! Just need to fix the test part. 
> (It's fine to just remove the test in this case.)
> 
> ::: devtools/client/netmonitor/test/browser_net_waterfall-click.js
> @@ +12,5 @@
> > +  let { document } = monitor.panelWin;
> > +
> > +  yield performRequestsAndWait();
> > +
> > +  let wait = waitForDOM(document, "#response-panel");
> 
> what we want to show is the "#timings-panel". You can use the browser
> toolbox to inspect the correct element
> https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
> 
> @@ +13,5 @@
> > +
> > +  yield performRequestsAndWait();
> > +
> > +  let wait = waitForDOM(document, "#response-panel");
> > +  let icon = document.querySelectorAll(".requests-list-timings")[0];
> 
> querySelectorAll(".requests-list-timings")[0] will select the column title
> but not the first row. You can use the browser toolbox to find the proper
> selector as well.
> 
> and `icon` is not a proper name for this selector, I suggest to use `timing`
> 
> @@ +20,5 @@
> > +  EventUtils.synthesizeMouseAtCenter(icon, {}, monitor.panelWin);
> > +
> > +  yield wait;
> > +
> > +  let tabEl = document.querySelectorAll("#details-pane tab")[4];
> 
> You can check with "#timings-tab" directly
> 
> ```
> ok(document.querySelector("#timings-tab[aria-selected=true]"),
>      "Timings tab is selected.");
> ```

Thank you, I was really struggling with understanding the HTML structure of the developer tools! I did not know about the browser toolbox.
I will fix the test based on your comments.
Posted patch 1359681_v3.patch (obsolete) — Splinter Review
The test now passes! 

However, 7 other netmonitor tests fail. Specifically
```
25743 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-flags.js | There should be a specific amount of visible items in the requests menu. - Got 0, expected 1

25744 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-flags.js | The item at index 1 has visibility=true - Got false, expected true

25745 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-flags.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/test/head.js:399 - TypeError: target is undefined

25746 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_prefs-reload.js | Pref networkDetailsHeight should validate: 450 - Got (new Number(438)), expected (new Number(450))

25747 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_prefs-reload.js | The UI element affecting networkDetailsHeight should validate: 450 - Got (new Number(438)), expected (new Number(450))

25748 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_prefs-reload.js | Pref networkDetailsHeight should validate: 450 - Got (new Number(438)), expected (new Number(450))

25749 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_simple-request-details.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_simple-request-details.js:66 - TypeError: tabEl is undefined
```

Are these other bugs that need to be dealt with?
Attachment #8867514 - Attachment is obsolete: true
Attachment #8868391 - Flags: review?(gasolin)
The simple rule of test:

* if those tests fail is caused by this patch, figure out why and fix them
* if those tests fail without any of your modification, it's ok (the patch cause the fail need to be backed out, or someone still need to fix them)
(In reply to Fred Lin [:gasolin] from comment #12)
> The simple rule of test:
> 
> * if those tests fail is caused by this patch, figure out why and fix them
> * if those tests fail without any of your modification, it's ok (the patch
> cause the fail need to be backed out, or someone still need to fix them)

Those tests fail, regardless of this patch.
Comment on attachment 8868391 [details] [diff] [review]
1359681_v3.patch

the patch can not be apply on today's repository now.
Please rebase the patch with new mozilla-central. It might solve test fail issues.
Attachment #8868391 - Flags: review?(gasolin)
Flags: needinfo?(gabrielle.singhcadieux)
Posted patch 1359681_v3_rebased.patch (obsolete) — Splinter Review
I've now rebased the patch. However, the following 4 tests still fail:

28710 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_prefs-reload.js | Pref networkDetailsHeight should validate: 450 - Got (new Number(438)), expected (new Number(450))

28711 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_prefs-reload.js | The UI element affecting networkDetailsHeight should validate: 450 - Got (new Number(438)), expected (new Number(450))

28712 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_prefs-reload.js | Pref networkDetailsHeight should validate: 450 - Got (new Number(438)), expected (new Number(450))

28713 INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_simple-request-details.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_simple-request-details.js:66 - TypeError: tabEl is undefined
Attachment #8868391 - Attachment is obsolete: true
Flags: needinfo?(gabrielle.singhcadieux)
Attachment #8869273 - Flags: review?(gasolin)
gabrielle, sorry for late response. I'll help check the test part within this week.
browser_net_prefs-reload.js test looks fine in my desktop, and the issue around browser_net_simple-request-details.js looks like about the DOM load timing.


Since React does not use real mouse event, it has some delay between click & actual render the dom.
You can use `waitForDOM` for browser_net_simple-request-details as you have used in browser_net_waterfall_click, ex:

```
wait = waitForDOM(document, "#timings-panel");
EventUtils.sendMouseEvent({ type: "click" },
  document.querySelector(".network-details-panel-toggle"));
yield wait;
```

You may need to wrap each sendMouseEvent with waitForDOM to make test pass
Flags: needinfo?(gabrielle.singhcadieux)
Attachment #8869273 - Flags: review?(gasolin)
browser_net_simple-request-details fails at [1] because `tabEl` is not defined, meaning that the selector "#details-pane tab" is not matched. However, the only call to `sendMouseEvent` before this is at [2].
When I wrap the sendMouseEvent at [2] with `waitForDOM`, exactly as you suggested, the test times out.
I also tried wrapping the sendMouseEvent at [2] with `waitForDOM` which waits for "#details-pane", and again, the test times out.

Therefore, I believe that the issue is with the selector "#details-pane tab". I looked for the element with id "details-pane", and it appears to be a part of the performance tool rather than the netmonitor tool ([3] and [4]).
I verified this using the browser toolbox: when the developer toolbox is open, "#details-pane" is not found. It is also not found when the network tab is open. However, it is found when the performance tab is open.

What are your thoughts?

[1] https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/netmonitor/test/browser_net_simple-request-details.js#l66
[2] https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/netmonitor/test/browser_net_simple-request-details.js#l38
[3] https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/performance/performance-view.js#l93
[4] https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/performance/views/details.js#l50
Flags: needinfo?(gabrielle.singhcadieux) → needinfo?(gasolin)
I think it still related to the timing when the panel DOM is actually ready. Instead of get general tabpanel element, you can rewrite the test and try to listen and check the actual target panel like #heads-panel, #timings-panel instead. 

You can refer browser_net_raw_headers to see how it check `headers-summary` after click the first row in requests-list
Flags: needinfo?(gasolin)
I replaced [1] with
```
let tabEl = document.querySelector("#headers-tab");
let tabpanel = document.querySelector("#headers-panel");
```
which are both successfully matched.

However, the checks starting at [2] fail because the selectors aren't matched. I believe, based on searching using the browser toolbox, that these elements (eg. `"#headers-summary-url-value") don't exist.

However, I also can't find unique identifiers for those values. I believe that the following is the HTML for the header summary panel:
```
<div class="tabpanel-summary-container headers-summary">
  <div class="tabpanel-summary-label headers-summary-label">Request URL:</div>
  <input class="tabpanel-summary-value textbox-input devtools-monospace" readonly="" value="http://example.com/browser/devtools/client/netmonitor/test/sjs_simple-test-server.sjs">
</div>
<div class="tabpanel-summary-container headers-summary">
  <div class="tabpanel-summary-label headers-summary-label">Request method:</div>
  <input class="tabpanel-summary-value textbox-input devtools-monospace" readonly="" value="GET">
</div>
<div class="tabpanel-summary-container headers-summary">
  <div class="tabpanel-summary-label headers-summary-label">Remote address:</div>
  <input class="tabpanel-summary-value textbox-input devtools-monospace" readonly="" value="93.184.216.34:80">
</div>
<div class="tabpanel-summary-container headers-summary">
  <div class="tabpanel-summary-label headers-summary-label">Status code:</div>
  <div class="requests-list-status-icon" data-code="404"></div>
  <input class="tabpanel-summary-value textbox-input devtools-monospace status-text" readonly="" value="404 Not Found" size="13">
  <a class="learn-more-link" title="https://developer.mozilla.org/docs/Web/HTTP/Status/404?utm_source=mozilla&amp;utm_medium=devtools-netmonitor&amp;utm_campaign=default">[Learn More]</a>
  <button class="devtools-button">Edit and Resend</button>
  <button class="devtools-button">Raw headers</button>
</div>
<div class="tabpanel-summary-container headers-summary">
  <div class="tabpanel-summary-label headers-summary-label">Version:</div>
  <input class="tabpanel-summary-value textbox-input devtools-monospace" readonly="" value="HTTP/1.1">
</div>
```
If I understand [2] correctly, the test is checking that each of the summary values is correct. However, I don't see any identifier that distinguishes the summary values from one another.

[1] https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/netmonitor/test/browser_net_simple-request-details.js#l63
[2] https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/netmonitor/test/browser_net_simple-request-details.js#l69
Flags: needinfo?(gasolin)
Ouch deeply sorry, its my bad to not find out at first time. 

The true reason is in browser.ini

```
 [browser_net_simple-request-details.js]
+[browser_net_waterfall-click.js]
 skip-if = true # Bug 1258809
```

The simple-request-details.js test was skipped with `skip-if` syntax, but the patch add a line between the script and the skip-if syntax...

Please add [browser_net_waterfall-click.js] after skip-if syntax, and it should be fine!


Please update the patch and we can try to land it :)
Flags: needinfo?(gasolin)
By the way you are right in comment 20, the test target in simple-request-details are not exist anymore, and we skipped this test during the recent refactor.
Posted patch 1359681_v4.patch (obsolete) — Splinter Review
Okay, I didn't realize that the if-condition in the ini file applied to the test file immediately above it!
Attachment #8869273 - Attachment is obsolete: true
Attachment #8874577 - Flags: review?(gasolin)
Comment on attachment 8874577 [details] [diff] [review]
1359681_v4.patch

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

in browser.ini please put these tests name in alphabet order, then its ready to go

[browser_net_waterfall-click.js]
[browser_net_persistent_logs.js]
Attachment #8874577 - Flags: review?(gasolin) → review+
according to https://treeherder.mozilla.org/#/jobs?repo=try&revision=522acba5a377&selectedJob=104763568

2 lint error need to be fixed,

* components/request-list-content.js:309:2 | Expected indentation of 6 spaces but found 1 tab. 
* components/request-list-item.js:167:1 | Line 167 exceeds the maximum line length of 90

You can use following command to make sure all lint passed

`./mach eslint devtools/client/netmonitor/`
(In reply to Fred Lin [:gasolin] from comment #26)
> according to
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=522acba5a377&selectedJob=104763568
> 
> 2 lint error need to be fixed,
> 
> * components/request-list-content.js:309:2 | Expected indentation of 6
> spaces but found 1 tab. 
> * components/request-list-item.js:167:1 | Line 167 exceeds the maximum line
> length of 90
> 
> You can use following command to make sure all lint passed
> 
> `./mach eslint devtools/client/netmonitor/`

Neither of those lines was modified by this patch - should I still change them and add that to this patch?
Flags: needinfo?(gasolin)
Since we changed the request-list-content.js & request-list-item.js content, we do need make sure the lint for these files are valid. Or the sheriff will back out the patch
Flags: needinfo?(gasolin)
If the error is caused by others, try to rebase the repo later and the lint issue might be gone
I'm sorry, I was misunderstanding the line numbers in the patch format - those lint errors were being produced by this code! They should be gone now.
Attachment #8874577 - Attachment is obsolete: true
Attachment #8875327 - Flags: review?(gasolin)
Attachment #8874674 - Attachment is obsolete: true
Attachment #8875327 - Flags: review?(gasolin) → review+
thanks!
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24ef2aa69cf3
Tap timeline column to open the timing sidebar panel. r=gasolin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/24ef2aa69cf3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
I can verify that in latest beta 55.0b7 (64-bit), clicking timeline column opens "Timing" tab in sidebar panel.

Build ID 	20170706155135
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
QA Whiteboard: [testday-20170707]
I also can confirm the bug is verified fixed on 55.0b7 build1 (20170706085221) using Windows 10 x64.  
Thank you, Sayed for verifying this issue.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.