Settings: do not handle cmd+ctrl+f as a find shortcut

Bug: 869528
Change-Id: I07ed3e041a24612b063d9b85e05305811b903fb9
Reviewed-on: https://chromium-review.googlesource.com/1159324Reviewed-by: Scott Chen <scottchen@chromium.org>
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581298}

chrome/browser/resources/settings/BUILD.gn

@@ -130,6 +130,7 @@ js_library("find_shortcut_behavior") {
   deps = [
     "//ui/webui/resources/js:assert",
     "//ui/webui/resources/js:cr",
+    "//ui/webui/resources/js/cr/ui:command",
   ]
 }
 

chrome/browser/resources/settings/find_shortcut_behavior.html

 <link rel="import" href="chrome://resources/html/assert.html">
 <link rel="import" href="chrome://resources/html/cr.html">
+<link rel="import" href="chrome://resources/html/cr/ui/command.html">
 <script src="find_shortcut_behavior.js"></script>

chrome/browser/resources/settings/find_shortcut_behavior.js

@@ -23,15 +23,14 @@ cr.define('settings', function() {
    */
   let modalContextOpen = false;
 
+  const shortcut =
+      new cr.ui.KeyboardShortcutList(cr.isMac ? 'meta|f' : 'ctrl|f');
+
   window.addEventListener('keydown', e => {
-    if (listeners.length == 0)
+    if (e.defaultPrevented || listeners.length == 0)
       return;
 
-    const modifierPressed = cr.isMac ? e.metaKey : e.ctrlKey;
-    if (modifierPressed && e.key == 'f') {
-      if (e.defaultPrevented)
-        return;
-
+    if (shortcut.matchesEvent(e)) {
       const listener = /** @type {!{handleFindShortcut: function(boolean)}} */ (
           listeners[listeners.length - 1]);
       if (listener.handleFindShortcut(modalContextOpen))

chrome/test/data/webui/settings/find_shortcut_behavior_test.js

@@ -25,6 +25,9 @@ suite('find-shortcut', () => {
       fns.reduce((acc, fn) => acc.then(fn), Promise.resolve());
 
   /**
+   * This method registers the |testElement| as a find shortcut listener, runs
+   * the |wrapped| function, then removes |testElement| from being a listener.
+   * The listeners stack is global and should be empty after a successful test.
    * @param {!HTMLElement} testElement
    * @return {!function(!Function)): !Promise}
    */
@@ -35,6 +38,9 @@ suite('find-shortcut', () => {
   ]);
 
   /**
+   * Checks that the handleFindShortcut method is being called for all the
+   * |expectedSelves| element references in the order that they are passed in
+   * when a single find shortcut is invoked.
    * @param {!Array<!HTMLElement>} expectedSelves
    * @param {?boolean} expectedModalContextOpen
    * @return {!Promise}
@@ -53,6 +59,8 @@ suite('find-shortcut', () => {
   };
 
   /**
+   * Checks that the handleFindShortcut method is being called for the
+   * element reference |expectedSelf| when a find shortcut is invoked.
    * @param {!HTMLElement} expectedSelf
    * @param {?boolean} expectedModalContextOpen
    * @return {!Promise}
@@ -61,6 +69,8 @@ suite('find-shortcut', () => {
       checkMultiple([expectedSelf], expectedModalContextOpen);
 
   /**
+   * Register |expectedSelf| element as a listener, check that
+   * handleFindShortcut is called, then remove as a listener.
    * @param {!HTMLElement} expectedSelf
    * @param {?boolean} expectedModalContextOpen
    * @return {!Promise}
@@ -68,6 +78,23 @@ suite('find-shortcut', () => {
   const wrappedCheck = (expectedSelf, expectedModalContextOpen) => listenScope(
       expectedSelf)(() => check(expectedSelf, expectedModalContextOpen));
 
+  /**
+   * Registers for a keydown event to check whether the bubbled up event has
+   * defaultPrevented set to true, in which case the event was handled.
+   * @param {boolean} defaultPrevented
+   * @return {!Promise}
+   */
+  const listenOnceAndCheckDefaultPrevented = defaultPrevented => {
+    const resolver = new PromiseResolver();
+    const handler = e => {
+      window.removeEventListener('keydown', handler);
+      if (e.defaultPrevented == defaultPrevented)
+        resolver.resolve();
+    };
+    window.addEventListener('keydown', handler);
+    return resolver.promise;
+  };
+
   suiteSetup(() => {
     document.body.innerHTML = `
         <dom-module id="find-shortcut-element">
@@ -93,8 +120,6 @@ suite('find-shortcut', () => {
     PolymerTest.clearBody();
   });
 
-  test('no listeners are okay', () => checkMultiple([]));
-
   test('handled', () => {
     document.body.innerHTML = `<find-shortcut-element></find-shortcut-element>`;
     const testElement = document.body.querySelector('find-shortcut-element');
@@ -149,4 +174,28 @@ suite('find-shortcut', () => {
       assertThrows(() => testElement.becomeActiveFindShortcutListener());
     });
   });
+
+  test('cmd+ctrl+f bubbles up (defaultPrevented=false)', () => {
+    const bubbledUp = listenOnceAndCheckDefaultPrevented(false);
+    document.body.innerHTML = `<find-shortcut-element></find-shortcut-element>`;
+    const testElement = document.body.querySelector('find-shortcut-element');
+    return listenScope(testElement)(() => {
+      MockInteractions.pressAndReleaseKeyOn(window, 70, ['meta', 'ctrl'], 'f');
+      return bubbledUp;
+    });
+  });
+
+  test('find shortcut bubbles up (defaultPrevented=true)', () => {
+    const bubbledUp = listenOnceAndCheckDefaultPrevented(true);
+    document.body.innerHTML = `<find-shortcut-element></find-shortcut-element>`;
+    const testElement = document.body.querySelector('find-shortcut-element');
+    return Promise.all([wrappedCheck(testElement), bubbledUp]);
+  });
+
+  test('shortcut with no listeners bubbles up (defaultPrevented=false)', () => {
+    const bubbledUp = listenOnceAndCheckDefaultPrevented(false);
+    MockInteractions.pressAndReleaseKeyOn(
+        window, 70, cr.isMac ? 'meta' : 'ctrl', 'f');
+    return bubbledUp;
+  });
 });