Fix bug 378090

linux: Make sure we're getting colors from a GtkCustomMenu.

BUG=378090
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291468

[oshima, 2014-08-13 03:48:40]

I'm not familiar with this code. erg@ would be the right person. removing
myself.

[Elliot Glaysher, 2014-08-13 17:34:18]

OP: I don't know what you intend to do because your description isn't helpful.

estade: It looks like you added the code that this person is trying to delete.

[dbdaniel42, 2014-08-13 18:17:47]

On 2014/08/13 17:34:18, Elliot Glaysher wrote:
> OP: I don't know what you intend to do because your description isn't helpful.
> 
> estade: It looks like you added the code that this person is trying to delete.

Sorry, this my first patch.  Looks like I forgot to add a description.  This
prior revision introduced the bug I'm trying to fix:

https://codereview.chromium.org/243633003

https://code.google.com/p/chromium/issues/detail?id=378090

Chromium is supposed to pull the color for the background of a selected menu
item using the GtkCustomMenuItem GTK class.  This "native theme overrider"
breaks that behavior and instead pulls from GtkMenuItem when "Use Classic Theme"
is set.  This in turn makes the selected menu item invisible when using the
Adwaita theme as you can see from the screenshots in the bug report (Adwaita is
the default theme for Gnome 3 and for many Linux distributions.)

I recompiled Chromium with this section of code removed and verified that my bug
was indeed fixed by this change.

[Evan Stade, 2014-08-13 23:18:25]

On 2014/08/13 18:17:47, dbdaniel42 wrote:
> On 2014/08/13 17:34:18, Elliot Glaysher wrote:
> > OP: I don't know what you intend to do because your description isn't
helpful.
> > 
> > estade: It looks like you added the code that this person is trying to
delete.
> 
> Sorry, this my first patch.  Looks like I forgot to add a description.  This
> prior revision introduced the bug I'm trying to fix:
> 
> https://codereview.chromium.org/243633003
> 
> https://code.google.com/p/chromium/issues/detail?id=378090
> 
> Chromium is supposed to pull the color for the background of a selected menu
> item using the GtkCustomMenuItem GTK class.  This "native theme overrider"
> breaks that behavior and instead pulls from GtkMenuItem when "Use Classic
Theme"
> is set.  This in turn makes the selected menu item invisible when using the
> Adwaita theme as you can see from the screenshots in the bug report (Adwaita
is
> the default theme for Gnome 3 and for many Linux distributions.)
> 
> I recompiled Chromium with this section of code removed and verified that my
bug
> was indeed fixed by this change.

Believe it or not, this code exists for a reason :) Deleting it creates other
bugs. What you've done is essentially disabled "Use Classic Theme".

[Evan Stade, 2014-08-13 23:18:45]

so in case it wasn't clear, not lgtm

[dbdaniel42, 2014-08-13 23:42:37]

On 2014/08/13 23:18:25, Evan Stade wrote:
> On 2014/08/13 18:17:47, dbdaniel42 wrote:
> > On 2014/08/13 17:34:18, Elliot Glaysher wrote:
> > > OP: I don't know what you intend to do because your description isn't
> helpful.
> > > 
> > > estade: It looks like you added the code that this person is trying to
> delete.
> > 
> > Sorry, this my first patch.  Looks like I forgot to add a description.  This
> > prior revision introduced the bug I'm trying to fix:
> > 
> > https://codereview.chromium.org/243633003
> > 
> > https://code.google.com/p/chromium/issues/detail?id=378090
> > 
> > Chromium is supposed to pull the color for the background of a selected menu
> > item using the GtkCustomMenuItem GTK class.  This "native theme overrider"
> > breaks that behavior and instead pulls from GtkMenuItem when "Use Classic
> Theme"
> > is set.  This in turn makes the selected menu item invisible when using the
> > Adwaita theme as you can see from the screenshots in the bug report (Adwaita
> is
> > the default theme for Gnome 3 and for many Linux distributions.)
> > 
> > I recompiled Chromium with this section of code removed and verified that my
> bug
> > was indeed fixed by this change.
> 
> Believe it or not, this code exists for a reason :) Deleting it creates other
> bugs. What you've done is essentially disabled "Use Classic Theme".

I thought that might be the case.  If I'm reading your revision correctly, you
added this code to hard-code some colors for the classic theme (as opposed to
grabbing the colors from the GTK theme.)  Unfortunately, the menu item
background color (kColorId_FocusedMenuItemBackgroundColor) is still being
grabbed from the GTK theme.  The only difference is now, for some reason, the
GTK class name is being changed when using the classic theme.

I'll try to find a more elegant solution that doesn't disable your code but if
you have any ideas I'm all ears.  I thought at the very least this patch would
show you the source of this bug.

[Evan Stade, 2014-08-13 23:50:08]

On 2014/08/13 23:42:37, dbdaniel42 wrote:
> On 2014/08/13 23:18:25, Evan Stade wrote:
> > On 2014/08/13 18:17:47, dbdaniel42 wrote:
> > > On 2014/08/13 17:34:18, Elliot Glaysher wrote:
> > > > OP: I don't know what you intend to do because your description isn't
> > helpful.
> > > > 
> > > > estade: It looks like you added the code that this person is trying to
> > delete.
> > > 
> > > Sorry, this my first patch.  Looks like I forgot to add a description. 
This
> > > prior revision introduced the bug I'm trying to fix:
> > > 
> > > https://codereview.chromium.org/243633003
> > > 
> > > https://code.google.com/p/chromium/issues/detail?id=378090
> > > 
> > > Chromium is supposed to pull the color for the background of a selected
menu
> > > item using the GtkCustomMenuItem GTK class.  This "native theme overrider"
> > > breaks that behavior and instead pulls from GtkMenuItem when "Use Classic
> > Theme"
> > > is set.  This in turn makes the selected menu item invisible when using
the
> > > Adwaita theme as you can see from the screenshots in the bug report
(Adwaita
> > is
> > > the default theme for Gnome 3 and for many Linux distributions.)
> > > 
> > > I recompiled Chromium with this section of code removed and verified that
my
> > bug
> > > was indeed fixed by this change.
> > 
> > Believe it or not, this code exists for a reason :) Deleting it creates
other
> > bugs. What you've done is essentially disabled "Use Classic Theme".
> 
> I thought that might be the case.  If I'm reading your revision correctly, you
> added this code to hard-code some colors for the classic theme (as opposed to
> grabbing the colors from the GTK theme.)  Unfortunately, the menu item
> background color (kColorId_FocusedMenuItemBackgroundColor) is still being
> grabbed from the GTK theme.  The only difference is now, for some reason, the
> GTK class name is being changed when using the classic theme.

Why are we hitting NativeThemeGtk2::GetSystemGdkColor() at all? What is the call
stack? We should be using NativeThemeAura.

> 
> I'll try to find a more elegant solution that doesn't disable your code but if
> you have any ideas I'm all ears.  I thought at the very least this patch would
> show you the source of this bug.

[dbdaniel42, 2014-08-14 00:01:30]

On 2014/08/13 23:50:08, Evan Stade wrote:
> On 2014/08/13 23:42:37, dbdaniel42 wrote:
> > On 2014/08/13 23:18:25, Evan Stade wrote:
> > > On 2014/08/13 18:17:47, dbdaniel42 wrote:
> > > > On 2014/08/13 17:34:18, Elliot Glaysher wrote:
> > > > > OP: I don't know what you intend to do because your description isn't
> > > helpful.
> > > > > 
> > > > > estade: It looks like you added the code that this person is trying to
> > > delete.
> > > > 
> > > > Sorry, this my first patch.  Looks like I forgot to add a description. 
> This
> > > > prior revision introduced the bug I'm trying to fix:
> > > > 
> > > > https://codereview.chromium.org/243633003
> > > > 
> > > > https://code.google.com/p/chromium/issues/detail?id=378090
> > > > 
> > > > Chromium is supposed to pull the color for the background of a selected
> menu
> > > > item using the GtkCustomMenuItem GTK class.  This "native theme
overrider"
> > > > breaks that behavior and instead pulls from GtkMenuItem when "Use
Classic
> > > Theme"
> > > > is set.  This in turn makes the selected menu item invisible when using
> the
> > > > Adwaita theme as you can see from the screenshots in the bug report
> (Adwaita
> > > is
> > > > the default theme for Gnome 3 and for many Linux distributions.)
> > > > 
> > > > I recompiled Chromium with this section of code removed and verified
that
> my
> > > bug
> > > > was indeed fixed by this change.
> > > 
> > > Believe it or not, this code exists for a reason :) Deleting it creates
> other
> > > bugs. What you've done is essentially disabled "Use Classic Theme".
> > 
> > I thought that might be the case.  If I'm reading your revision correctly,
you
> > added this code to hard-code some colors for the classic theme (as opposed
to
> > grabbing the colors from the GTK theme.)  Unfortunately, the menu item
> > background color (kColorId_FocusedMenuItemBackgroundColor) is still being
> > grabbed from the GTK theme.  The only difference is now, for some reason,
the
> > GTK class name is being changed when using the classic theme.
> 
> Why are we hitting NativeThemeGtk2::GetSystemGdkColor() at all? What is the
call
> stack? We should be using NativeThemeAura.
> 
> > 
> > I'll try to find a more elegant solution that doesn't disable your code but
if
> > you have any ideas I'm all ears.  I thought at the very least this patch
would
> > show you the source of this bug.

I think it's this code in src/chrome/browser/ui/views/toolbar/wrench_menu.cc

  static SkColor BackgroundColor(const View* view,
                                 views::Button::ButtonState state) {
    const ui::NativeTheme* theme = view->GetNativeTheme();
    switch (state) {
      case views::Button::STATE_HOVERED:
        // Hovered should be handled in DrawBackground.
        NOTREACHED();
        return theme->GetSystemColor(
            ui::NativeTheme::kColorId_HoverMenuItemBackgroundColor);
      case views::Button::STATE_PRESSED:
        return theme->GetSystemColor(
            ui::NativeTheme::kColorId_FocusedMenuItemBackgroundColor);
      default:
        return theme->GetSystemColor(
            ui::NativeTheme::kColorId_MenuBackgroundColor);

[Evan Stade, 2014-08-14 00:12:43]

On 2014/08/14 00:01:30, dbdaniel42 wrote:
> On 2014/08/13 23:50:08, Evan Stade wrote:
> > On 2014/08/13 23:42:37, dbdaniel42 wrote:
> > > On 2014/08/13 23:18:25, Evan Stade wrote:
> > > > On 2014/08/13 18:17:47, dbdaniel42 wrote:
> > > > > On 2014/08/13 17:34:18, Elliot Glaysher wrote:
> > > > > > OP: I don't know what you intend to do because your description
isn't
> > > > helpful.
> > > > > > 
> > > > > > estade: It looks like you added the code that this person is trying
to
> > > > delete.
> > > > > 
> > > > > Sorry, this my first patch.  Looks like I forgot to add a description.

> > This
> > > > > prior revision introduced the bug I'm trying to fix:
> > > > > 
> > > > > https://codereview.chromium.org/243633003
> > > > > 
> > > > > https://code.google.com/p/chromium/issues/detail?id=378090
> > > > > 
> > > > > Chromium is supposed to pull the color for the background of a
selected
> > menu
> > > > > item using the GtkCustomMenuItem GTK class.  This "native theme
> overrider"
> > > > > breaks that behavior and instead pulls from GtkMenuItem when "Use
> Classic
> > > > Theme"
> > > > > is set.  This in turn makes the selected menu item invisible when
using
> > the
> > > > > Adwaita theme as you can see from the screenshots in the bug report
> > (Adwaita
> > > > is
> > > > > the default theme for Gnome 3 and for many Linux distributions.)
> > > > > 
> > > > > I recompiled Chromium with this section of code removed and verified
> that
> > my
> > > > bug
> > > > > was indeed fixed by this change.
> > > > 
> > > > Believe it or not, this code exists for a reason :) Deleting it creates
> > other
> > > > bugs. What you've done is essentially disabled "Use Classic Theme".
> > > 
> > > I thought that might be the case.  If I'm reading your revision correctly,
> you
> > > added this code to hard-code some colors for the classic theme (as opposed
> to
> > > grabbing the colors from the GTK theme.)  Unfortunately, the menu item
> > > background color (kColorId_FocusedMenuItemBackgroundColor) is still being
> > > grabbed from the GTK theme.  The only difference is now, for some reason,
> the
> > > GTK class name is being changed when using the classic theme.
> > 
> > Why are we hitting NativeThemeGtk2::GetSystemGdkColor() at all? What is the
> call
> > stack? We should be using NativeThemeAura.
> > 
> > > 
> > > I'll try to find a more elegant solution that doesn't disable your code
but
> if
> > > you have any ideas I'm all ears.  I thought at the very least this patch
> would
> > > show you the source of this bug.
> 
> I think it's this code in src/chrome/browser/ui/views/toolbar/wrench_menu.cc
> 
>   static SkColor BackgroundColor(const View* view,
>                                  views::Button::ButtonState state) {
>     const ui::NativeTheme* theme = view->GetNativeTheme();
>     switch (state) {
>       case views::Button::STATE_HOVERED:
>         // Hovered should be handled in DrawBackground.
>         NOTREACHED();
>         return theme->GetSystemColor(
>             ui::NativeTheme::kColorId_HoverMenuItemBackgroundColor);
>       case views::Button::STATE_PRESSED:
>         return theme->GetSystemColor(
>             ui::NativeTheme::kColorId_FocusedMenuItemBackgroundColor);
>       default:
>         return theme->GetSystemColor(
>             ui::NativeTheme::kColorId_MenuBackgroundColor);

a) that code wouldn't apply to the context menu (which is mentioned in the
original bug report)
b) that code looks correct --- |theme| should be a NativeThemeAura instance
though, so you wouldn't hit NativeThemeGtk2::GetSystemColor

[dbdaniel42, 2014-08-18 18:52:03]

Please review my new patch.  From looking at the code further, the best I can
tell is that the calling function is void MenuItemView::PaintButton(gfx::Canvas*
canvas, PaintButtonMode mode) in src/ui/views/controls/menu/menu_item_view.cc 
Like the wrench_menu.cc function it *looks* like it should use GetNativeTheme()
and re-route to Aura but for some reason it does not and uses GTK to get the
menu item color.

This patch simply changes the color from using bg[SELECTED] to bg[PRELIGHT]
which is more standard among GTK themes/applications.  It's still not using the
GtkCustomMenuItem class but this change puts the Menu Item color in line with
what GTK expects so a custom class isn't needed.  I confirmed this fixes the bug
on the Adwaita theme.

[Elliot Glaysher, 2014-08-19 17:42:32]

On 2014/08/18 18:52:03, dbdaniel42 wrote:
> Please review my new patch.  From looking at the code further, the best I can
> tell is that the calling function is void
MenuItemView::PaintButton(gfx::Canvas*
> canvas, PaintButtonMode mode) in src/ui/views/controls/menu/menu_item_view.cc 
> Like the wrench_menu.cc function it *looks* like it should use
GetNativeTheme()
> and re-route to Aura but for some reason it does not and uses GTK to get the
> menu item color.
> 
> This patch simply changes the color from using bg[SELECTED] to bg[PRELIGHT]
> which is more standard among GTK themes/applications.  It's still not using
the
> GtkCustomMenuItem class but this change puts the Menu Item color in line with
> what GTK expects so a custom class isn't needed.  I confirmed this fixes the
bug
> on the Adwaita theme.

I patched this in locally; it seems to break Canonical's Ambiance. (The menu
background is now white text on a white background on hover.)

[dbdaniel42, 2014-08-20 16:16:31]

The CQ bit was checked by dbdaniel42@gmail.com

[dbdaniel42, 2014-08-20 16:16:32]

The CQ bit was unchecked by dbdaniel42@gmail.com

[dbdaniel42, 2014-08-20 16:38:52]

OK third time's a charm I hope.  This should make it use the correct
GtkCustomMenu class.  I verified that it fixed my bug and also built it on an
Ubuntu VM to verify that it doesn't break anything there.

[Elliot Glaysher, 2014-08-22 17:37:04]

lgtm

Some background: we have a GtkCustomMenu class for semiinteresting historical
reasons. When we first extended GtkMenu to allow buttons in them, we named our
class GtkCustomMenu. IIRC (and it's been a few years so I might get the details
here wrong), Canonical started relying on the class's existence. They added
rules on GtkMenu to work around how OpenOffice themed their menus, which they
noted in the comments was wrong, but didn't seem to break any other gtk apps.
They noticed that their workaround broke Chrome, so they do their OO.org
workaround on GtkMenu and then reset the defaults GtkCustomMenu.

https://codereview.chromium.org/465203002/diff/40001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right):

https://codereview.chromium.org/465203002/diff/40001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:441:
fake_menu_.Own(gtk_custom_menu_new());
Yeah, this was obviously wrong. Thanks for fixing it.

[Elliot Glaysher, 2014-08-22 17:37:38]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-08-22 17:38:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dbdaniel42@gmail.com/465203002/40001

[commit-bot: I haz the power, 2014-08-22 17:38:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 17:38:11]

A disapproval has been posted by following reviewers: estade@chromium.org.
Please make sure to get positive review before another CQ attempt.

[Evan Stade, 2014-08-22 18:04:45]

ok, lgtm

[Evan Stade, 2014-08-22 18:04:48]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-08-22 18:05:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dbdaniel42@gmail.com/465203002/40001

[commit-bot: I haz the power, 2014-08-22 19:02:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-22 19:14:19]

Committed patchset #3 (40001) as 291468