1379122 - UI density popup is illegible

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Dão Gottwald [:dao]]

Attachment: screenshot

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1379122 - Fix UI density menuitem font color on Linux.

- Fixes illegible font color on Linux
- Fixes too large font size on Mac (compared to the theme menu)

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

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8886142 [details]
Bug 1379122 - Fix UI density menuitem font color on Linux.

https://reviewboard.mozilla.org/r/156944/#review162034

::: browser/themes/shared/customizableui/customizeMode.inc.css:479
(Diff revision 1)
>    padding-bottom: 0;
>    padding-inline-start: 0;
>  }
>  
> +.customization-uidensity-menu-button {
> +  font: message-box;

Shouldn't this be font: menu?

::: browser/themes/shared/customizableui/customizeMode.inc.css:480
(Diff revision 1)
>    padding-inline-start: 0;
>  }
>  
> +.customization-uidensity-menu-button {
> +  font: message-box;
> +  color: inherit;

Where did the gray color come from in the first place?

Comment 3

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #2)
> Comment on attachment 8886142 [details]
> Bug 1379122 - Fix UI density menuitem font in customize mode.
> 
> https://reviewboard.mozilla.org/r/156944/#review162034
> 
> ::: browser/themes/shared/customizableui/customizeMode.inc.css:479
> (Diff revision 1)
> >    padding-bottom: 0;
> >    padding-inline-start: 0;
> >  }
> >  
> > +.customization-uidensity-menu-button {
> > +  font: message-box;
> 
> Shouldn't this be font: menu?

It's font: message-box for the theme menu. My thought was that we want to style those items like toolbarbuttons even though they're technically menuitems (to take advantage of the acceltext for indicating it's overriden). font: menu is a bit too large.
> ::: browser/themes/shared/customizableui/customizeMode.inc.css:480
> (Diff revision 1)
> >    padding-inline-start: 0;
> >  }
> >  
> > +.customization-uidensity-menu-button {
> > +  font: message-box;
> > +  color: inherit;
> 
> Where did the gray color come from in the first place?

Again, from being a menuitem. :)

Comment 4

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #3)
> > > +.customization-uidensity-menu-button {
> > > +  font: message-box;
> > 
> > Shouldn't this be font: menu?
> 
> It's font: message-box for the theme menu. My thought was that we want to
> style those items like toolbarbuttons even though they're technically
> menuitems (to take advantage of the acceltext for indicating it's
> overriden). font: menu is a bit too large.

This sounds backwards. The popup is a menu for all intents and purposes. That some of these popups use toolbarbuttons is an implementation detail.

Comment 5

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #4)
> (In reply to Johann Hofmann [:johannh] from comment #3)
> > > > +.customization-uidensity-menu-button {
> > > > +  font: message-box;
> > > 
> > > Shouldn't this be font: menu?
> > 
> > It's font: message-box for the theme menu. My thought was that we want to
> > style those items like toolbarbuttons even though they're technically
> > menuitems (to take advantage of the acceltext for indicating it's
> > overriden). font: menu is a bit too large.
> 
> This sounds backwards. The popup is a menu for all intents and purposes.
> That some of these popups use toolbarbuttons is an implementation detail.

In any case, the theme menu is using font: message-box. I'd like it to be consistent.

Comment 6

[Dão Gottwald [:dao]]

So the theme menu should probably use font: menu.

Comment 7

[Johann Hofmann [:johannh]]

But font: menu is much larger, which is totally inconsistent with the rest of the customize mode on Mac and IMO looks much worse. I don't think that's warranted.

If you want to change it to font: menu, please advise me on how you're planning on keeping the font-size consistent, cross-platform.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #7)
> But font: menu is much larger, which is totally inconsistent with the rest
> of the customize mode on Mac and IMO looks much worse. I don't think that's
> warranted.

I can't really argue with "looks much worse" (haven't even seen it myself). Why do you think the popup's font size is supposed to be the same as is non-popup content? panelUI.inc.css uses font: menu too for various popups.

> If you want to change it to font: menu, please advise me on how you're
> planning on keeping the font-size consistent, cross-platform.

Not sure what you mean. Fonts aren't necessarily consistent in different UI elements, nor across platforms. But maybe try font: -moz-pull-down-menu?

Comment 9

[Johann Hofmann [:johannh]]

Attachment: font-menu.png

Comment 10

[Johann Hofmann [:johannh]]

Attachment: font-message-box.png

Comment 11

[Johann Hofmann [:johannh]]

The font-size is much larger with font: menu (and even larger with font: -moz-pull-down-menu). As a Mac user I would consider this a regression, do you agree? It doesn't really matter if the font size is supposed to be the same as non-popup content or not, coincidentally it is in this case and breaking that would be a visual regression.

If there's no way we can resolve this I'll take this part out of my patch.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #11)
> The font-size is much larger with font: menu (and even larger with font:
> -moz-pull-down-menu). As a Mac user I would consider this a regression, do
> you agree?

Not necessarily, no. It really depends on platform conventions. The fact that font: menu and -moz-pull-down-menu are larger suggests that this is the way it's supposed to be on Mac. In a way this makes sense too: menus are mostly text based and transient, so there's a different trade-off of usability vs. saving screen real estate. It's okay for them to temporarily consume more screen space than non-transient UI does.

> It doesn't really matter if the font size is supposed to be the
> same as non-popup content or not, coincidentally it is in this case and
> breaking that would be a visual regression.

Of course it matters how UIs like this one are supposed to look and feel on Mac. In fact, that's really all that matters here.

Comment 13

[Johann Hofmann [:johannh]]

Attachment: font-message-box.png

Accidentally made a screenshot with the menu font twice. This is message-box now.

Comment 14

[Johann Hofmann [:johannh]]

Ok, I'm going to spin this off in its own bug and needinfo shorlander.

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8886142 [details]
Bug 1379122 - Fix UI density menuitem font color on Linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156944/diff/1-2/

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8886142 [details]
Bug 1379122 - Fix UI density menuitem font color on Linux.

https://reviewboard.mozilla.org/r/156944/#review162474

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/884c8089012e
Fix UI density menuitem font color on Linux. r=dao

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/884c8089012e

Comment 19

[Deac Alin]

Hi Dão Gottwald,
Can you please provide Expected behaviour for this issue? What exactly needs to be Tested(QA)? The density popup menu size and color on Linux to be resized and to be consistent with Mac and Windows? Thank you

Comment 20

[Dão Gottwald [:dao]]

This bug is about the text color used in the menu, see attachment 8884253 [details]

Comment 21

[Deac Alin]

Attachment: Screen Shot 2017-08-22 at 17.19.03.png

Tested on Mac OSX 10.12.5 and Ubuntu 16.04 x 64 on latest Nightly and it can be observed that there is a color inconsistency (darker on Mac) between Mac and Linux for the mentioned text in the density popup Menu(check the attached screenshot). If this is intended, i will mark this bug as Verified Fixed. Thank you

Comment 22

[Dão Gottwald [:dao]]

(In reply to Deac Alin from comment #21)
> Created attachment 8899850 [details]
> Screen Shot 2017-08-22 at 17.19.03.png
> 
> Tested on Mac OSX 10.12.5 and Ubuntu 16.04 x 64 on latest Nightly and it can
> be observed that there is a color inconsistency (darker on Mac) between Mac
> and Linux for the mentioned text in the density popup Menu(check the
> attached screenshot). If this is intended, i will mark this bug as Verified
> Fixed. Thank you

That text on Ubuntu 16.04 looks too faint. That's not what I see on Ubuntu 17.04 though.

Comment 23

[Deac Alin]

Attachment: Screen Shot 2017-08-22 at 18.08.51.png

(In reply to Dão Gottwald [::dao] from comment #22)
> That text on Ubuntu 16.04 looks too faint. That's not what I see on Ubuntu
> 17.04 though.

Yes you are right. The text is too faint on 16.04. I attached a new screenshot with the text color from 16.04 VS 17.04. Please advise on the next course of action regarding this bug.

Comment 24

[Dão Gottwald [:dao]]

Please file a new bug on the 16.04 issue.

Comment 25

[Deac Alin]

Logged a new bug for the above mentioned issue for Linux 16.04 - 1392942 
Verified Fixed for Mac 10.12.5, Windows 10 x 64 and Ubuntu 17.04. Thank you