UI density popup is illegible

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
2 months ago
a day ago

People

(Reporter: dao, Assigned: johannh)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 56
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed, firefox57 verified)

Details

(Whiteboard: [photon-visual][p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

2 months ago
Created attachment 8884253 [details]
screenshot

Updated

a month ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: brindusa.tot
Whiteboard: [photon-visual][triage] → [photon-visual]
(Reporter)

Updated

a month ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [photon-visual] → [photon-visual][p1]

Updated

a month ago
Iteration: --- → 56.3 - Jul 24
Comment hidden (mozreview-request)
(Reporter)

Comment 2

a month ago
mozreview-review
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?
(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. :)
(Reporter)

Comment 4

a month ago
(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 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.
(Reporter)

Comment 6

a month ago
So the theme menu should probably use font: menu.
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.
(Reporter)

Comment 8

a month ago
(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?
Created attachment 8886509 [details]
font-menu.png
Created attachment 8886510 [details]
font-message-box.png
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.
(Reporter)

Comment 12

a month ago
(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.
Created attachment 8886528 [details]
font-message-box.png

Accidentally made a screenshot with the menu font twice. This is message-box now.
Attachment #8886510 - Attachment is obsolete: true
Ok, I'm going to spin this off in its own bug and needinfo shorlander.
Comment hidden (mozreview-request)
(Reporter)

Comment 16

a month ago
mozreview-review
Comment on attachment 8886142 [details]
Bug 1379122 - Fix UI density menuitem font color on Linux.

https://reviewboard.mozilla.org/r/156944/#review162474
Attachment #8886142 - Flags: review?(dao+bmo) → review+
See Also: → bug 1380955

Comment 17

a month ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/884c8089012e
Fix UI density menuitem font color on Linux. r=dao

Comment 18

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/884c8089012e
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
QA Contact: brindusa.tot → ovidiu.boca

Comment 19

2 days ago
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
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 20

2 days ago
This bug is about the text color used in the menu, see attachment 8884253 [details]
Flags: needinfo?(dao+bmo)
(Reporter)

Updated

2 days ago
Attachment #8886509 - Attachment is obsolete: true
(Reporter)

Updated

2 days ago
Attachment #8886528 - Attachment is obsolete: true

Comment 21

2 days ago
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
(Reporter)

Comment 22

2 days ago
(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

2 days ago
Created attachment 8899880 [details]
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.
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 24

2 days ago
Please file a new bug on the 16.04 issue.
Flags: needinfo?(dao+bmo)

Updated

a day ago
Depends on: 1392942

Comment 25

a day ago
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
Status: RESOLVED → VERIFIED
status-firefox57: --- → verified
No longer depends on: 1392942
Flags: qe-verify+

Updated

a day ago
Depends on: 1392942
You need to log in before you can comment on or make changes to this bug.