1566527 - Waterfall column never hides with sidebar opened

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[:Harald Kirschner :digitarald]

Attachment: image.png

What were you doing?

1. Open sidebar on a network resource
2. Resize sidebar to squish the resource table

What happened?

Waterfall column never hides.

What should have happened?

Waterfall column hides, as it does responsively in the sidebar-less mode.

Comment 1

[:Harald Kirschner :digitarald]

Attachment: ESR behavior

We could treat this as regression, as the past non-resizing table just was overlayed by the sidebar which does not cause this issue.

Comment 2

[Victoria Wang [:victoria]]

Agreed. Looks like it does get hidden in dock-to-side mode. We may want to consider responsively hiding a few other columns as well so that the most important columns are usable for as long as possible. E.g. would you say Status/Domain/File the most important? Would love to learn more about this.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Some instructions for anyone interested:

1. The waterfall column is rendered here
   https://searchfox.org/mozilla-central/rev/4fbcfe6fecf75d7b828fbebda79a31385f7eab5f/devtools/client/netmonitor/src/components/RequestListItem.js#326

2. It's rendered only if visible i.e. if columns.waterfall is true

3. There condition should be extended to also check state of the sidebar

Something like:
columns.waterfall && sideClosed

4. The state of the sidebar visibility is stored in the UI reducer
   https://searchfox.org/mozilla-central/rev/4fbcfe6fecf75d7b828fbebda79a31385f7eab5f/devtools/client/netmonitor/src/reducers/ui.js#75

5. The state is accessed here
   https://searchfox.org/mozilla-central/rev/4fbcfe6fecf75d7b828fbebda79a31385f7eab5f/devtools/client/netmonitor/src/components/RequestListContent.js#404

So, we should pass the networkDetailsOpen into the RequestListItem as a prop somewhere here
https://searchfox.org/mozilla-central/rev/4fbcfe6fecf75d7b828fbebda79a31385f7eab5f/devtools/client/netmonitor/src/components/RequestListContent.js#374-392

Honza

Comment 4

[Vincent Lequertier]

Hi Honza,

can I work on this?

Vincent

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Sure, assigned to you.
Honza

Comment 6

[Vincent Lequertier]

Hi,

I got started on this. I have done exactly as you suggested in comment 3, but there's an issue with the headers. The header for the waterfall column is not removed (columns.waterfall is still true) so it looks like in the image I posted broken_waterfall_header.png. I tried a couple of approaches to fix this:

1

In RequestListItem, I tried to set columns.waterfall to sideClosed so that it would hide the column and the column header if the sidebar is open. Then, I added sideClosed here so that the component updates whenever the sidebar is opened or closed. But when closing the sidebar the devtools crashes withis this error, and I am not sure how to fix it:

JavaScript error: resource://devtools/client/netmonitor/src/components/RequestListHeader.js, line 441: TypeError: headerRef is undefined

2

I modified getVisibleColumns() so that columns.waterfall is equal to sideClosed. This works, but that doesn't seem like the right fix to me. Also, this breaks the column selection feature (right click on the column headers to select the visible colums).

Now my question is: what is the proper way to deal with the waterfall column header? Would you prefer that I submit a (broken) patch so that you can better understand my question?

Best,

Vincent

Comment 7

[Vincent Lequertier]

Attachment: broken_waterfall_header.png

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the update!

Can you please attach a patch with your changes, so I can take a look?
Honza

Comment 9

[Vincent Lequertier]

Attachment: approach1.patch

Here is the patch for the first approach

Comment 10

[Vincent Lequertier]

Attachment: approach2.patch

Here is the patch for the second approach. Thanks for your help!

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the analysis!
You are right, there are two places where we need to care about visible columns.

I think that #2 is closed to what I have in mind now.

New comments:

1. Let's implement a new selector that will return list of all available columns and excludes the waterfall if the side bar is closed.

Something like as follows:

const getColumns = createSelector(
  state => state.ui, (ui) => {
    if (!ui.networkDetailsOpen) {
      return ui.columns;
    }

    // Remove the Waterfall/Timeline column from the list of available
    // columns if the details side-bar is opened.
    let columns = {...ui.columns};
    delete columns.waterfall;
    return columns;
  }
);

It should go to selectors/ui

2. Consequently we need to use the selector on both places where we access the list of available columns.

• In RequestListHeader & RequestListContent

• https://searchfox.org/mozilla-central/search?q=columns%3A+state.ui.columns%2C&case=true&path=

This way, we'll pretend that the Waterfall/Timeline column doesn't exist if the side bar is opened. So, the header context menu should also be ok (please test)

Honza

Comment 12

[Vincent Lequertier]

Attachment: Bug 1566527 - Hide the waterfall column when the netmonitor sidebar is opened;r=Honza

Comment 13

[Vincent Lequertier]

Thanks for the help! Didn't know about the selectors. I have updated the patch and associated tests.

Vincent

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Please see my comments in Phabricator
Honza

Comment 15

[Vincent Lequertier]

Updated the patch :-)

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comments in Phabricator.
Honza

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Vincent, this is very close to landing. Let me know if I can help with something...
Honza

Comment 18

[Vincent Lequertier]

Hi,

Sadly I haven't found the time to tackle the remaining issues yet. I hope to fix them this week-end. As the priority is noted "P3", I haven't realized it was urgent, so I chose this issue because I thought I wasn't slowing things down, sorry.

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Don't be sorry, I didn't mean to push :-)
Thanks for the help!
Honza

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/215d17a0eeb5
Hide the waterfall column when the netmonitor sidebar is opened;r=Honza

Comment 21

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=215d17a0eeb55512dfe99281e3eeb9a5c0c8198b&selectedJob=267987562&searchStr=Linux%2Cx64%2Casan%2CMochitests%2Ctest-linux64-asan%2Fopt-mochitest-devtools-chrome-e10s-5%2CM%28dt5%29

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=267987562&repo=autoland&lineNumber=66002

Backout link: https://hg.mozilla.org/integration/autoland/rev/21d079adc41d172d4bba1c8f2481345abf2a6a4b

[task 2019-09-23T16:42:11.721Z] 16:42:11 INFO - TEST-PASS | devtools/client/webconsole/test/browser/browser_webconsole_network_messages_openinnet.js | The attached url is correct. -
[task 2019-09-23T16:42:11.722Z] 16:42:11 INFO - Wait for the netmonitor headers panel to appear as it spawn RDP requests
[task 2019-09-23T16:42:11.722Z] 16:42:11 INFO - Wait for the netmonitor headers panel to appear as it spawn RDP requests
[task 2019-09-23T16:42:11.722Z] 16:42:11 INFO - Wait for the event timings request which do not necessarily update the UI as timings may be undefined for cached requests
[task 2019-09-23T16:42:11.722Z] 16:42:11 INFO - Buffered messages finished
[task 2019-09-23T16:42:11.723Z] 16:42:11 INFO - TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser/browser_webconsole_network_messages_openinnet.js | Test timed out -
[task 2019-09-23T16:42:13.281Z] 16:42:13 INFO - Removing tab.
[task 2019-09-23T16:42:13.282Z] 16:42:13 INFO - Waiting for event: 'TabClose' on [object XULElement].
[task 2019-09-23T16:42:13.343Z] 16:42:13 INFO - Got event: 'TabClose' on [object XULElement].
[task 2019-09-23T16:42:13.384Z] 16:42:13 INFO - Tab removed and finished closing
[task 2019-09-23T16:42:13.474Z] 16:42:13 INFO - GECKO(2542) | MEMORY STAT | vsize 20977515MB | residentFast 3807MB
[task 2019-09-23T16:42:13.474Z] 16:42:13 INFO - TEST-OK | devtools/client/webconsole/test/browser/browser_webconsole_network_messages_openinnet.js | took 92145ms

Comment 22

[Vincent Lequertier]

My bad, I have only checked the tests for the netmonitor part, not the whole devtools test suite.

Comment 23

[Vincent Lequertier]

I have found the issue. I will do a try push to make sure everything is fine this time.

Comment 24

[Vincent Lequertier]

Try build https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3d027ed64c348914e1169595562dbcf3e0a750b&selectedJob=268596990

Comment 25

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This is very close to landing, the patch just needs to be rebased on top of the latest m-c

See my comment in phab:
https://phabricator.services.mozilla.com/D45139#1441883

Honza

Comment 26

[Vincent Lequertier]

The conflict should be gone, please confirm :-)

Vincent

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cae70059c0fa
Hide the waterfall column when the netmonitor sidebar is opened;r=Honza

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Vincent Lequertier from comment #26)

The conflict should be gone, please confirm :-)

Awesome, thanks!

I can cleanly apply it now so, landed.
Honza

Comment 29

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/cae70059c0fa