1399747 - The font-size from Theme and UI Density in not consistent (Mac OS)

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[ovidiu boca[:Ovidiu]]

Attachment: the font size.png

This is a follow up from https://bugzilla.mozilla.org/show_bug.cgi?id=1380955#c14

[Affected versions]: Nightly 57.0a1(2017-09-13)

[Affected platforms]: Tested on Mac OS 

[Steps to reproduce]:

1. Go to Customize
2. Compare the font-size from Themes and from Density

[Expected result]:

The font-size should be the same. 

[Actual result]:

The font size is not the same, for example, the "N" from Normal has 11px and the "D" from Default has 10px have different px size. See the attached print screen.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1399747 - Density panel menuitems should use `font: menu` to be consistent with other panels.

Review commit: https://reviewboard.mozilla.org/r/182006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182006/

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

There's already a rule[1] that was likely intended for this, but it's applied to the panel and intended to be inherited by its children. The problem is that menuitem elements have `font: -moz-pull-down-menu;` set on them[2], which overrules [1].

I'd be interested to investigate whether we can remove that rule from menu.css (do we use any menuitem elements that rely on that rule?) but that's out of the scope of this bug.

Johann, I want to keep the density menu selector for [1] for correctness even though it's currently not being used, let me know what you think.

[1] https://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/browser/themes/shared/customizableui/customizeMode.inc.css#303
[2] https://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/toolkit/themes/osx/global/menu.css#13

Comment 3

[Johann Hofmann [:johannh]]

Comment on attachment 8910562 [details]
Bug 1399747 - Density panel menuitems should use `font: menu` to be consistent with other panels.

https://reviewboard.mozilla.org/r/182006/#review187484

Hm I think I'd be fine with either removing or leaving the unused rule. If you prefer to leave it, that's ok.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6a2ee5c3532
Density panel menuitems should use `font: menu` to be consistent with other panels. r=johannh

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e6a2ee5c3532

Comment 6

[Hani Yacoub, Desktop QA]

Build ID: 20170924220116
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:58.0) Gecko/20100101 Firefox/58.0

Verified as fixed on Firefox Nightly 58.0a1 on Mac OS X 10.12.

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8910562 [details]
Bug 1399747 - Density panel menuitems should use `font: menu` to be consistent with other panels.

Approval Request Comment
[Feature/Bug causing the regression]: photon-visual polish
[User impact if declined]: inconsistent font size in two different menus in customize mode
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: minor visual change
[String changes made/needed]:

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910562 [details]
Bug 1399747 - Density panel menuitems should use `font: menu` to be consistent with other panels.

Polish photon, taking it.
Should be in 57b4. GTB later today.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a99eb3b7ac5a

Comment 10

[Hani Yacoub, Desktop QA]

Build ID: 20170928180207
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0

Verified as fixed on Firefox Beta 57.0b5 on Mac OS X 10.12.