Closed Bug 1224201 Opened 9 years ago Closed 8 years ago

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

Categories

(DevTools :: Memory, defect, P2)

defect

Tracking

(firefox44 affected, firefox45 affected, firefox46 fixed, firefox47 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox44 --- affected
firefox45 --- affected
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: magicp.jp, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 4 obsolete files)

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.
Has STR: --- → yes
Component: Untriaged → Developer Tools: Memory
OS: Unspecified → All
Hardware: Unspecified → All
Priority: -- → P2
Summary: [devtools] Should be set enough minimum column width in Memory tool → Cells in the memory tool's heap view need more minimum width
Attached patch bug1224201.wip.diff (obsolete) — Splinter Review
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.
Assignee: nobody → jdescottes
Attachment #8708117 - Flags: feedback?(jsantell)
Screenshot of the memory table with the patch previously uploaded.
(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.
(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 on attachment 8708117 [details] [diff] [review]
bug1224201.wip.diff

I think it looks good, but passing to VP for better feedback!
Attachment #8708117 - Flags: feedback?(jsantell) → feedback?(vporof)
Comment on attachment 8708117 [details] [diff] [review]
bug1224201.wip.diff

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

nice
Attachment #8708117 - Flags: feedback?(vporof) → feedback+
Attached patch bug1224201.v1.diff (obsolete) — Splinter Review
Thanks for the feedback! 
(same diff, just added a proper commit message)
Attachment #8708117 - Attachment is obsolete: true
Attachment #8708812 - Flags: review?(vporof)
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 ;)
Attachment #8708812 - Flags: review?(vporof) → review+
Thanks :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3f0c3481ec93
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Can I confirm display resolution supported for memory tool(devtools)? If it is 800x600 (or 1024x768) and above, still column width is not sufficiently.
Flags: needinfo?(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.
Status: RESOLVED → REOPENED
Flags: needinfo?(jdescottes)
Resolution: FIXED → ---
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.
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.
Attachment #8708812 - Attachment is obsolete: true
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.
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.
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.
(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).
Attached patch bug1224201.part2.v2.patch (obsolete) — Splinter Review
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
Attachment #8711479 - Attachment is obsolete: true
Attachment #8712058 - Flags: review?(vporof)
Comment on attachment 8712058 [details] [diff] [review]
bug1224201.part2.v2.patch

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

loving the ch units
Attachment #8712058 - Flags: review?(vporof) → review+
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%").
"""
Thanks for the review!
Attachment #8712058 - Attachment is obsolete: true
Attachment #8712419 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/70c749999c5a
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
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
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
[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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.