Closed Bug 1176109 Opened 4 years ago Closed 4 years ago

Popup menu text is too dark on some GTK3 themes

Categories

(Core :: Widget: Gtk, defect)

Unspecified
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: acomminos, Assigned: acomminos)

References

Details

Attachments

(1 file, 3 obsolete files)

On some GTK3 themes (such as the version of Ambiance on Ubuntu 12.04), the text in popup menus is rendered too dark. This is because some themes rely on the menu bar's style to set the colour of popup menus' text.
This patch adds the menubar styling to the context, as is done by GTK when rendering popups.
Attachment #8624539 - Flags: review?(karlt)
Comment on attachment 8624539 [details] [diff] [review]
Fix popup text colour on some GTK3 themes.

GTK doesn't add GTK_STYLE_CLASS_MENUBAR to GtkMenu's, and popup menu's don't
have a menubar, so I don't think this is the right fix.

I suspect the parent style context might be involved.
That might come from gtk_window_set_attached_to(), but
gtk_style_context_set_parent() can be used without needing widgets.
Attachment #8624539 - Flags: review?(karlt) → review-
Thanks for taking a look.

It looks like this is a bit more complex; GTK themes can specify different theming for menus depending on whether or not a menu is attached to a menubar or primary toolbar (compare right-clicking a text field in gtk3-widget-factory vs. selecting a menu item on Ubuntu 12.04).

See this example in 12.04's Ambiance:

Right-click menus and menuitems:
> .menuitem {
>     color: @fg_color;
> }
> .menu {
>     background-image: none;
>     border-bottom-color: shade (@bg_color, 0.66);
>     border-left-color: shade (@bg_color, 0.7);
>     border-right-color: shade (@bg_color, 0.7);
>     border-top-color: shade (@bg_color, 0.8);
>     border-style: solid;
>     padding: 0;
> 
>     color: @fg_color;
> 
>     -unico-inner-stroke-gradient: none;
>     -unico-inner-stroke-width: 1px 1px 0 1px;
> }

Menubar-spawned menus and menuitems:
> Genericmenuitem .menu,
> DbusmenuGtkMenu .menu,
> .menubar .menu,
> .primary-toolbar .menu {
>     background-color: shade (@dark_bg_color, 1.08);
>     border-bottom-color: shade (@dark_bg_color, 0.96);
>     border-left-color: shade (@dark_bg_color, 0.8);
>     border-right-color: shade (@dark_bg_color, 0.8);
>     border-top-color: shade (@dark_bg_color, 0.96);
> 
>     color: @dark_fg_color;
> 
>     -unico-inner-stroke-color: shade (@dark_bg_color, 1.18);
> }
> Genericmenuitem .menuitem,
> DbusmenuGtkMenu .menuitem,
> .menubar .menuitem,
> .primary-toolbar .menuitem {
>     color: @dark_fg_color;
> }

...where dark_{fg,bg}_color and {fg,bg}_color are colors on dark and light backgrounds respectively.

I'm not sure if there's a trivial solution for this that will get us the right result for dropdowns. I'd imagine the right solution here would be styling popup menus based on whether or not they're attached to a menubar by revealing the menu's source as a different LookAndFeel colour constant. Alternatively, we could treat all popups identically, we would just have to be consistent- i.e., treat everything like it's attached to a menubar, as this patch does.
Flags: needinfo?(karlt)
(In reply to Andrew Comminos [:acomminos] from comment #3)

> > Genericmenuitem .menu,
> > DbusmenuGtkMenu .menu,
> > .menubar .menu,
> > .primary-toolbar .menu {

> I'm not sure if there's a trivial solution for this that will get us the
> right result for dropdowns. I'd imagine the right solution here would be
> styling popup menus based on whether or not they're attached to a menubar by
> revealing the menu's source as a different LookAndFeel colour constant.

Yes, something like eColorID_menubarmenutext, is probably required, but the
.primary-toolbar case implies we also need eColorID_toolbarmenutext, and maybe
others.

> Alternatively, we could treat all popups identically, we would just have to
> be consistent- i.e., treat everything like it's attached to a menubar, as
> this patch does.

Ensuring we have things right for at least one of the styles and using that
for all is a sensible intermediate solution.

However, I don't think there is enough evidence here to suggest that we should
switch a single style to menubar style, particularly if this is a Ubuntu-only
issue.  The menubar is hidden by default in firefox, and Unity users with the
Ubuntu's build will be using Unity's menubar.

The most visible dropdown in Firefox is the urlbar dropdown.
It is debatable whether menubar colors or textfield popup colors are more
appropriate there, but I expect the textfield popup colors are OK.

Most Firefox toolbar menus don't seem to use the menu colors.

If we do pick a parent widget then we should provide it style context as an
ancestor rather than adding a special style class to the accel label.

> Right-click menus and menuitems:
> > .menuitem {
> >     color: @fg_color;
> > }
> > .menu {
> >     background-image: none;
> >     border-bottom-color: shade (@bg_color, 0.66);
> >     border-left-color: shade (@bg_color, 0.7);
> >     border-right-color: shade (@bg_color, 0.7);
> >     border-top-color: shade (@bg_color, 0.8);
> >     border-style: solid;
> >     padding: 0;
> > 
> >     color: @fg_color;
> > 
> >     -unico-inner-stroke-gradient: none;
> >     -unico-inner-stroke-width: 1px 1px 0 1px;
> > }

I wonder where the background color comes from.
Perhaps a parent style context may be required, even with this style, so the
background color can be inherited.
Flags: needinfo?(karlt)
This is a trivial workaround that draws all menu popups like text field popups. While menu bar spawned popup menus won't inherit the bar's theming as with native GTK apps, at least the widget drawing will be consistent with our look and feel colours and be readable.
Attachment #8624539 - Attachment is obsolete: true
Attachment #8629490 - Flags: review?(karlt)
Comment on attachment 8629490 [details] [diff] [review]
Don't attach menu popups to menubars on GTK3.

I'm fine with this in principle but having no attached widget for gMenuPopupWidget will mean that the menu popup is not destroyed, and so this change would introduce a shutdown leak.

I expect it is safer to use gtk_menu_attach_to_widget, in case the theme is expecting a parent style context, than to leave it unattached and add explicit destroy.

gProtoWindow might be a safe generic widget.
Attachment #8629490 - Flags: review?(karlt) → review-
Thanks, updated to attach menu to gProtoWindow.
Attachment #8629490 - Attachment is obsolete: true
Attachment #8629950 - Flags: review?(karlt)
Comment on attachment 8629950 [details] [diff] [review]
Don't attach menu popups to menubars on GTK3.

>         ensure_menu_bar_item_widget();
>         gMenuPopupWidget = gtk_menu_new();
>-        gtk_menu_item_set_submenu(GTK_MENU_ITEM(gMenuBarItemWidget),
>-                                  gMenuPopupWidget);
>+        gtk_menu_attach_to_widget(GTK_MENU(gMenuPopupWidget), gProtoWindow,
>+                                  NULL);

Please replace ensure_menu_bar_item_widget() with ensure_window_widget().

>+STUB(gtk_menu_attach_to_widget)

I think this should be in COMMON_SYMBOLS for reasons described in
https://bugzilla.mozilla.org/show_bug.cgi?id=1180008#c11
Attachment #8629950 - Flags: review?(karlt) → review+
Updated. Skipping try push since this is a trivial GTK3-specific change.
Attachment #8629950 - Attachment is obsolete: true
Attachment #8630314 - Flags: review?(karlt)
Keywords: checkin-needed
Attachment #8630314 - Attachment description: Don't attach menu popups to menubars on GTK3. → Don't attach menu popups to menubars on GTK3. Carry r=karlt
Attachment #8630314 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/89ef09135c28
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.