1359681 - Tap timeline column to open the timing sidebar panel

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[[e-mail]]

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

Comment 1

[gabrielle.singhcadieux]

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`).

Comment 2

[[e-mail]]

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.

Comment 3

[[e-mail]]

And welcome to go to slack #netmonitor channel to discuss anything related https://firefox-team.slack.com/messages/devtools/

Comment 4

[gabrielle.singhcadieux]

(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?

Comment 5

[gabrielle.singhcadieux]

(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?

Comment 6

[gabrielle.singhcadieux]

Attachment: 1359681.patch

Based on visual inspection, I believe that this patch produces the desired behaviour.

Comment 7

[[e-mail]]

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.

Comment 8

[gabrielle.singhcadieux]

Attachment: 1359681_v2.patch

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.

Comment 9

[[e-mail]]

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.");
```

Comment 10

[gabrielle.singhcadieux]

(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.

Comment 11

[gabrielle.singhcadieux]

Attachment: 1359681_v3.patch

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?

Comment 12

[[e-mail]]

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)

Comment 13

[gabrielle.singhcadieux]

(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 14

[[e-mail]]

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.

Comment 15

[gabrielle.singhcadieux]

Attachment: 1359681_v3_rebased.patch

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

Comment 16

[[e-mail]]

gabrielle, sorry for late response. I'll help check the test part within this week.

Comment 17

[[e-mail]]

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

Comment 18

[gabrielle.singhcadieux]

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

Comment 19

[[e-mail]]

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

Comment 20

[gabrielle.singhcadieux]

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

Comment 21

[[e-mail]]

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 :)

Comment 22

[[e-mail]]

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.

Comment 23

[gabrielle.singhcadieux]

Attachment: 1359681_v4.patch

Okay, I didn't realize that the if-condition in the ini file applied to the test file immediately above it!

Comment 24

[[e-mail]]

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]

Comment 25

[[e-mail]]

Attachment: Bug 1359681 - Tap timeline column to open the timing sidebar panel

Review commit: https://reviewboard.mozilla.org/r/146026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146026/

Comment 26

[[e-mail]]

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/`

Comment 27

[gabrielle.singhcadieux]

(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?

Comment 28

[[e-mail]]

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

Comment 29

[[e-mail]]

If the error is caused by others, try to rebase the repo later and the lint issue might be gone

Comment 30

[gabrielle.singhcadieux]

Attachment: 1359681_v5.patch

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.

Comment 31

[[e-mail]]

thanks!

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24ef2aa69cf3
Tap timeline column to open the timing sidebar panel. r=gasolin

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/24ef2aa69cf3

Comment 34

[Sayed [:LiquidO2]]

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

Comment 35

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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.