Enable keyboard shortcuts for hosted app window frames

This CL enables the following keyboard shortcuts for hosted
app windows:
 - Shift-Alt-T: Focus window frame toolbar buttons
 - Alt-E and Alt-F: Open app menu
 - F6: Cycle focus between frame buttons and web contents.

Screencast: https://bugs.chromium.org/p/chromium/issues/attachment?aid=330498&signed_aid=BFJfbOWHFSLjA4dEetB3SA==&inline=1

Bug: 822146
Change-Id: Ieb00735dc92c914cb2eb2022bdbd7d72e9c3b603
Reviewed-on: https://chromium-review.googlesource.com/977521
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549834}

chrome/browser/ui/browser_command_controller.cc

@@ -1179,6 +1179,20 @@ void BrowserCommandController::UpdateCommandsForFullscreenMode() {
 
   UpdateCommandsForBookmarkBar();
   UpdateCommandsForIncognitoAvailability();
+  UpdateCommandsForHostedAppAvailability();
+}
+
+void BrowserCommandController::UpdateCommandsForHostedAppAvailability() {
+  bool has_toolbar =
+      browser_->is_type_tabbed() ||
+      extensions::HostedAppBrowserController::IsForExperimentalHostedAppBrowser(
+          browser_);
+  if (window() && window()->ShouldHideUIForFullscreen())
+    has_toolbar = false;
+  command_updater_.UpdateCommandEnabled(IDC_FOCUS_TOOLBAR, has_toolbar);
+  command_updater_.UpdateCommandEnabled(IDC_FOCUS_NEXT_PANE, has_toolbar);
+  command_updater_.UpdateCommandEnabled(IDC_FOCUS_PREVIOUS_PANE, has_toolbar);
+  command_updater_.UpdateCommandEnabled(IDC_SHOW_APP_MENU, has_toolbar);
 }
 
 #if defined(OS_CHROMEOS)

chrome/browser/ui/browser_command_controller.h

@@ -147,6 +147,10 @@ class BrowserCommandController : public CommandUpdater,
   // window is in.
   void UpdateCommandsForFullscreenMode();
 
+  // Update commands whose state depends on whether they're available to hosted
+  // app windows.
+  void UpdateCommandsForHostedAppAvailability();
+
 #if defined(OS_CHROMEOS)
   // Update commands whose state depends on whether the window is in locked
   // fullscreen mode or not.

chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc

@@ -613,6 +613,7 @@ class HostedAppNonClientFrameViewAshTest
         BrowserActionsBarBrowserTest>::SetUpOnMainThread();
 
     scoped_feature_list_.InitAndEnableFeature(features::kDesktopPWAWindowing);
+    HostedAppButtonContainer::DisableAnimationForTesting();
 
     WebApplicationInfo web_app_info;
     web_app_info.app_url = GetAppURL();
@@ -649,6 +650,22 @@ class HostedAppNonClientFrameViewAshTest
     return hosted_app_button_container_->active_icon_color_;
   }
 
+  ContentSettingImageView* GrantGeolocationPermission() {
+    content::RenderFrameHost* frame =
+        app_browser_->tab_strip_model()->GetActiveWebContents()->GetMainFrame();
+    TabSpecificContentSettings* content_settings =
+        TabSpecificContentSettings::GetForFrame(frame->GetProcess()->GetID(),
+                                                frame->GetRoutingID());
+    content_settings->OnGeolocationPermissionSet(GetAppURL().GetOrigin(), true);
+
+    return *std::find_if(
+        content_setting_views_->begin(), content_setting_views_->end(),
+        [](auto v) {
+          return ContentSettingImageModel::ImageType::GEOLOCATION ==
+                 v->GetTypeForTesting();
+        });
+  }
+
   void SimulateClickOnView(views::View* view) {
     const gfx::Point point;
     ui::MouseEvent event(ui::ET_MOUSE_PRESSED, point, point,
@@ -683,29 +700,68 @@ IN_PROC_BROWSER_TEST_P(HostedAppNonClientFrameViewAshTest, ThemeColor) {
             GetActiveIconColor(hosted_app_button_container_));
 }
 
+// Tests that the focus toolbar command focuses the app menu button in web app
+// windows.
+IN_PROC_BROWSER_TEST_P(HostedAppNonClientFrameViewAshTest,
+                       BrowserCommandFocusToolbarAppMenu) {
+  EXPECT_FALSE(app_menu_button_->HasFocus());
+  chrome::ExecuteCommand(app_browser_, IDC_FOCUS_TOOLBAR);
+  EXPECT_TRUE(app_menu_button_->HasFocus());
+}
+
+// Tests that the focus toolbar command focuses content settings icons before
+// the app menu button when present in web app windows.
+IN_PROC_BROWSER_TEST_P(HostedAppNonClientFrameViewAshTest,
+                       BrowserCommandFocusToolbarGeolocation) {
+  ContentSettingImageView* geolocation_icon = GrantGeolocationPermission();
+
+  EXPECT_FALSE(app_menu_button_->HasFocus());
+  EXPECT_FALSE(geolocation_icon->HasFocus());
+
+  chrome::ExecuteCommand(app_browser_, IDC_FOCUS_TOOLBAR);
+
+  EXPECT_FALSE(app_menu_button_->HasFocus());
+  EXPECT_TRUE(geolocation_icon->HasFocus());
+}
+
+// Tests that the show app menu command opens the app menu for web app windows.
+IN_PROC_BROWSER_TEST_P(HostedAppNonClientFrameViewAshTest,
+                       BrowserCommandShowAppMenu) {
+  EXPECT_EQ(nullptr, GetAppMenu(hosted_app_button_container_));
+  chrome::ExecuteCommand(app_browser_, IDC_SHOW_APP_MENU);
+  EXPECT_NE(nullptr, GetAppMenu(hosted_app_button_container_));
+}
+
+// Tests that the focus next pane command focuses the app menu for web app
+// windows.
+IN_PROC_BROWSER_TEST_P(HostedAppNonClientFrameViewAshTest,
+                       BrowserCommandFocusNextPane) {
+  EXPECT_FALSE(app_menu_button_->HasFocus());
+  chrome::ExecuteCommand(app_browser_, IDC_FOCUS_NEXT_PANE);
+  EXPECT_TRUE(app_menu_button_->HasFocus());
+}
+
+// Tests that the focus previous pane command focuses the app menu for web app
+// windows.
+IN_PROC_BROWSER_TEST_P(HostedAppNonClientFrameViewAshTest,
+                       BrowserCommandFocusPreviousPane) {
+  EXPECT_FALSE(app_menu_button_->HasFocus());
+  chrome::ExecuteCommand(app_browser_, IDC_FOCUS_PREVIOUS_PANE);
+  EXPECT_TRUE(app_menu_button_->HasFocus());
+}
+
 // Tests that a web app's content settings icons can be interacted with.
 IN_PROC_BROWSER_TEST_P(HostedAppNonClientFrameViewAshTest,
                        ContentSettingIcons) {
-  for (auto* v : *content_setting_views_)
-    EXPECT_FALSE(v->visible());
-
-  // Invoke the the geolocation icon to show.
-  content::RenderFrameHost* frame =
-      app_browser_->tab_strip_model()->GetActiveWebContents()->GetMainFrame();
-  TabSpecificContentSettings* content_settings =
-      TabSpecificContentSettings::GetForFrame(frame->GetProcess()->GetID(),
-                                              frame->GetRoutingID());
-  content_settings->OnGeolocationPermissionSet(GetAppURL().GetOrigin(), true);
-
-  auto is_visible = [](auto v) { return v->visible(); };
-  EXPECT_EQ(1, std::count_if(content_setting_views_->begin(),
-                             content_setting_views_->end(), is_visible));
-
-  ContentSettingImageView* geolocation_icon =
-      *std::find_if(content_setting_views_->begin(),
-                    content_setting_views_->end(), is_visible);
-  EXPECT_TRUE(ContentSettingImageModel::ImageType::GEOLOCATION ==
-              geolocation_icon->GetTypeForTesting());
+  for (auto* view : *content_setting_views_)
+    EXPECT_FALSE(view->visible());
+
+  ContentSettingImageView* geolocation_icon = GrantGeolocationPermission();
+
+  for (auto* view : *content_setting_views_) {
+    bool is_geolocation_icon = view == geolocation_icon;
+    EXPECT_EQ(is_geolocation_icon, view->visible());
+  }
 
   // Press the geolocation button.
   base::HistogramTester histograms;

chrome/browser/ui/views/frame/browser_view.cc

@@ -1029,7 +1029,7 @@ void BrowserView::FocusToolbar() {
 
   // Start the traversal within the main toolbar. SetPaneFocus stores
   // the current focused view before changing focus.
-  toolbar_->SetPaneFocus(nullptr);
+  toolbar_button_provider_->FocusToolbar();
 }
 
 ToolbarActionsBar* BrowserView::GetToolbarActionsBar() {
@@ -1919,7 +1919,7 @@ void BrowserView::GetAccessiblePanes(std::vector<views::View*>* panes) {
   // (Windows) or Ctrl+Back/Forward (Chrome OS).  If one of these is
   // invisible or has no focusable children, it will be automatically
   // skipped.
-  panes->push_back(toolbar_);
+  panes->push_back(toolbar_button_provider_->GetAsAccessiblePaneView());
   if (bookmark_bar_view_.get())
     panes->push_back(bookmark_bar_view_.get());
   if (infobar_container_)

chrome/browser/ui/views/frame/hosted_app_button_container.cc

@@ -25,6 +25,8 @@
 
 namespace {
 
+bool g_animation_disabled_for_testing = false;
+
 constexpr base::TimeDelta kContentSettingsFadeInDuration =
     base::TimeDelta::FromMilliseconds(500);
 
@@ -110,6 +112,10 @@ class HostedAppButtonContainer::ContentSettingsContainer
   DISALLOW_COPY_AND_ASSIGN(ContentSettingsContainer);
 };
 
+void HostedAppButtonContainer::DisableAnimationForTesting() {
+  g_animation_disabled_for_testing = true;
+}
+
 const std::vector<ContentSettingImageView*>&
 HostedAppButtonContainer::GetContentSettingViewsForTesting() const {
   return content_settings_container_->GetContentSettingViewsForTesting();
@@ -124,10 +130,12 @@ HostedAppButtonContainer::ContentSettingsContainer::ContentSettingsContainer(
       views::LayoutProvider::Get()->GetDistanceMetric(
           views::DISTANCE_RELATED_CONTROL_HORIZONTAL)));
 
-  SetVisible(false);
-  SetPaintToLayer();
-  layer()->SetFillsBoundsOpaquely(false);
-  layer()->SetOpacity(0);
+  if (!g_animation_disabled_for_testing) {
+    SetVisible(false);
+    SetPaintToLayer();
+    layer()->SetFillsBoundsOpaquely(false);
+    layer()->SetOpacity(0);
+  }
 
   std::vector<std::unique_ptr<ContentSettingImageModel>> models =
       ContentSettingImageModel::GenerateContentSettingImageModels();
@@ -198,6 +206,9 @@ void HostedAppButtonContainer::SetPaintAsActive(bool active) {
 
 void HostedAppButtonContainer::StartTitlebarAnimation(
     base::TimeDelta origin_text_slide_duration) {
+  if (g_animation_disabled_for_testing)
+    return;
+
   app_menu_button_->StartHighlightAnimation(origin_text_slide_duration);
 
   fade_in_content_setting_buttons_timer_.Start(
@@ -247,3 +258,11 @@ HostedAppButtonContainer::GetBrowserActionsContainer() {
 AppMenuButton* HostedAppButtonContainer::GetAppMenuButton() {
   return app_menu_button_;
 }
+
+void HostedAppButtonContainer::FocusToolbar() {
+  SetPaneFocus(nullptr);
+}
+
+views::AccessiblePaneView* HostedAppButtonContainer::GetAsAccessiblePaneView() {
+  return this;
+}

chrome/browser/ui/views/frame/hosted_app_button_container.h

@@ -11,6 +11,9 @@
 #include "chrome/browser/ui/views/frame/toolbar_button_provider.h"
 #include "chrome/browser/ui/views/location_bar/content_setting_image_view.h"
 #include "chrome/browser/ui/views/toolbar/browser_actions_container.h"
+#include "ui/views/accessible_pane_view.h"
+#include "ui/views/controls/button/menu_button.h"
+#include "ui/views/controls/button/menu_button_listener.h"
 #include "ui/views/view.h"
 
 namespace {
@@ -26,7 +29,7 @@ class MenuButton;
 }
 
 // A container for hosted app buttons in the title bar.
-class HostedAppButtonContainer : public views::View,
+class HostedAppButtonContainer : public views::AccessiblePaneView,
                                  public BrowserActionsContainer::Delegate,
                                  public ToolbarButtonProvider {
  public:
@@ -50,6 +53,8 @@ class HostedAppButtonContainer : public views::View,
  private:
   friend class HostedAppNonClientFrameViewAshTest;
 
+  static void DisableAnimationForTesting();
+
   class ContentSettingsContainer;
 
   const std::vector<ContentSettingImageView*>&
@@ -72,6 +77,8 @@ class HostedAppButtonContainer : public views::View,
   // ToolbarButtonProvider:
   BrowserActionsContainer* GetBrowserActionsContainer() override;
   AppMenuButton* GetAppMenuButton() override;
+  void FocusToolbar() override;
+  views::AccessiblePaneView* GetAsAccessiblePaneView() override;
 
   // The containing browser view.
   BrowserView* browser_view_;

chrome/browser/ui/views/frame/hosted_app_menu_button.cc

@@ -23,6 +23,8 @@ constexpr int kMenuHighlightFadeDurationMs = 800;
 HostedAppMenuButton::HostedAppMenuButton(BrowserView* browser_view)
     : AppMenuButton(this), browser_view_(browser_view) {
   SetInkDropMode(InkDropMode::ON);
+  // Disable focus ring for consistency with sibling buttons and AppMenuButton.
+  SetFocusPainter(nullptr);
   // This name is guaranteed not to change during the lifetime of this button.
   // Get the app name only, aka "Google Docs" instead of "My Doc - Google Docs",
   // because the menu applies to the entire app.

chrome/browser/ui/views/frame/toolbar_button_provider.h

@@ -8,6 +8,10 @@
 class AppMenuButton;
 class BrowserActionsContainer;
 
+namespace views {
+class AccessiblePaneView;
+}
+
 // An interface implemented by a view contains and provides access to toolbar
 // buttons in a BrowserView.
 class ToolbarButtonProvider {
@@ -18,6 +22,12 @@ class ToolbarButtonProvider {
   // Gets the app menu button.
   virtual AppMenuButton* GetAppMenuButton() = 0;
 
+  // Gives the toolbar focus.
+  virtual void FocusToolbar() = 0;
+
+  // Returns the toolbar as an AccessiblePaneView.
+  virtual views::AccessiblePaneView* GetAsAccessiblePaneView() = 0;
+
   // TODO(calamity): Move other buttons and button actions into here.
  protected:
   virtual ~ToolbarButtonProvider() {}

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

@@ -708,6 +708,14 @@ AppMenuButton* ToolbarView::GetAppMenuButton() {
   return app_menu_button_;
 }
 
+void ToolbarView::FocusToolbar() {
+  SetPaneFocus(nullptr);
+}
+
+views::AccessiblePaneView* ToolbarView::GetAsAccessiblePaneView() {
+  return this;
+}
+
 gfx::Size ToolbarView::GetSizeInternal(
     gfx::Size (View::*get_size)() const) const {
   gfx::Size size((location_bar_->*get_size)());

chrome/browser/ui/views/toolbar/toolbar_view.h

@@ -184,6 +184,8 @@ class ToolbarView : public views::AccessiblePaneView,
   // ToolbarButtonProvider:
   BrowserActionsContainer* GetBrowserActionsContainer() override;
   AppMenuButton* GetAppMenuButton() override;
+  void FocusToolbar() override;
+  views::AccessiblePaneView* GetAsAccessiblePaneView() override;
 
   // Used to avoid duplicating the near-identical logic of
   // ToolbarView::CalculatePreferredSize() and ToolbarView::GetMinimumSize().