1360457 - Network monitor columns are shifted

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Tim Nguyen :ntim]

Attachment: bug-columns.png

Comment 1

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

Thanks for reporting this Tim!

Honza

Comment 2

[Ricky Chien [:rickychien] (inactive)]

This issue can be seen when enabling physical scrollbar on macOS.

1. Enable permanent scrollbars on OSX
2. System prefs -> General -> Show Scrollbars: Always

Comment 3

[Johann Hofmann [:johannh]]

FYI mozscreenshots caught this, too (we're a bit behind on triaging). Screenshots of all platforms:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=e17cbb839dd225a2da7e5d5bec43cf94e11749d8&newProject=mozilla-central&newRev=62b649c6b314f756f21cb95f2b0d491e2664e944

Comment 4

[tera_1225]

Hello, 
I am a first time contributor, and have a working build environment set up. I was wondering if I could work on this bug, and if anybody could help me get going by pointing me to a starting point.
Thanks for any help.
Regards,
Mark.

Comment 5

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

Great! I assigned it to you.

* The network monitor panel code-base is here: http://searchfox.org/mozilla-central/source/devtools/client/netmonitor
* The architecture is based on React & Redux
* The Waterfall columns is here: http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/request-list-column-waterfall.js
* Perhaps the issue could be somewhere in this place? http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/request-list-column-waterfall.js#49

Honza

Comment 6

[tera_1225]

Great, thanks for the pointers [:Honza], will get too it now!

Comment 7

[Abhinav Koppula]

Hi Honza,
I have quickly taken a look at this and I had figured out that the mis-alignment is happening because of the scrollbar in `requests-list-contents` class - so if we have the `overflow` as `hidden`, I see that the headers and the columns align fine, but it is because of the scrollbar which comes if there are too many network requests which causes the issue.

My question is - Have we faced such an issue before where we have seen mis-aligning happening because of scrollbars and if yes, how do we generally go about fixing such issues?

Comment 8

[tera_1225]

Hello Abhinav,

I have been looking into this bug also. I had gathered the same as you: that it is a scroll-bar appearing issue.
It is related to the fact the when requests are too numerous to fit on one vertical pane, the content of the requests is resized horizontally to accommodate the scrollbar, however the headers bar is not resized: that is how the mismatch occurs.
Selector wise: .requests-list-contents overflows with a scrollbar, but obviously .requests-list-headers wants to stay put so there is no scrolling there, and the width becomes wrong.
It seems there is controversy over how browsers should report width when scrollbars appear/disappear. Ample discussion can be found on this issue here: https://github.com/w3c/csswg-drafts/issues/92 and there is mention of it in the CSS3 specs: https://drafts.csswg.org/css-overflow-3/#scrollable
CSS4 is aiming to offer a "scrollbar gutter" to pre-allocate space for when a scrollbar would appear.
I am dubious as to what should be done to fix this. Can anybody come up with a pure CSS fix? I have been scratching my head and trying various things but can't seem to get anywhere. 
Otherwise it could be done with some javascript, along the lines of this: https://jsfiddle.net/5soc2z2f/#&togetherjs=hNw0P7lJMr
Or, another hack that is in the code is to use a css variable, that can be set as a DOM property and used in the other element to size it. This is done for the waterfall already, here:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/request-list-content.js#101

I could sure use some help on this, as my knowledge of React is far too small to be able to hack this together myself. I am trying to implement something similar to the waterfall hack - and to be honest am not even sure that's the right way to go - but I will need a little more time to get a fix myself, and I have no problem with somebody else having a shot now if you have the time.

Regards,
Mark.

Comment 9

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

(In reply to Abhinav Koppula from comment #7)
> My question is - Have we faced such an issue before where we have seen
> mis-aligning happening because of scrollbars and if yes, how do we generally
> go about fixing such issues?

Hi Abhinav,
I think that Mark summarizes the issue quite well in comment #8.

@ntim: any ideas how to properly fix this?

Note that the entire layout might be change quite a bit when bug 1358414 lands, but it might take some time yet.

Honza

Comment 10

[Tim Nguyen :ntim]

(In reply to Jan Honza Odvarko [:Honza] from comment #9)
> @ntim: any ideas how to properly fix this?

What I would try doing would be moving the overflow to the parent element containing both the header and the request list, then applying position: sticky; top: 0; to the header.

Comment 11

[tera_1225]

@ntim : now that sounds like it can be done relatively easily! I never thought of "sticky"... I'll keep on trying then.
As for bug 1358414, with a bit of luck, if we only modify the css for request-container and the resizing implies modifications to the request-header-* elements, it might just still work anyway.

Comment 12

[tera_1225]

I think I have a fix for this, only it is causing a mochitest to fail and I can't seem to figure out why: when I reproduce the test "by hand", the browser does what it should...
I have a try push here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=24c187c212899d5faba67c05117ec1820ce3cd08&selectedJob=133836280
Any comments would be much appreciated.
Thanks in advance.

Comment 13

[tera_1225]

Attachment: Proposed patch - causes mochitest dt1 to fail

Comment 14

[Tim Nguyen :ntim]

Thanks for working on this!

I haven't looked in detail yet, but what you might want to check is whether the selectors (querySelector) in the tests are still correct, if they're not, you should update them.

Comment 15

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

(In reply to tera_1225 from comment #13)
> Created attachment 8914200 [details] [diff] [review]
> Proposed patch - causes mochitest dt1 to fail

So, I am seeing the following tests to fail:

* devtools/client/netmonitor/test/browser_net_autoscroll.js 
It looks like the `auto scroll to bottom` feature is broken by the patch
Search for `shouldScrollBottom` in `request-list-content.js` file
It's probably caused by adding the new wrapper element.

* devtools/client/netmonitor/test/browser_net_columns*
4 tests related to column/visibility. At least one of the problems
is that the header is not visible when the net panel is empty.
(it was visible before).

all these failures are nicely reproducible locally, so you can just
run them using:

`mach test devtools/client/netmonitor/test/browser_net_autoscroll.js`

... in your m-c directory.

Honza

Comment 16

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

Comment on attachment 8914200 [details] [diff] [review]
Proposed patch - causes mochitest dt1 to fail

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

F- since there are tests failing.

Thanks for working on this it's nice improvement!
Honza

Comment 17

[Eugene Timokhov]

Attachment: screenshot.png

Hi guys,
if I add a new column into table then diff of shift is more than scrollbar width. It will be very nice if it will be fixed in this issue too.

Comment 18

[tera_1225]

Hi Eugene. I'll take a look try and see if my proposed fix will also solve the issue you are describing with adding a new column. I am still working on getting my solution to pass the tests that it was failing and can't really say when that will be done as I'm rather busy right now, but I'll be sure to check what you're describing anyhow.
Regards,
Mark.

Comment 19

[tera_1225]

Attachment: Bug1360457-new.patch

I have modified my patch, and also the test somewhat to account for css selector changes as you mentioned Tim (there was a "firstChild" that was no longer pointing to the right element because of my changes).
Test is now green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8aa6ccb158b955a3f6e0bc0e51dc6e9e773ccc06
Also, incidentally, what Eugene was describing seems fixed with this, to me. Let me know if it's not in your opinion.
I'm guessing now it would be a good idea to write a test for this particular bug?

Comment 20

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8916371 [details] [diff] [review]
Bug1360457-new.patch

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

Nice work tera_1225 \o/

Looking into your patch I can see that your fix is doing the right thing!

I'm not really sure whether it's easy to write test case for checking visual changes.

Especially for macOS, we need to figure out how to always enable scrollbar in System preferences. To not add too much obstacle for landing this patch, I'm in a favor of landing this fix without test case.

Please rebase your patch and feel free to add me as second reviewer :)

Comment 21

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

Comment on attachment 8916371 [details] [diff] [review]
Bug1360457-new.patch

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

I think Ricky has better knowledge of this code,
so ask him for any future review/feedback request.

From what I can tell the patch looks good.

Thanks for working on this!
Honza

Comment 22

[tera_1225]

Attachment: Proposed test to check for regression on this bug.

This is a proposed test... It doesn't actually discriminate mac OS scroll bar "always visible", but test's correctly (i.e. succeeds with the patch, fails without) if scroll bar always visible is active in mac OS. As the other cases (always hidden and hide on no-scroll) don't seem to affect column alignment maybe that's sufficient? If not as you said Rick, we can just skip the test, or with a bit more time I might be able to figure out a way to check that mac OS scroll-bar visible option more specifically. Let me know what you think.

Comment 23

[tera_1225]

Thanks Rick and Honza for looking into this and for the thumbs up! Glad to have a fix.
I've also attached a test with above comment, and I take note of the fact that you're the one to ask for review Rick - guess I should have headed over to the slack to ask who was best suited before really!
Regards,
Mark.

Comment 24

[Tim Nguyen :ntim]

The test seems to do what it should! What you could also test is if the computed width of the columns are equal to the header widths

Comment 25

[tera_1225]

Attachment: Proposed test to check for regression on this bug (with width).

Like this? Or just one of the two?

Comment 26

[Tim Nguyen :ntim]

Looks good, you can now combine it with the other patch and submit that one for review.

You should make sure to rebase on top of the latest source code as well.

Thanks for your work on this bug!

Comment 27

[tera_1225]

Attachment: Bug 1360457 - moved header into content, created wrapper around header, adapted css, fixed an old test, created new regression test.

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

Comment 28

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8923279 [details]
Bug 1360457 - moved header into content, created wrapper around header, adapted css, fixed an old test, created new regression test.

https://reviewboard.mozilla.org/r/194462/#review199452

Thanks for the hard work! I can see the misalignment issue has been fixed.

Please address open issues & nits.

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:174
(Diff revision 1)
>  
> +.empty-notice-element {
> +  padding-top: 12px;
> +  padding-left: 12px;
> +  padding-right: 12px;
> +  font-size: 120%;

nit: font-size: 1.2rem;

::: devtools/client/netmonitor/src/components/RequestList.js:24
(Diff revision 1)
>  const { div } = DOM;
>  
>  /**
>   * Request panel component
>   */
> -function RequestList({
> +function RequestList({isEmpty,}) {

nit: { isEmpty }

Remove redundant , and add spaces in-between.

::: devtools/client/netmonitor/src/components/RequestListContent.js:23
(Diff revision 1)
>  const RequestListItem = createFactory(require("./RequestListItem"));
>  const RequestListContextMenu = require("../request-list-context-menu");
> +const RequestListHeader = createFactory(require("./RequestListHeader"));

nit: define components in alphabetical order

```
const RequestListHeader = createFactory(require("./RequestListHeader"));
const RequestListItem = createFactory(require("./RequestListItem"));
const RequestListContextMenu = require("../request-list-context-menu");
```

::: devtools/client/netmonitor/src/components/RequestListEmptyNotice.js:18
(Diff revision 1)
>  const { connect } = require("devtools/client/shared/vendor/react-redux");
>  const Actions = require("../actions/index");
>  const { ACTIVITY_TYPE } = require("../constants");
>  const { L10N } = require("../utils/l10n");
>  const { getPerformanceAnalysisURL } = require("../utils/mdn-utils");
> +const RequestListHeader = createFactory(require("./RequestListHeader"));

nit: move compoent definition below after // Components and make sure in alphabeticall order.

Comment 29

[Tim Nguyen :ntim]

Comment on attachment 8923279 [details]
Bug 1360457 - moved header into content, created wrapper around header, adapted css, fixed an old test, created new regression test.

https://reviewboard.mozilla.org/r/194462/#review199472

Looks good! Please address the comment below in addition to Ricky's comments

::: devtools/client/netmonitor/src/components/RequestListContent.js:262
(Diff revision 1)
>              ref: "contentEl",
>              className: "requests-list-contents",
>              tabIndex: 0,
>              onKeyDown: this.onKeyDown,
>            },
> +          RequestListHeader(),

This should be indented one more level in.

Comment 30

[tera_1225]

Comment on attachment 8923279 [details]
Bug 1360457 - moved header into content, created wrapper around header, adapted css, fixed an old test, created new regression test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194462/diff/1-2/

Comment 31

[Tim Nguyen :ntim]

Comment on attachment 8923279 [details]
Bug 1360457 - moved header into content, created wrapper around header, adapted css, fixed an old test, created new regression test.

https://reviewboard.mozilla.org/r/194462/#review199696

::: devtools/client/netmonitor/src/components/RequestListContent.js:256
(Diff revision 2)
>            div({
> -            ref: "contentEl",
> +              ref: "contentEl",
> -            className: "requests-list-contents",
> +              className: "requests-list-contents",
> -            tabIndex: 0,
> +              tabIndex: 0,
> -            onKeyDown: this.onKeyDown,
> +              onKeyDown: this.onKeyDown,
> -          },
> +            },
> +            RequestListHeader(),

The object should keep the old indentation.
Here's what it should look like:

div({
  ref: "contentEl
  ...
},
  RequestListHeader()

Comment 32

[tera_1225]

Comment on attachment 8923279 [details]
Bug 1360457 - moved header into content, created wrapper around header, adapted css, fixed an old test, created new regression test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194462/diff/2-3/

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91de4b003648
moved header into content, created wrapper around header, adapted css, fixed an old test, created new regression test. r=ntim,rickychien

Comment 34

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/439754acc61c for eslint bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=140843429&repo=autoland

Comment 35

[Ricky Chien [:rickychien] (inactive)]

We didn't notice there are some eslint errors.

Tera, please address eslint error and land the patch again. Thanks :)

Comment 36

[tera_1225]

Comment on attachment 8923279 [details]
Bug 1360457 - moved header into content, created wrapper around header, adapted css, fixed an old test, created new regression test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194462/diff/3-4/

Comment 37

[tera_1225]

Sorry about that, those errors were introduced when I rebased - I had a hard time understanding the vimdiff interface, and obviously selected some elements from the wrong versions of the files...
Should be fixed, I reopened the review request.
Thanks for all your patience and work on this.
Regards,
Mark.

Comment 38

[Ricky Chien [:rickychien] (inactive)]

Good job Mark. Thanks :)

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/083a5838f76a
moved header into content, created wrapper around header, adapted css, fixed an old test, created new regression test. r=ntim,rickychien

Comment 40

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/083a5838f76a

Comment 41

[Eugene Timokhov]

Attachment: shifted-columns

I just installed Firefox Nightly 59.0a1 (2018-01-10) (64-bit) and seems that columns still are shifted.

Comment 42

[tera_1225]

I am unable to reproduce this with Firefox Nightly 59.0a1 (2018-01-10)… I have tried Firebug, light and dark themes and everything seems ok. There was a mochi test included in the fix for this particular bug, so if there had been a regression it should have been caught. Could you perhaps give some more info about your setup please?

Comment 43

[Eugene Timokhov]

Sure! What kind of setup do you need?
After opening devtools I added 1 column - Content-Encoding.

Comment 44

[tera_1225]

Ah that's it! When adding the new column I can indeed reproduce… This is actually a distinct bug though. I have just checked and even before the commit from this bug fix was included the behavior you're seeing was already there. If you don't mind, I think it would be best to file a separate bug for this. Thanks for reporting this anyhow!

Comment 45

[Eugene Timokhov]

Okay, I just created another one for this problem https://bugzilla.mozilla.org/show_bug.cgi?id=1429721. Thanks a lot!