Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

chrome/browser/ui/browser_command_controller_unittest.cc

 // Copyright (c) 2012 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/browser_command_controller.h"
 
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
@@ skipping 234 matching lines @@
 
 //////////////////////////////////////////////////////////////////////////////
 class BrowserCommandControllerFullscreenTest;
 
 // A test browser window that can toggle fullscreen state.
 class FullscreenTestBrowserWindow : public TestBrowserWindow,
                                     ExclusiveAccessContext {
  public:
   FullscreenTestBrowserWindow(
       BrowserCommandControllerFullscreenTest* test_browser)
-      : fullscreen_(false), test_browser_(test_browser) {}
+      : fullscreen_(false),
+        toolbar_showing_(false),
+        test_browser_(test_browser) {}
 
   ~FullscreenTestBrowserWindow() override {}
 
   // TestBrowserWindow overrides:
   bool ShouldHideUIForFullscreen() const override { return fullscreen_; }
   bool IsFullscreen() const override { return fullscreen_; }
   void EnterFullscreen(const GURL& url,
                        ExclusiveAccessBubbleType type) override {
     fullscreen_ = true;
   }
   void ExitFullscreen() override { fullscreen_ = false; }
+  bool IsToolbarShowing() const override { return toolbar_showing_; }
 
   ExclusiveAccessContext* GetExclusiveAccessContext() override { return this; }
 
   // Exclusive access interface:
   Profile* GetProfile() override;
   content::WebContents* GetActiveWebContents() override;
   void HideDownloadShelf() override {}
   void UnhideDownloadShelf() override {}
   void UpdateExclusiveAccessExitBubbleContent(
       const GURL& url,
       ExclusiveAccessBubbleType bubble_type) override {}
   void OnExclusiveAccessUserInput() override {}
 
+  // Setters

[msw, 2017/04/04 22:46:39]

nit: remove this comment, it doesn't help.

[Hzj_jie, 2017/04/05 18:09:16]

Done.

+  void ShowToolbar() { toolbar_showing_ = true; }

[msw, 2017/04/04 22:46:39]

nit: make this set_toolbar_showing(bool showing) { toolbar_showing_ = showing; }

[Hzj_jie, 2017/04/05 18:09:17]

Done.

+
  private:
   bool fullscreen_;
+  bool toolbar_showing_;
   BrowserCommandControllerFullscreenTest* test_browser_;
 
   DISALLOW_COPY_AND_ASSIGN(FullscreenTestBrowserWindow);
 };
 
 // Test that uses FullscreenTestBrowserWindow for its window.
 class BrowserCommandControllerFullscreenTest
     : public BrowserWithTestWindowTest {
  public:
   BrowserCommandControllerFullscreenTest() {}
@@ skipping 33 matching lines @@
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_DEVELOPER_MENU));
 #if defined(GOOGLE_CHROME_BUILD)
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FEEDBACK));
 #endif
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS));
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_EDIT_SEARCH_ENGINES));
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_VIEW_PASSWORDS));
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_ABOUT));
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_SHOW_APP_MENU));
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));

[msw, 2017/04/04 22:46:39]

Test IDC_EXIT (and others) here?

[Hzj_jie, 2017/04/05 18:09:16]

Done.

 
   // Simulate going fullscreen.
   chrome::ToggleFullscreenMode(browser());
   ASSERT_TRUE(browser()->window()->IsFullscreen());
   browser()->command_controller()->FullscreenStateChanged();
 
   // Most commands are disabled in fullscreen.
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_OPEN_CURRENT_URL));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_AS_TAB));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_TOOLBAR));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_LOCATION));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_SEARCH));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_MENU_BAR));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_NEXT_PANE));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_PREVIOUS_PANE));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_BOOKMARKS));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_DEVELOPER_MENU));
 #if defined(GOOGLE_CHROME_BUILD)
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FEEDBACK));
 #endif
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_EDIT_SEARCH_ENGINES));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_VIEW_PASSWORDS));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_ABOUT));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_APP_MENU));
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));

[msw, 2017/04/04 22:46:39]

Test IDC_EXIT (and others) here?

[Hzj_jie, 2017/04/05 18:09:17]

Done.

 
+  // In fullscreen, only the exit fullscreen commands are reserved. All other
+  // shortcuts should be delivered to the web page. See https://goo.gl/4tJ32G.

[msw, 2017/04/04 22:46:39]

Ditto, use the full url, or a crbug.com/# addy.

[Hzj_jie, 2017/04/05 18:09:17]

Done.

+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(

[msw, 2017/04/04 22:46:39]

nit: it would be nice to reduce the boilerplate for all these checks, to make
the behavior of the tests/expectations more readily apparent. Consider one or
both of these suggestions:
(1) creating command id array(s) with corresponding expectations (ie. const
int[] commands_reserved_in_fullscreen = { IDC..., IDC... }; ), or an array of
structs with bool values for the expected (per-fullscreen?) reserved/enabled
state, which you can then loop over like RenderTextTest:
  struct CommandExpectations { int command_id; bool reserved; bool enabled; };
  CommandExpectations fullscreen_expectations[] = { {IDC..., true, true}, ... };
  for (size_t i = 0; i < arraysize(fullscreen_expectations); ++i) {
    SCOPED_TRACE(base::StringPrintf("Testing i=[%" PRIuS "] '%d'", i,
fullscreen_expectations[i].command_id));
    EXPECT_EQ(fullscreen_expectations[i].enabled,
chrome::IsCommandEnabled(browser(), fullscreen_expectations[i].command_id));
If the enabled state always matches the reserved state, combine those bools
(2) Caching the BrowserCommandController and NativeWebKeyboardEvent like:
  BrowserCommandController* controller = browser()->command_controller();
  content::NativeWebKeyboardEvent event(blink::WebInputEvent::TypeFirst, 0, 0);
  EXPECT_FALSE(controller->IsReservedCommandOrKey(IDC_CLOSE_TAB, event);

[Hzj_jie, 2017/04/05 18:09:16]

Done.

+      IDC_CLOSE_TAB,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_CLOSE_WINDOW,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_NEW_INCOGNITO_WINDOW,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_NEW_TAB,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_NEW_WINDOW,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_SELECT_NEXT_TAB,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_SELECT_PREVIOUS_TAB,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_EXIT,

[msw, 2017/04/04 22:46:39]

Test IDC_FULLSCREEN here?

[Hzj_jie, 2017/04/05 18:09:16]

Done.

+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+
+  // Exit fullscreen.

[msw, 2017/04/04 22:46:39]

If this block is just a duplicate of the one at the top, it'd be extra nice to
use command arrays and loops, instead of copy-pasting this whole thing.

[Hzj_jie, 2017/04/05 18:09:17]

Yes, updated.

+  chrome::ToggleFullscreenMode(browser());
+  ASSERT_FALSE(browser()->window()->IsFullscreen());
+  browser()->command_controller()->FullscreenStateChanged();
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPEN_CURRENT_URL));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_AS_TAB));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_TOOLBAR));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_LOCATION));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_SEARCH));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_MENU_BAR));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_NEXT_PANE));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_PREVIOUS_PANE));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_BOOKMARKS));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_DEVELOPER_MENU));
+#if defined(GOOGLE_CHROME_BUILD)
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FEEDBACK));
+#endif
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_EDIT_SEARCH_ENGINES));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_VIEW_PASSWORDS));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_ABOUT));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_SHOW_APP_MENU));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));
+
   EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_CLOSE_TAB,
       content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_CLOSE_WINDOW,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_NEW_INCOGNITO_WINDOW,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_NEW_TAB,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_NEW_WINDOW,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_SELECT_NEXT_TAB,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_SELECT_PREVIOUS_TAB,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_EXIT,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+
+  // Guest Profiles disallow some options.
+  TestingProfile* testprofile = browser()->profile()->AsTestingProfile();
+  EXPECT_TRUE(testprofile);
+  testprofile->SetGuestSession(true);
+
+  browser()->command_controller()->FullscreenStateChanged();
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS));
+}
+
+#if defined(OS_MACOSX)
+TEST_F(BrowserCommandControllerFullscreenTest,
+       TabShortcutsShouldBePreservedOnMacOSX) {
+  static_cast<FullscreenTestBrowserWindow*>(window())->ShowToolbar();
+
+  // Defaults for a tabbed browser.

[msw, 2017/04/04 22:46:39]

If this is a duplicate of the top of UpdateCommandsForFullscreenMode, it'd be
nice to use the array approach. It'd be even better to merge this test with that
above, with either some extra loops for Mac, or as a parameterized fixture
TEST_P() with a bool value for set_toolbar_showing or similar.

[Hzj_jie, 2017/04/05 18:09:17]

Merged into the previous test.

+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPEN_CURRENT_URL));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_AS_TAB));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_TOOLBAR));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_LOCATION));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_SEARCH));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_MENU_BAR));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_NEXT_PANE));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_PREVIOUS_PANE));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_BOOKMARKS));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_DEVELOPER_MENU));
+#if defined(GOOGLE_CHROME_BUILD)
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FEEDBACK));
+#endif
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_EDIT_SEARCH_ENGINES));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_VIEW_PASSWORDS));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_ABOUT));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_SHOW_APP_MENU));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));
+
+  // Simulate going fullscreen.
+  chrome::ToggleFullscreenMode(browser());
+  ASSERT_TRUE(browser()->window()->IsFullscreen());
+  browser()->command_controller()->FullscreenStateChanged();
+
+  // Most commands are disabled in fullscreen.
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_OPEN_CURRENT_URL));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_AS_TAB));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_TOOLBAR));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_LOCATION));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_SEARCH));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_MENU_BAR));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_NEXT_PANE));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_PREVIOUS_PANE));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_BOOKMARKS));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_DEVELOPER_MENU));
+#if defined(GOOGLE_CHROME_BUILD)
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FEEDBACK));
+#endif
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_EDIT_SEARCH_ENGINES));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_VIEW_PASSWORDS));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_ABOUT));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_APP_MENU));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));
+
+  // In fullscreen, once the toolbar is visible, browser shortcuts should still
+  // be reserved. See http://crbug.com/702251.
+  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_CLOSE_TAB,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
   EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_CLOSE_WINDOW,
       content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
   EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_NEW_INCOGNITO_WINDOW,
       content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
   EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_NEW_TAB,
       content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
   EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
@@ skipping 61 matching lines @@
 
   // Guest Profiles disallow some options.
   TestingProfile* testprofile = browser()->profile()->AsTestingProfile();
   EXPECT_TRUE(testprofile);
   testprofile->SetGuestSession(true);
 
   browser()->command_controller()->FullscreenStateChanged();
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS));
 }
+#endif
 
 // Ensure that the logic for enabling IDC_OPTIONS is consistent, regardless of
 // the order of entering fullscreen and forced incognito modes. See
 // http://crbug.com/694331.
 TEST_F(BrowserCommandControllerTest, OptionsConsistency) {
   TestingProfile* profile = browser()->profile()->AsTestingProfile();
   // Setup guest session.
   profile->SetGuestSession(true);
   // Setup forced incognito mode.
   IncognitoModePrefs::SetAvailability(browser()->profile()->GetPrefs(),
@@ skipping 38 matching lines @@
 
 TEST_F(BrowserCommandControllerTest, OnSigninAllowedPrefChange) {
   chrome::BrowserCommandController command_controller(browser());
   const CommandUpdater* command_updater = command_controller.command_updater();
 
   // Check that the SYNC_SETUP command is updated on preference change.
   EXPECT_TRUE(command_updater->IsCommandEnabled(IDC_SHOW_SYNC_SETUP));
   profile()->GetPrefs()->SetBoolean(prefs::kSigninAllowed, false);
   EXPECT_FALSE(command_updater->IsCommandEnabled(IDC_SHOW_SYNC_SETUP));
 }