[top-chrome-md] Made extension buttons show the highlight when focused.

chrome/browser/ui/views/toolbar/toolbar_action_view.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/toolbar/toolbar_action_view.h"
 
 #include <string>
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h"
 #include "chrome/browser/ui/toolbar/toolbar_actions_bar.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "content/public/browser/notification_source.h"
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/theme_provider.h"
 #include "ui/compositor/paint_recorder.h"
 #include "ui/events/event.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_operations.h"
 #include "ui/gfx/image/image_skia_source.h"
+#include "ui/views/animation/flood_fill_ink_drop_ripple.h"
 #include "ui/views/animation/ink_drop_impl.h"
 #include "ui/views/controls/button/label_button_border.h"
 #include "ui/views/controls/menu/menu_controller.h"
 #include "ui/views/controls/menu/menu_model_adapter.h"
 #include "ui/views/controls/menu/menu_runner.h"
 #include "ui/views/mouse_constants.h"
 
 using views::LabelButtonBorder;
 
 namespace {
@@ skipping 67 matching lines @@
   }
 
   return GetThemeProvider()->GetColor(
       ThemeProperties::COLOR_TOOLBAR_BUTTON_ICON);
 }
 
 std::unique_ptr<views::InkDrop> ToolbarActionView::CreateInkDrop() {
   std::unique_ptr<views::InkDropImpl> ink_drop =
       CustomButton::CreateDefaultInkDropImpl();
   ink_drop->SetShowHighlightOnHover(!delegate_->ShownInsideMenu());
+  ink_drop->SetShowHighlightOnFocus(true);
   return std::move(ink_drop);
 }
 
+std::unique_ptr<views::InkDropRipple> ToolbarActionView::CreateInkDropRipple()
+    const {
+  if (delegate_->ShownInsideMenu()) {
+    // The ripple should not be visible when |this| is shown inside of a menu.
+    // TODO(bruthig): Use a lighter weight InkDropRipple type that doesn't
+    // consume as many the resources as the FloodFillInkDropRipple. See
+    // https://crbug.com/665214.
+    return base::MakeUnique<views::FloodFillInkDropRipple>(
+        gfx::Rect(), gfx::Point(), SK_ColorTRANSPARENT, 0.f);

[Peter Kasting, 2016/11/15 00:38:39]

Can we just return null, and have the caller handle that correctly?

[bruthig, 2016/11/15 01:25:12]

Short answer, returning null is a possible long term solution but isn't that
straight forward to do since the InkDropImpl can kick off highlight animations
based on the starting/ending of ripple animations. This is a quick fix which is
strictly better. Issue 665214 will remain open afterwards to track the work
required for a better solution.

[Peter Kasting, 2016/11/15 01:47:36]

I commented there about the overall route here.  I'm concerned that removing the
ripple isn't the right behavior change to begin with.

How bad is this bug?  Can we just leave the ripple for items inside menus for
now, until (assuming we want to make this change) we have the right framework to
do this better, e.g. some kind of null ripple class that just kicks off
subsequent animations, or callers that kick off those animations themselves when
there's no ripple, or something?  It seems like the addition on line 14 is the
most critical one to get in soon?

[bruthig, 2016/11/15 03:47:50]

Done.

+  }
+
+  return MenuButton::CreateInkDropRipple();
+}
+
 content::WebContents* ToolbarActionView::GetCurrentWebContents() const {
   return delegate_->GetCurrentWebContents();
 }
 
 void ToolbarActionView::UpdateState() {
   content::WebContents* web_contents = GetCurrentWebContents();
   if (SessionTabHelper::IdForTab(web_contents) < 0)
     return;
 
   if (!view_controller_->IsEnabled(web_contents) &&
@@ skipping 185 matching lines @@
     if (menu_controller->in_nested_run()) {
       // There is another menu showing. Close the outermost menu (since we are
       // shown in the same menu, we don't want to close the whole thing).
       menu_controller->Cancel(views::MenuController::EXIT_OUTERMOST);
       return true;
     }
   }
 
   return false;
 }