[Control Center] Icons don't align with section headlines

VERIFIED FIXED in Firefox 42

Status

()

defect
P1
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: bgrins)

Tracking

Trunk
Firefox 43
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox42 verified, firefox43 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(4 attachments)

Ash sent me a few screenshots a week or two ago that show that the top line of the headline is not aligned with the top border of the icon to the left. I tried fixing that but wasn't very successful, there seems to be a "padding" that the font rendering creates and as that's flexible depending on the font and size I wouldn't know how to fix this properly.
Flags: firefox-backlog?
Blocks: 1188565
Flags: firefox-backlog? → firefox-backlog+
Priority: -- → P2
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Iteration: --- → 43.1 - Aug 24
Flags: qe-verify?
Priority: P2 → P1
Ash, can you attach the screenshots showing this misalignment in the Control Center?
Flags: needinfo?(agrigas)
Posted image current.png
I think I figured this out.  It looks like the logo is currently just a *touch* too high (as you can see in this screenshot where i've replaced the background image of the section with gradients and drawn a line across the TP section
Flags: needinfo?(agrigas)
Posted image new.png
Same thing, but with my patch applied
I realized that my patch makes the header container top-aligned with the image, but the text still doesn't have a sharp top alignment with the image.  Is this what we want? (see attached screenshot)

Note that if we want the top of the text to align with the top of the image, it won't always happen in the case where all of the text is lower cased (like about:preferences)
Flags: needinfo?(agrigas)
I guess qe-verify+ but it's going to be very subtle.
Flags: qe-verify? → qe-verify+
(In reply to Brian Grinstead [:bgrins] from comment #4)
> Created attachment 8646625 [details]
> new-no-background-color.png
> 
> I realized that my patch makes the header container top-aligned with the
> image, but the text still doesn't have a sharp top alignment with the image.
> Is this what we want? (see attached screenshot)
> 
> Note that if we want the top of the text to align with the top of the image,
> it won't always happen in the case where all of the text is lower cased
> (like about:preferences)

Can we at least have the ascender top aline (the top of the letter). Its ok if that breaks when its all lower-case or has no ascenders.

https://www.dropbox.com/s/fr3vh38zzpuhjgc/Screenshot%202015-08-11%2015.11.21.png?dl=0
Flags: needinfo?(agrigas)
QA Contact: mwobensmith
Looked further into it, it appears that there is some line-height that is set when <label value="foo" /> is used that isn't accessible via CSS on the label selector.  But if you do <label>foo</label> you can set line-height: 1 which aligns it properly.  Unfortunately I'm not sure if it'll be practical to switch to this, since setting it to use textContent doesn't seem to crop long strings correctly.
So I spent some time debugging inside nsTextBoxFrame (which is the frame used when setting value="" as opposed to textContent).  As best as I can tell, I think GetTextSize [0] is somehow not taking into account the line height of the element.

I'm going to upload a patch that makes things better across systems by just tweaking the existing padding / margin, but I think it's not going to be possible to get this pixel-perfect across all default fonts unless if we come up with a fix in the XUL layout.  I don't think it's worth spending more time on this, though.

[0]: https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsTextBoxFrame.cpp?#1017
With the default fonts on my test systems, Linux seems to have the smallest gap (align the text higher), OSX in the middle, and Windows has the biggest gap (aligns the text lower)
Bug 1192214 - Use matching alignment for section headers and icons in control center;r=ttaubert
Attachment #8647263 - Flags: review?(ttaubert)
(In reply to Brian Grinstead [:bgrins] from comment #7)
> Looked further into it, it appears that there is some line-height that is
> set when <label value="foo" /> is used that isn't accessible via CSS on the
> label selector.  But if you do <label>foo</label> you can set line-height: 1
> which aligns it properly.  Unfortunately I'm not sure if it'll be practical
> to switch to this, since setting it to use textContent doesn't seem to crop
> long strings correctly.

That's a great find, it seems like that looks a lot better than what we have now but we can't crop strings properly. I hate XUL, especially in combination with text and overflow :(
Comment on attachment 8647263 [details]
MozReview Request: Bug 1192214 - Use matching alignment for section headers and icons in control center;r=ttaubert

https://reviewboard.mozilla.org/r/15811/#review14277

Seems reasonable. I don't think we can achieve anything better with XUL so we have to optimize for the default case.
Attachment #8647263 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/mozilla-central/rev/17cd8b68c37c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
(In reply to Brian Grinstead [:bgrins] from comment #9)
> With the default fonts on my test systems, Linux seems to have the smallest
> gap (align the text higher), OSX in the middle, and Windows has the biggest
> gap (aligns the text lower)
Verified fixed 43.0a1 (2015-08-24) Win7, OS X 10.10, Ubuntu 14.04
Status: RESOLVED → VERIFIED
Comment on attachment 8647263 [details]
MozReview Request: Bug 1192214 - Use matching alignment for section headers and icons in control center;r=ttaubert

Approval Request Comment
[Feature/regressing bug #]: 1170759
[User impact if declined]: Minor visual alignment issue between icon and section headlines in Control Center
[Describe test coverage new/current, TreeHerder]: No tests, changing some CSS positioning
[Risks and why]: Not much, but it could cause some different misalignment depending on default system fonts
[String/UUID change made/needed]:
Attachment #8647263 - Flags: approval-mozilla-aurora?
Comment on attachment 8647263 [details]
MozReview Request: Bug 1192214 - Use matching alignment for section headers and icons in control center;r=ttaubert

Polish, taking it.
Attachment #8647263 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed 42.0a2 (2015-09-09) Win7
You need to log in before you can comment on or make changes to this bug.