1224201 - Cells in the memory tool's heap view need more minimum width

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P2)

Description

[magicp]

Attachment: devtools-memory-column-width.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151111030207

Steps to reproduce:

1. Run Nightly (or Aurora)
2. Open devtools > Memory
3. Firefox window width to narrow


Actual results:

Column width is too narrow.


Expected results:

Should be set enough minimum column width.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: bug1224201.wip.diff

First attempt at displaying more in the memory table :
- set all columns width to 10% except for the last one (flex: 1)
- reduced paddings
- removed ellipsis 

Ideally, the "number" should overflow to the left instead of right, so that all numbers remain aligned to the right and can easily be compared when reading a column.

I also changed the font to the monospace font, again so that numbers are perfectly aligned in a given column.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: bug1224201.wip.screenshot.png

Screenshot of the memory table with the patch previously uploaded.

Comment 5

[magicp]

(In reply to Julian Descottes from comment #4)
> Created attachment 8708118 [details]
> bug1224201.wip.screenshot.png
> 
> Screenshot of the memory table with the patch previously uploaded.

It seems like that column width and header width do not fit.

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to magicp from comment #5)
> (In reply to Julian Descottes from comment #4)
> 
> It seems like that column width and header width do not fit.

Happens when there is a scrollbar in the memory table body. The screenshot doesn't actually show it, but there was one :). 
This issue is tracked by Bug 1222403.

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8708117 [details] [diff] [review]
bug1224201.wip.diff

I think it looks good, but passing to VP for better feedback!

Comment 8

[Victor Porof [:vporof][:vp]]

Comment on attachment 8708117 [details] [diff] [review]
bug1224201.wip.diff

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

nice

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: bug1224201.v1.diff

Thanks for the feedback! 
(same diff, just added a proper commit message)

Comment 10

[Victor Porof [:vporof][:vp]]

Comment on attachment 8708812 [details] [diff] [review]
bug1224201.v1.diff

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

Feel free to r+ such minor patch increments in the future ;)

Comment 11

[Julian Descottes [:jdescottes]]

Thanks :)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3f0c3481ec93

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f0c3481ec93

Comment 14

[magicp]

Attachment: display-resolution-800x600.png

Can I confirm display resolution supported for memory tool(devtools)? If it is 800x600 (or 1024x768) and above, still column width is not sufficiently.

Comment 15

[magicp]

Attachment: display-resolution-1024x768.png

Comment 16

[Julian Descottes [:jdescottes]]

@magicp : I don't know if we have a minimum width for the devtools, but I think they should display nicely with a 800px width anyway.

I didn't mean to have this closed after my first patch though it was just a first improvement.

Let's reopen this bug, with the target of making sure we can see at least 6 digits in each column when the viewport is 800px wide.

Comment 17

[Julian Descottes [:jdescottes]]

Attachment: screenshot 800px with min-width and 90% font-size

I propose to : 
- reduce the font size to 90% for the table content
- add a min-width of 80px for the first columns

See attachment for a preview.

Comment 18

[Julian Descottes [:jdescottes]]

Attachment: bug1224201.part2.v1.patch (min-width for non-flex columns and reduced font size)

Bug 1224201 - memory table style : add min-width and reduce font-size

Goal of this commit is to ensure we can display at least 6 digits in the memory table
columns whatever the viewport size.

Some items of the memory table are already using 90% font size.
This commit applies this font-size to the whole memory table.
Also added a 80px min width to the non-flex columns.

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Shouldn't the minimum width be based on how many characters the cell must be able to contain? The % sub-cells should have minimum width to hold four characters at most: "100%"[0]. The non-% sub-cells are constrained by the maximum 64bit integer in theory, but in practice it should be safe to cap it at the tens of gigabytes. When including the spaces every 3 digits, this gives us 14 characters: "10 000 000 000".

Now, my understanding is that we have a CSS length unit *exactly* for width of a character in the current font: the "ch" unit!

So I think we can do this:

  .heap-tree-number {
    min-width: 14ch;
  }

  .heap-tree-percent {
    min-width: 4ch;
  }

And leave the .heap-tree-item-blah's 10% width suggestion as it is now, but allow it to be pushed wider by the above rules for the sub-cells.

Any reason we shouldn't do this? Am I misunderstanding the "ch" unit? Missing something else?

[0] Note that when diffing we may have a "+" or "-" in front of all of these numbers, and additionally percents are no longer capped at 100%. We may want to add another CSS class to the tree-table container when diffing and bump all of our min-widths by 2 or so characters when that parent class matches.

Comment 20

[Julian Descottes [:jdescottes]]

I didn't know about the ch unit :) 

> allow it to be pushed wider by the above rules for the sub-cells.

One issue is for sizing the column headers : their content is not made of heap-tree-number/percent.
Since `all` our columns display `bytes + percentage`, we could set a min-width of 16ch for all heap-tree-item-blah's.

We just need to make sure that the remaining column is still visible and usable : 
16ch makes each column ~100px wide, sidebar is 185px wide. In census view that means we have almost 600px already consumed before the "Name" column. For a 800px viewport, the Name column will be 200px, which is not huge but still ok.

> We may want to add another CSS class to the tree-table container when diffing

Yes we need a special rule for the diffing scenario, especially to override the width: 2.5em; rule we have on .percent items.

Comment 21

[magicp]

Do we need the "Bytes" and the "Count" column? I think their values are covered by the "Total..." columns. If we don't need them, the "Name" column will have sufficient width.

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to magicp from comment #21)
> Do we need the "Bytes" and the "Count" column?

Yes, we do. Although totals do include these measures, forcing the user to do arithmetic in his/her head to figure out the self values is much worse usability than a dense UI.

(In reply to Julian Descottes [:jdescottes] from comment #20)
> I didn't know about the ch unit :)

Me neither until yesterday :)

> 
> > allow it to be pushed wider by the above rules for the sub-cells.
> 
> One issue is for sizing the column headers : their content is not made of
> heap-tree-number/percent.
> Since `all` our columns display `bytes + percentage`, we could set a
> min-width of 16ch for all heap-tree-item-blah's.

Works for me.

> We just need to make sure that the remaining column is still visible and
> usable : 
> 16ch makes each column ~100px wide, sidebar is 185px wide. In census view
> that means we have almost 600px already consumed before the "Name" column.
> For a 800px viewport, the Name column will be 200px, which is not huge but
> still ok.

I think this is the lesser of two evils; we can also continue with the font-size: 90% thing.

> > We may want to add another CSS class to the tree-table container when diffing
> 
> Yes we need a special rule for the diffing scenario, especially to override
> the width: 2.5em; rule we have on .percent items.

Yes, (nitpicking: although it won't be 2.5em anymore but instead 4ch).

Comment 23

[Julian Descottes [:jdescottes]]

Attachment: bug1224201.part2.v2.patch

Bug 1224201 - mem. profiler : reduce font-size & add column min-width (16ch);r=vporof

Goal of this commit is to ensure we can display at least 12 number digits +
4 percent chars in the memory table columns whatever the viewport size.
With a viewport width of 800px, we still have 200px usable for the last col
in census view

Some items of the memory table are already using 90% font size.
This commit applies this font-size to the whole memory table.
Also added a 16ch min-width to the non-flex columns

Comment 24

[Victor Porof [:vporof][:vp]]

Comment on attachment 8712058 [details] [diff] [review]
bug1224201.part2.v2.patch

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

loving the ch units

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8712058 [details] [diff] [review]
bug1224201.part2.v2.patch

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

::: devtools/client/themes/memory.css
@@ +364,5 @@
>  .heap-tree-item-bytes,
>  .heap-tree-item-total-bytes {
>    width: 10%;
> +  /*
> +   * provision for 12chars for number part + 4chars for percent part

Nit: Please use capital letters at the start of the sentence and punctuation at the end.

Also, I think it would be good to explain the magic numbers 12 and 4:

"""
Provision for 12 chars for the number part (10s of gigabytes and the spaces every three digits) and 4 chars for the percent part (the maximum length string is "100%").
"""

Comment 26

[Julian Descottes [:jdescottes]]

Attachment: bug1224201.part2.v3.diff

Thanks for the review!

Comment 27

[Julian Descottes [:jdescottes]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd635665b134

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/70c749999c5a

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70c749999c5a

Comment 30

[Md. Rahimul Islam]

I have successfully reproduce this bug on firefox nightly 45.0a1 (2015-11-12)
with windows 7(32 bit)
Mozilla/5.0 (Windows NT 6.1; rv:45.0) Gecko/20100101 Firefox/45.0

I found this fix on latest beta  46.0b2

Mozilla/5.0 (Windows NT 6.1; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID : 20160316065941

Comment 31

[Bolaram Paul [:ancon]]

The bug is now verified on Latest Beta 46.0b2!

Build ID: 20160316065941
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0

Comment 32

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test Successful.

Component: 
Name 			Firefox
Version 		46.0b9
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
Cells are of suitable size of that of devtools of screen size.

Actual Results: 
As expected