Flamegraph text is barely readable on non-retina display

VERIFIED FIXED in Firefox 40

Status

P1
normal
VERIFIED FIXED
4 years ago
2 months ago

People

(Reporter: paul, Assigned: vporof)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 41
Dependency tree / graph

Firefox Tracking Flags

(firefox40 verified, firefox41 fixed)

Details

(Whiteboard: [polish-backlog])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
(tested with Windows)
(Assignee)

Comment 1

4 years ago
A screenshot would really help.

Comment 2

4 years ago
Created attachment 8578797 [details]
Screenshot

Yeah, this looks pretty terrible on Windows.
I think the theme switching support regressed this.

It should use the message-box font.

Updated

4 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 3

4 years ago
My eyes are literally dying. </insidejoke>
(Assignee)

Updated

3 years ago
Blocks: 1145697
No longer blocks: 1075567
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1146661
Created attachment 8584123 [details]
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.
Comment hidden (typo)
Looking at the screenshots again I think the DPI used is backwards,I have half the amount of blocks vertically on my retina.
Comment hidden (typo)
(Assignee)

Updated

3 years ago
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]
(Assignee)

Comment 11

3 years ago
Fairly high, P1 and I'm assigned.
Flags: needinfo?(vporof)
(Assignee)

Comment 12

3 years ago
Created attachment 8604693 [details] [diff] [review]
flame-font-non-retina.patch
Attachment #8604693 - Flags: review?(jsantell)

Comment 13

3 years ago
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 ? :(
(Assignee)

Comment 14

3 years ago
Helvetica exists on windows by default no?
(Assignee)

Comment 15

3 years ago
Probably safe to add Arial there though.

Comment 16

3 years ago
(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.
(Assignee)

Comment 17

3 years ago
(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+
(Assignee)

Comment 19

3 years ago
(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..
(Assignee)

Comment 21

3 years ago
Created attachment 8607647 [details] [diff] [review]
v2

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
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Blocks: 1167252

Updated

3 years ago
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
status-firefox40: fixed → verified
Flags: qe-verify+
Whiteboard: [devedition-40] → [polish-backlog]

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.