1176109 - Popup menu text is too dark on some GTK3 themes

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Andrew Comminos [:acomminos]]

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.

Comment 1

[Andrew Comminos [:acomminos]]

Attachment: Fix popup text colour on some GTK3 themes.

This patch adds the menubar styling to the context, as is done by GTK when rendering popups.

Comment 2

[Karl Tomlinson (: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.

Comment 3

[Andrew Comminos [:acomminos]]

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.

Comment 4

[Karl Tomlinson (: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.

Comment 5

[Andrew Comminos [:acomminos]]

Attachment: Don't attach menu popups to menubars on GTK3.

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.

Comment 6

[Karl Tomlinson (: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.

Comment 7

[Andrew Comminos [:acomminos]]

Attachment: Don't attach menu popups to menubars on GTK3.

Thanks, updated to attach menu to gProtoWindow.

Comment 8

[Karl Tomlinson (: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

Comment 9

[Andrew Comminos [:acomminos]]

Attachment: Don't attach menu popups to menubars on GTK3. Carry r=karlt

Updated. Skipping try push since this is a trivial GTK3-specific change.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89ef09135c28

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/89ef09135c28