1339558 - Net monitor chart monitor chart view overflow

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Leonardo Couto]

One thing that I noticed after the addition of https://bugzilla.mozilla.org/show_bug.cgi?id=1168376 is that the empty cache data table is overflowing the screen to the right. Don't know if this should be solved on this ticket on in a new one, but anyway here some of alternatives at a first look that I thought to solve it:

1) Reduce spaces to accommodate the tables and charts.
2) Let the chart overflow how it is right now and make the line in middle draggable.
3) Put the empty cache chart on bottom and enable vertical scrolling.
4) Reduce font size, pie chart size or both.
5) ?

Need further discussion.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

This issue will be fixed in bug 1309826.

Comment 2

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

Attachment: netperf.png

I don't think this is fixed yet. See the attached image.

@Leonardo, can you confirm?

Honza

Comment 3

[Leonardo Couto]

(In reply to Jan Honza Odvarko [:Honza] from comment #2)
> Created attachment 8838103 [details]
> netperf.png
> 
> I don't think this is fixed yet. See the attached image.
> 
> @Leonardo, can you confirm?
> 
> Honza

Yeah, can confirm. The empty cache chart is still overflowing.

Comment 4

[Deepjyoti Mondal]

@Honza I would like to work on this bug, It would be nice if you could point me out the files I need to look into inorder to solve this bug :)

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Here you go!

Deepjyoti Mondal,

Chart view overflow issue was supposed to be fixed in bug 1309826 but it didn't, because a new "transferred" column was added recently (as you see in attachment 8838103 [details]).

And I also found a text-align issue in "transferred" column, the size XXX KB should text-align from left. To fix this, it's easy to set "text-align: start;" to a new CSS rule .table-chart-row-label[name="transferredSize"] in netmonitor.css.

Comment 6

[Deepjyoti Mondal]

@Ricky Thanks a lot for the info :) I will upload a patch soon

Comment 7

[Deepjyoti Mondal]

Attachment: bug1339558.patch

Left aligned the text under transferred column and moved table under pie chart

Comment 8

[Tim Nguyen :ntim]

Attachment: cutoff chart.png

The chart seems cut-off with your patch, and it doesn't seem possible to see all of it by scrolling up.

I really think we should keep the behaviour we had before netmonitor-html, i.e. having the charts show at the left of the legend to make use of all of the available space.

Comment 9

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

Comment on attachment 8839227 [details] [diff] [review]
bug1339558.patch

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

Thanks for working on this Deepjyoti!

Two issues:
1) When I shrink browser window to cause DevTools UI to switch to Portrait (vertical) mode, charts are cut off and there is no scroll bar that would allow to scroll the content. See the screenshot chars-portrait.png
2) Similar thing happens also in Landscape (horizontal) mode. This is what ntim is referring to. Interesting is the there is no scrollbar for one of the charts, but mouse wheel works fine


I think that we could do the following:

1) Keep the charts at the left of the text (like ntim is suggesting) in Landscape mode and introduce scrollbar that can be used to scroll both charts at once. (vertical and even horizontal scrollbar should be available if necessary) 
2) Display the charts at the top of the text in Portrait mode and again introduce scrollbar that can be used to scroll both charts at once. See the attached screenshot charts-scrollbar.png
3) Optionally, we might also shrink the font if there is not enough space (just like the charts itself are shrinking).

Honza

Comment 10

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

Attachment: chars-portrait.png

Comment 11

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

Attachment: charts-scrollbar.png

Comment 12

[Deepjyoti Mondal]

@Honza, thanks for the pointers I will upload a patch soon

Comment 14

[Deepjyoti Mondal]

Attachment: bug1339558.patch

I have added vertical and horizontal scrollbars which can be used to scroll both the charts at once.I have moved the charts above the tables in portrait mode.
On squeezing the browser the charts and the tables are visible (with charts on top of tables) but on squeezing further some of the left portion of the tables is cutoff(horizontal scrolling is unable to show those portions). If this is unwanted then I guess we can add another breakpoint (at around 400px perhaps) and add some rules to save space and bring the entire table into view.I have not modified the font-size yet.

Comment 15

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

Comment on attachment 8840822 [details] [diff] [review]
bug1339558.patch

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

Thanks for the update!

My comments:

1) If I dock DevTools Toolbox to the right (browser) side the horizontal scrollbar doesn't allow to scroll even if I don't see the legend (labels). There is also weird whit gap at the right side. See attached charts1.png file

2) Similar problem is when the Toolbox is docked at the bottom of browser window (default). I can't see titles (Primed cache and Empty cache) and the vertical scroll bar doesn't allow to scroll. See attached charts2.png

3) Both scroll-bars are visible all the time - often disabled. Can we hide them when they are not necessary?

Honza

Comment 16

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

Attachment: charts1.png

Comment 17

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

Attachment: charts2.png

Comment 18

[Deepjyoti Mondal]

Attachment: bug1339558.patch

I have removed the white space which was present at the right side when the devtools is docked.Now the legend can be viewed by horizontal scrolling.
Now the scroll bars appear only when necessary.
When the devtools toolbox is present at bottom, the headings can be viewed using vertical scrolling.

Comment 19

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

Comment on attachment 8841712 [details] [diff] [review]
bug1339558.patch

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

Looks a lot better!

I am yet seeing that the splitter is not scrolling properly, see the attached screenshot (charts-splitter.png).

STR:
1) Dock the Toolbox at the bottom
2) Make the height small so, the verticall scrollbar appears.
3) Scrolll the content, the splitter doesn't scroll -> BUG

Also, it would be great if the content has some padding (10px?). When scrolling the content it's often just at the edge.


Honza

Comment 20

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

Attachment: charts-splitter.png

Comment 21

[Deepjyoti Mondal]

Attachment: bug1339558.patch

@Honza 

I have added padding to the content for both horizontal and vertical modes.
When the toolbox is docked at the bottom, I have tried to make the splitter scroll. I did so by adding multiple height media queries and adjusting the splitter's height (in %). I do not think this is the best way. I have used it as a work around. I am trying to find out a better way.

Comment 22

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

Comment on attachment 8842337 [details] [diff] [review]
bug1339558.patch

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

(In reply to Deepjyoti Mondal from comment #21)
> Created attachment 8842337 [details] [diff] [review]
> bug1339558.patch
> 
> @Honza 
> 
> I have added padding to the content for both horizontal and vertical modes.
> When the toolbox is docked at the bottom
I am seeing that the padding is 100px/150px. Can you please make it 10px.

> I have tried to make the splitter
> scroll. I did so by adding multiple height media queries and adjusting the
> splitter's height (in %). I do not think this is the best way. I have used
> it as a work around. I am trying to find out a better way.
It fixes the problem but, agreed better way is needed.

Honza

Comment 23

[Deepjyoti Mondal]

Attachment: bug1339558.patch

I have reduced the top and bottom padding to 10px. But on doing so the headings are not visible on reducing the height of the toolbox below a certain amount

Comment 24

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

Comment on attachment 8842538 [details] [diff] [review]
bug1339558.patch

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

I don't understand why we need so many new CSS rules.

The following should be enough to set the scrollbars + 20px margin 

diff --git a/devtools/client/themes/netmonitor.css b/devtools/client/themes/netm--- a/devtools/client/themes/netmonitor.css
+++ b/devtools/client/themes/netmonitor.css
@@ -878,28 +878,24 @@
   cursor: default;
   pointer-events: none;
   height: 100%;
 }

 .statistics-panel .charts-container {
   display: flex;
   width: 100%;
-}
-
-.statistics-panel .charts,
-.statistics-panel .pie-table-chart-container {
-  width: 100%;
-  height: 100%;
+  overflow: auto;
 }

 .pie-table-chart-container {
   display: flex;
   justify-content: center;
   align-items: center;
+  margin: 20px;
 }

 .statistics-panel .pie-chart-container {
   margin-inline-start: 3vw;
   margin-inline-end: 1vw;
 }

 .statistics-panel .table-chart-container {



---

There is problem with the splitter (see comment #19).
Can you look into the splitter problem?

(in case it's hard to fix the splitter:
Note, that single 1px line would be better than a
fake splitter anyway)


Honza

Comment 25

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

Any progress with this bug?

It would be so great to have this landed!

Honza

Comment 26

[Deepjyoti Mondal]

@Honza I was busy with some work, I will upload a patch ASAP

Comment 27

[Deepjyoti Mondal]

Attachment: bug1339558.patch

@Honza I did the changes you mentioned. The scrollbar is working fine. In vertical view the splitter is causing no problem.But in the horizontal view to make the splitter scrollable I have to use multiple media queries.I am trying to find a better and less hacky way. What do suggest regarding the vertical splitter?

Comment 28

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

(In reply to Deepjyoti Mondal from comment #27)
> Created attachment 8855649 [details] [diff] [review]
> bug1339558.patch
> 
> @Honza I did the changes you mentioned. The scrollbar is working fine. In
> vertical view the splitter is causing no problem.But in the horizontal view
> to make the splitter scrollable I have to use multiple media queries.I am
> trying to find a better and less hacky way. What do suggest regarding the
> vertical splitter?
I see, thanks for the investigation!

@ntim: any ideas how to simplify the CSS here?

Honza

Comment 29

[Tim Nguyen :ntim]

Not sure how the patch is supposed to work, but to get the old (correct behaviour), adding this seems enough:

@media (max-width: 800px) {
  .statistics-panel .charts-container {
    display: block;
    overflow-y: auto;
    overflow-x: hidden;
  }
  .statistics-panel .splitter {
    height: 1px;
  }
}


and overflow: auto; to .statistics-panel .charts,
.statistics-panel .pie-table-chart-container

Comment 30

[Deepjyoti Mondal]

Attachment: bug1339558.patch

@Honza I did the changes which Tim suggested. Now in horizontal view both the chart-table sections has got a vertical scroll bar attached to them (as expected), so they can be scrolled independent of each other. I have removed vertical scroll bar from the parent div (charts-container) as I guess there is no need to vertically scroll this entire div (both the charts simultaneously) any more as we are able to scroll each section separately.The horizontal scroll bar is there as it was.

In the vertical view, horizontal scroll bars are now attached to each of the two sections, however the scrollbars are not occupying the entire horizontal area. I guess there is some problem with margin and padding. 
Alternatively in the vertical view we can use a single horizontal scrollbar to scroll both the charts at once as it was just before and remove the individual horizontal scrollbars.The splitter did not cause any trouble in the vertical view so this approach will not cause any problem hopefully.

Comment 31

[Tim Nguyen :ntim]

Comment on attachment 8856089 [details] [diff] [review]
bug1339558.patch

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

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css
@@ +1355,4 @@
>    }
>  
>    .statistics-panel .splitter {
> +    width: 130%;

Why is this needed ?

@@ +1361,5 @@
> +
> +  .pie-table-chart-container {
> +    flex-direction: column;
> +    padding-bottom: 25px;
> +    padding-top: 25px;

Same question here

@@ +1367,5 @@
> +}
> +
> +@media (max-width: 600px) {
> +  .pie-table-chart-container {
> +    padding-left: 150px;

Same here

Comment 32

[Deepjyoti Mondal]

Attachment: bug1339558.patch

@Honza I have made some changes to the previous patch
@Tim Thanks for pointing out :)
I have removed the breakpoint at 600px as its not necessary. I have applied padding-left:150px so that in vertical view the leftmost part of the table is not cut off (without padding the leftmost part of the chart is not visible). 
Also I added some top and bottom padding so that the content is not at the edge.

Comment 33

[Tim Nguyen :ntim]

Comment on attachment 8856103 [details] [diff] [review]
bug1339558.patch

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

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css
@@ +1291,5 @@
> +@media (max-width: 800px) {
> +  .statistics-panel .charts-container {
> +    display: block;
> +    overflow-y: auto;
> +    overflow-x: hidden;

overflow: auto; (instead of y: auto and x: hidden) allows you to scroll on both directions.

Given that
.statistics-panel .charts, .statistics-panel .pie-table-chart-container is set to overflow: visible;

for max-width: 800px;

@@ +1355,4 @@
>    }
>  
>    .statistics-panel .splitter {
> +    width: 130%;

This change is unnecessary with my suggested changes.

@@ +1361,5 @@
> +
> +  .pie-table-chart-container {
> +    flex-direction: column;
> +    padding-bottom: 25px;
> +    padding-top: 25px;

The padding is also not needed, since there's already a 20px margin.

@@ +1367,5 @@
> +}
> +
> +@media (max-width: 600px) {
> +  .pie-table-chart-container {
> +    padding-left: 150px;

Same.

Comment 34

[Tim Nguyen :ntim]

My last comment solves the issue you brought up in comment 32

Comment 35

[Deepjyoti Mondal]

Attachment: bug1339558.patch

@Tim Thanks for the pointers. I removed the extra paddings and I also removed overflow:auto from pie-table-chart-container. Now in the vertical view the horizontal scroll bars are taking the entire horizontal area and there is no more white space. 
Only problem is in the vertical view the leftmost portion of the table is getting cutoff.

Comment 36

[Tim Nguyen :ntim]

Comment on attachment 8856242 [details] [diff] [review]
bug1339558.patch

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

Adding min-width: 0; to .statistics-panel .charts, .statistics-panel .pie-table-chart-container

seems to solve the issue you've mentioned.

Comment 37

[Tim Nguyen :ntim]

Comment on attachment 8856242 [details] [diff] [review]
bug1339558.patch

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

Whoops, looks like I've tested the wrong patch.

Comment 38

[Deepjyoti Mondal]

@Tim I justed checked after adding min-width:0, its not working

Comment 39

[Tim Nguyen :ntim]

Comment on attachment 8856242 [details] [diff] [review]
bug1339558.patch

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

I think the patch is fine for now. We plan to completely redesign this view, so I don't think we should spend too much time on fixing this view for very small widths (< 500px) which is not a very common configuration.

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css
@@ +1356,1 @@
>      height: 1px;

I would remove this too, as this is duplicated code from above.

Comment 40

[Deepjyoti Mondal]

@Tim I would love to help in redesigning the view. Do let know if I can be of any help :)

Comment 41

[Tim Nguyen :ntim]

(In reply to Deepjyoti Mondal from comment #40)
> @Tim I would love to help in redesigning the view. Do let know if I can be
> of any help :)

Bug 1015201 is filed for that, feel free to take it. It's not an easy bug though as it involves changing charts and layout. Not to mention tests that need to be fixed.

Comment 42

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 6 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.

Comment 43

[Thomas]

Attachment: Bug 1339558 - Fixed overflow related styling issues for the netmonitor charts view. r=honza

Make the chart view vertically and horizontally scrollable when content
overflows so the information is accessible on all sidebar sizes/orientations.

Comment 44

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

Attachment: image.png

Thanks for working on this!

I am seeing a gap between vertical scrollbar and the right side edge of the browser window (see the attached screenshot)

Also, could we have a gap (white space) between the two charts (again see the screenshot), so it's possible to see when one chart ends and the other begins. Ideally we would have the splitter there in this case!

Honza

Comment 46

[lloan alas:[lloanalas]]

(In reply to Thomas from comment #43)

Created attachment 9048401 [details]
Bug 1339558 - Fixed overflow related styling issues for the netmonitor charts view. r=honza

Make the chart view vertically and horizontally scrollable when content
overflows so the information is accessible on all sidebar sizes/orientations.

Thomas,

Are you still interested in working on this? If not, I would like to help push it cross the finish line.

Let me know - thanks!

Comment 47

[Thomas]

(In reply to lloan alas:[lloanalas] from comment #46)

(In reply to Thomas from comment #43)

Created attachment 9048401 [details]
Bug 1339558 - Fixed overflow related styling issues for the netmonitor charts view. r=honza

Make the chart view vertically and horizontally scrollable when content
overflows so the information is accessible on all sidebar sizes/orientations.

Thomas,

Are you still interested in working on this? If not, I would like to help push it cross the finish line.

Let me know - thanks!

I think it's basically all been done, just waiting for review here:
https://phabricator.services.mozilla.com/D22053

Comment 48

[Patrick Brosset <:pbro>]

Honza, do you mind taking a look at this bug again? Thomas said it's all done in comment 47, and the patch on phabricator looks like it's waiting for a new review.

Comment 49

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

Thanks for the ping Patrick!

I commented in Phab

Honza

Comment 50

[Patrick Brosset <:pbro>]

Hey Thomas, are you still interested in fixing this bug? Honza did provide some comments on your code changes in phabricator.
If you just need more time that's fine. If you don't plan on working on this anymore, please let us know so others can pick up where you left off.

Comment 51

[aarushivij]

Attachment: piechartss.png

Hello
Can I work on this issue?
Still there are no scroll bars, on shrinking the browser window.
Also vertically too either padding or scrollbar is required.(As seen the time part is slightly cut off)

Comment 52

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

Assigned to you

Honza

Comment 53

[aarushivij]

Attachment: Bug 1339558 - Net monitor chart monitor chart view overflow. r=Honza

Comment 54

[aarushivij]

Hello :Honza
I have submitted the patch. Please review :)

Comment 55

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

Florens, could you please help with reviewing patch for this bug?
There are some previous attempts too and it looks like that solution for this isn't that easy as it might look like.

Thanks!
Honza

Comment 56

[Florens Verschelde :fvsch]

Sure, I'll take a look.
I remember trying to fix it previously with the Browser Toolbox but finding some edge cases.

Comment 57

[aarushivij]

Attachment: Bug 1339558 - Net monitor chart monitor chart view overflow Honza

Comment 58

[aarushivij]

Created a new patch.
Please review :)

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9aa8d616b424
Net monitor chart monitor chart view overflow  Honza r=Honza,fvsch

Comment 60

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9aa8d616b424

Comment 61

[Ciprian Georgiu, Desktop QA]

Verified that the scroll bars are shown in Network tab, on latest Nightly 76.0a1 under macOS 10.15, Ubuntu 18.04 x64 and Windows 10 x64.

While testing this, it appears that the line between the graphs is not correctly rendered while scrolling into Devtools panel. I filed bug 1627266 in order to track the issue.