Closed
Bug 1360457
Opened 8 years ago
Closed 7 years ago
Network monitor columns are shifted
Categories
(DevTools :: Netmonitor, defect, P3)
DevTools
Netmonitor
Tracking
(firefox58 fixed)
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: ntim, Assigned: tera_1225)
References
Details
(Keywords: good-first-bug)
Attachments
(6 files, 2 obsolete files)
No description provided.
Comment 1•8 years ago
|
||
Thanks for reporting this Tim!
Honza
Keywords: good-first-bug
Priority: -- → P3
Comment 2•8 years ago
|
||
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•7 years ago
|
||
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
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)
Comment 5•7 years ago
|
||
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)
Great, thanks for the pointers [:Honza], will get too it now!
Comment 7•7 years 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)
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•7 years ago
|
||
(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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Attachment #8914200 -
Flags: feedback?(odvarko)
Attachment #8914200 -
Flags: feedback?(ntim.bugs)
Reporter | ||
Comment 14•7 years 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)
Comment 15•7 years ago
|
||
(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 16•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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•7 years ago
|
||
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•7 years ago
|
Attachment #8916371 -
Flags: feedback?(rchien)
Comment 20•7 years ago
|
||
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 21•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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•7 years 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•7 years ago
|
||
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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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
Comment 34•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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)
Comment 38•7 years ago
|
||
Good job Mark. Thanks :)
Comment 39•7 years 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•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 41•7 years ago
|
||
I just installed Firefox Nightly 59.0a1 (2018-01-10) (64-bit) and seems that columns still are shifted.
Assignee | ||
Comment 42•7 years 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•7 years 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•7 years 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)
Comment 45•7 years ago
|
||
Okay, I just created another one for this problem https://bugzilla.mozilla.org/show_bug.cgi?id=1429721. Thanks a lot!
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•