Closed
Bug 1224201
Opened 8 years ago
Closed 8 years ago
Cells in the memory tool's heap view need more minimum width
Categories
(DevTools :: Memory, defect, P2)
DevTools
Memory
Tracking
(firefox44 affected, firefox45 affected, firefox46 fixed, firefox47 fixed)
RESOLVED
FIXED
Firefox 46
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
status-firefox44:
--- → affected
status-firefox45:
--- → affected
Component: Untriaged → Developer Tools: Memory
OS: Unspecified → All
Hardware: Unspecified → All
Updated•8 years ago
|
Blocks: memory-frontend
Updated•8 years ago
|
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
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
(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•8 years ago
|
||
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 8•8 years ago
|
||
Comment on attachment 8708117 [details] [diff] [review] bug1224201.wip.diff Review of attachment 8708117 [details] [diff] [review]: ----------------------------------------------------------------- nice
Attachment #8708117 -
Flags: feedback?(vporof) → feedback+
Assignee | ||
Comment 9•8 years ago
|
||
Thanks for the feedback! (same diff, just added a proper commit message)
Attachment #8708117 -
Attachment is obsolete: true
Attachment #8708812 -
Flags: review?(vporof)
Comment 10•8 years ago
|
||
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+
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3f0c3481ec93
Keywords: checkin-needed
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3f0c3481ec93
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Reporter | ||
Comment 14•8 years ago
|
||
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)
Reporter | ||
Comment 15•8 years ago
|
||
Assignee | ||
Comment 16•8 years ago
|
||
@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 → ---
Assignee | ||
Comment 17•8 years ago
|
||
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.
Assignee | ||
Comment 18•8 years ago
|
||
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
Comment 19•8 years ago
|
||
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.
Assignee | ||
Comment 20•8 years ago
|
||
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.
Reporter | ||
Comment 21•8 years ago
|
||
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•8 years ago
|
||
(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).
Assignee | ||
Comment 23•8 years ago
|
||
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 24•8 years ago
|
||
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 25•8 years ago
|
||
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%"). """
Assignee | ||
Comment 26•8 years ago
|
||
Thanks for the review!
Attachment #8712058 -
Attachment is obsolete: true
Attachment #8712419 -
Flags: review+
Assignee | ||
Comment 27•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd635665b134
Keywords: checkin-needed
Comment 28•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/70c749999c5a
Keywords: checkin-needed
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/70c749999c5a
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Comment 30•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
[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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•