Closed Bug 1144034 Opened 9 years ago Closed 9 years ago

Flamegraph text is barely readable on non-retina display

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P1)

defect

Tracking

(firefox40 verified, firefox41 fixed)

VERIFIED FIXED
Firefox 41
Tracking Status
firefox40 --- verified
firefox41 --- fixed

People

(Reporter: paul, Assigned: vporof)

References

Details

(Whiteboard: [polish-backlog])

Attachments

(3 files, 1 obsolete file)

(tested with Windows)
A screenshot would really help.
Attached image Screenshot
Yeah, this looks pretty terrible on Windows.
I think the theme switching support regressed this.

It should use the message-box font.
OS: Mac OS X → All
Hardware: x86 → All
My eyes are literally dying. </insidejoke>
Blocks: perf-tool-papercuts
No longer blocks: perf-tool-v2
Attached image Screenshot on OS X
Also true on OS X, copying my screenshot form the other bug.
With the landing of bug 1121180, which increased the font size, block height, and changed the background colors, is this looking better? In m-c, or probably tomorrow's nightly.
Looking at the screenshots again I think the DPI used is backwards,I have half the amount of blocks vertically on my retina.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P1
Victor, marking this for devedition 40. It's quite painful on non-retina screens, where is this in your queue?
Flags: needinfo?(vporof)
Whiteboard: [devedition-40]
Fairly high, P1 and I'm assigned.
Flags: needinfo?(vporof)
Attached patch flame-font-non-retina.patch (obsolete) — Splinter Review
Attachment #8604693 - Flags: review?(jsantell)
Comment on attachment 8604693 [details] [diff] [review]
flame-font-non-retina.patch

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

::: browser/devtools/shared/widgets/FlameGraph.js
@@ +42,3 @@
>  const FLAME_GRAPH_BLOCK_BORDER = 1; // px
> +const FLAME_GRAPH_BLOCK_TEXT_FONT_SIZE = 10; // px
> +const FLAME_GRAPH_BLOCK_TEXT_FONT_FAMILY = "Helvetica Neue, Helvetica, sans-serif";

Windows ? :(
Helvetica exists on windows by default no?
Probably safe to add Arial there though.
(In reply to Victor Porof [:vporof][:vp] from comment #15)
> Probably safe to add Arial there though.

The font: message-box shorthand works fine for all platforms.
You could fallback to Segoe UI otherwise.
(In reply to Tim Nguyen [:ntim] (limited availability) from comment #16)
> 
> The font: message-box shorthand works fine for all platforms.

Does it look good though?
Comment on attachment 8604693 [details] [diff] [review]
flame-font-non-retina.patch

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

This will most def require updates to tests.

On my I M A C the blocks are more easy to read, but still quite hard just because of the kerning. Maybe there's some other solution where we change the size based on res, and still let the user change the block size, because this one will be tricky to have look good in all scenarios, I think. Or we can go even bigger, because the user can always scroll up/down on the flames, but they can't increase the "zoom" [currently anyway].

With the brightness/opacity changes (esp in dark theme), the blocks are too dark/hard to distinguish from differences in colors between each other -- would prefer the older brightness/opacity, as I don't think that this improves readability (I think the contrast is more stark now too, making it a bit hard to read)

With tests passing, a revert to the opacity/brightness unless there's strong feelings otherwise, and see if making the blocks even bigger helps, like 1.25x what is here, r+.

We should try and get f+ from someone on linux/windows.
Attachment #8604693 - Flags: review?(jsantell) → review+
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #18)
> Comment on attachment 8604693 [details] [diff] [review]
> 
> With the brightness/opacity changes (esp in dark theme), the blocks are too
> dark/hard to distinguish from differences in colors between each other --
> would prefer the older brightness/opacity, as I don't think that this
> improves readability (I think the contrast is more stark now too, making it
> a bit hard to read)

The old colors were gnarly to my eyes. Meet you in the middle?
Ya sounds good. Mainly want each block not blending into its neighbor, text legible and not searing sharp (like #fff text on #000). The light colors looked good before but yeah sometimes dark theme got a little too bold..
Attached patch v2Splinter Review
Fixed tests, addressed comments.
Attachment #8604693 - Attachment is obsolete: true
Attachment #8607647 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2041288ab441
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Flags: qe-verify+
Comment on attachment 8607647 [details] [diff] [review]
v2


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None
Attachment #8607647 - Flags: approval-mozilla-aurora?
Note: I had verbal confirmation for these uplifts from Sylvestre even before he's flagged them as a+.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1167252#c26
Comment on attachment 8607647 [details] [diff] [review]
v2

Change approved to skip one train as part of the spring campaign.
Attachment #8607647 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on Aurora 40.0a2 (2015-06-09) using Windows 7 (x64), Ubuntu 13.10 (x64) and Mac OS X 10.9.5, each environment with non-hidpi displays. 

The data available in the "Flame Chart" detail view is now reasonably displayed on non-hidpi displays as well. Also (re)confirmed that the Flame Chart is still displayed properly on a MacBook with Retina (OS X 10.9.5).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [devedition-40] → [polish-backlog]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.