Network monitor columns are shifted

RESOLVED FIXED in Firefox 58

Status

DevTools
Netmonitor
P3
normal
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: ntim, Assigned: tera_1225)

Tracking

({good-first-bug})

unspecified
Firefox 58
good-first-bug
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8862741 [details]
bug-columns.png
(Reporter)

Updated

a year ago
Blocks: 1349173
Thanks for reporting this Tim!

Honza
Keywords: good-first-bug
Priority: -- → P3
This issue can be seen when enabling physical scrollbar on macOS.

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

Comment 4

11 months ago
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.
Flags: needinfo?(odvarko)
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
Assignee: nobody → tera_1225
Flags: needinfo?(odvarko)
(Assignee)

Comment 6

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

Comment 7

10 months ago
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?
Flags: needinfo?(odvarko)
(Assignee)

Comment 8

10 months ago
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.
(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
Flags: needinfo?(odvarko) → needinfo?(ntim.bugs)
(Reporter)

Comment 10

10 months ago
(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.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 11

10 months ago
@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.
(Assignee)

Comment 12

10 months ago
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.
Flags: needinfo?(odvarko)
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 13

10 months ago
Created attachment 8914200 [details] [diff] [review]
Proposed patch - causes mochitest dt1 to fail
Attachment #8914200 - Flags: feedback?(odvarko)
Attachment #8914200 - Flags: feedback?(ntim.bugs)
(Reporter)

Comment 14

10 months ago
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.
Flags: needinfo?(tera_1225)
(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
Flags: needinfo?(odvarko)
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
Attachment #8914200 - Flags: feedback?(odvarko) → feedback-

Comment 17

10 months ago
Created attachment 8916316 [details]
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.
(Assignee)

Comment 18

10 months ago
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.
(Assignee)

Comment 19

10 months ago
Created attachment 8916371 [details] [diff] [review]
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?
Attachment #8914200 - Attachment is obsolete: true
Attachment #8914200 - Flags: feedback?(ntim.bugs)
Flags: needinfo?(tera_1225)
Flags: needinfo?(ntim.bugs)
Attachment #8916371 - Flags: feedback?(odvarko)
(Reporter)

Updated

9 months ago
Attachment #8916371 - Flags: feedback?(rchien)
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 :)
Attachment #8916371 - Flags: feedback?(rchien) → feedback+
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
Attachment #8916371 - Flags: feedback?(odvarko) → feedback+
(Assignee)

Comment 22

9 months ago
Created attachment 8922005 [details]
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.
Attachment #8922005 - Flags: review?(rchien)
(Assignee)

Comment 23

9 months ago
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.
(Reporter)

Comment 24

9 months ago
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
(Assignee)

Comment 25

9 months ago
Created attachment 8922049 [details]
Proposed test to check for regression on this bug (with width).

Like this? Or just one of the two?
Attachment #8922005 - Attachment is obsolete: true
Attachment #8922005 - Flags: review?(rchien)
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 26

9 months ago
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!
Flags: needinfo?(ntim.bugs)
Comment hidden (mozreview-request)

Comment 28

9 months ago
mozreview-review
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.
Attachment #8923279 - Flags: review?(rchien) → review+
(Reporter)

Comment 29

9 months ago
mozreview-review
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.
Attachment #8923279 - Flags: review?(ntim.bugs) → review+
Comment hidden (mozreview-request)
(Reporter)

Comment 31

9 months ago
mozreview-review
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 hidden (mozreview-request)

Comment 33

9 months ago
Pushed by ntim.bugs@gmail.com:
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
We didn't notice there are some eslint errors.

Tera, please address eslint error and land the patch again. Thanks :)
Flags: needinfo?(tera_1225)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

9 months ago
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.
Flags: needinfo?(tera_1225)
Good job Mark. Thanks :)

Comment 39

9 months ago
Pushed by rchien@mozilla.com:
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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/083a5838f76a
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Updated

9 months ago
Depends on: 1414220

Updated

7 months ago
Depends on: 1425839

Updated

7 months ago
Depends on: 1426018

Comment 41

6 months ago
Created attachment 8941468 [details]
shifted-columns

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

Comment 42

6 months ago
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?
Flags: needinfo?(timocov)

Comment 43

6 months ago
Sure! What kind of setup do you need?
After opening devtools I added 1 column - Content-Encoding.
Flags: needinfo?(timocov) → needinfo?(tera_1225)
(Assignee)

Comment 44

6 months ago
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!
Flags: needinfo?(tera_1225)

Updated

6 months ago
Blocks: 1429721

Comment 45

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

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.