Closed Bug 1339558 Opened 4 years ago Closed 1 year ago

Net monitor chart monitor chart view overflow

Categories

(DevTools :: Netmonitor, defect, P3)

defect

Tracking

(firefox76 verified)

VERIFIED FIXED
Firefox 76
Tracking Status
firefox76 --- verified

People

(Reporter: leonardo.couto, Assigned: aarushivij, Mentored)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [netmonitor-reserve])

Attachments

(12 files, 9 obsolete files)

34.54 KB, image/png
Details
123.61 KB, image/png
Details
28.05 KB, image/png
Details
28.23 KB, image/png
Details
45.25 KB, image/png
Details
30.67 KB, image/png
Details
38.92 KB, image/png
Details
1.92 KB, patch
ntim
: review+
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
30.38 KB, image/png
Details
63.88 KB, image/png
Details
47 bytes, text/x-phabricator-request
Details | Review
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.
Flags: qe-verify?
Whiteboard: [netmonitor][triage]
This issue will be fixed in bug 1309826.
Priority: -- → P3
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Attached image netperf.png
I don't think this is fixed yet. See the attached image.

@Leonardo, can you confirm?

Honza
Flags: needinfo?(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.
Flags: needinfo?(leonardo.couto)
Flags: qe-verify? → qe-verify+
Assignee: nobody → ntim.bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Depends on: 1316291
QA Contact: ciprian.georgiu
@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 :)
Flags: needinfo?(odvarko)
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.
Assignee: ntim.bugs → djmdeveloper060796
Flags: needinfo?(odvarko)
@Ricky Thanks a lot for the info :) I will upload a patch soon
Attached patch bug1339558.patch (obsolete) — Splinter Review
Left aligned the text under transferred column and moved table under pie chart
Attachment #8839227 - Flags: review?(odvarko)
Attached image 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 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
Attachment #8839227 - Flags: review?(odvarko)
@Honza, thanks for the pointers I will upload a patch soon
Duplicate of this bug: 1341597
Attached patch bug1339558.patch (obsolete) — Splinter Review
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.
Attachment #8839227 - Attachment is obsolete: true
Attachment #8840822 - Flags: review?(odvarko)
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
Attachment #8840822 - Flags: review?(odvarko) → review-
Attached patch bug1339558.patch (obsolete) — Splinter Review
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.
Attachment #8840822 - Attachment is obsolete: true
Attachment #8841712 - Flags: review?(odvarko)
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
Attachment #8841712 - Flags: review?(odvarko) → review-
Attached patch bug1339558.patch (obsolete) — Splinter Review
@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.
Attachment #8841712 - Attachment is obsolete: true
Attachment #8842337 - Flags: review?(odvarko)
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
Attachment #8842337 - Flags: review?(odvarko)
Attached patch bug1339558.patch (obsolete) — Splinter Review
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
Attachment #8842337 - Attachment is obsolete: true
Attachment #8842538 - Flags: review?(odvarko)
Blocks: netmonitor-phaseII
No longer blocks: netmonitor-html
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
Attachment #8842538 - Flags: review?(odvarko) → review-
Any progress with this bug?

It would be so great to have this landed!

Honza
Flags: needinfo?(djmdeveloper060796)
@Honza I was busy with some work, I will upload a patch ASAP
Flags: needinfo?(djmdeveloper060796)
Attached patch bug1339558.patch (obsolete) — Splinter Review
@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?
Attachment #8842538 - Attachment is obsolete: true
Attachment #8855649 - Flags: review?(odvarko)
(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
Flags: needinfo?(ntim.bugs)
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
Flags: needinfo?(ntim.bugs)
Attached patch bug1339558.patch (obsolete) — Splinter Review
@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.
Flags: needinfo?(ntim.bugs)
Attachment #8856089 - Flags: review?(odvarko)
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
Attached patch bug1339558.patch (obsolete) — Splinter Review
@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.
Attachment #8856089 - Attachment is obsolete: true
Attachment #8856089 - Flags: review?(odvarko)
Attachment #8856103 - Flags: review?(odvarko)
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.
My last comment solves the issue you brought up in comment 32
Flags: needinfo?(ntim.bugs)
Attached patch bug1339558.patchSplinter Review
@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.
Attachment #8855649 - Attachment is obsolete: true
Attachment #8856103 - Attachment is obsolete: true
Attachment #8855649 - Flags: review?(odvarko)
Attachment #8856103 - Flags: review?(odvarko)
Attachment #8856242 - Flags: review?(ntim.bugs)
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.
Attachment #8856242 - Flags: review?(ntim.bugs)
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.
Attachment #8856242 - Flags: review?(ntim.bugs)
@Tim I justed checked after adding min-width:0, its not working
Flags: needinfo?(ntim.bugs)
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.
Attachment #8856242 - Flags: review?(ntim.bugs) → review+
Flags: needinfo?(ntim.bugs)
@Tim I would love to help in redesigning the view. Do let know if I can be of any help :)
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
Product: Firefox → DevTools

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.

Assignee: djmdeveloper060796 → nobody
Status: ASSIGNED → NEW

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

Attached image 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

Flags: needinfo?(im.toms.inbox)
Assignee: nobody → im.toms.inbox
Status: NEW → ASSIGNED

(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!

(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

Flags: needinfo?(im.toms.inbox)

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.

Flags: needinfo?(odvarko)

Thanks for the ping Patrick!

I commented in Phab

Honza

Flags: needinfo?(odvarko)

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.

Flags: needinfo?(im.toms.inbox)
Assignee: im.toms.inbox → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(im.toms.inbox)
Attached image 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)

Assigned to you

Honza

Assignee: nobody → aarushivij
Status: NEW → ASSIGNED

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

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

Flags: needinfo?(florens)

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

Flags: needinfo?(florens)

Created a new patch.
Please review :)

Flags: needinfo?(odvarko)
Attachment #9132817 - Attachment description: Bug 1339558 - Net monitor chart monitor chart view overflow Honza → Bug 1339558 - Net monitor chart monitor chart view overflow
Attachment #9132817 - Attachment description: Bug 1339558 - Net monitor chart monitor chart view overflow → Bug 1339558 - Net monitor chart monitor chart view overflow Honza
Attachment #9127136 - Attachment is obsolete: true
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9aa8d616b424
Net monitor chart monitor chart view overflow  Honza r=Honza,fvsch
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 76

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
No longer regressions: 1627266
Flags: needinfo?(odvarko)
You need to log in before you can comment on or make changes to this bug.