From d01ad017e43f4b99ed85604b436b0cfb56c105c9 Mon Sep 17 00:00:00 2001
From: Kasper Nilsson <kaspern@google.com>
Date: Wed, 7 Dec 2016 18:02:34 -0800
Subject: [PATCH] Fix improper implementation of getKeyboardEvent

Bug: Issue 5088
Change-Id: Iee82c135230f78f823c25982aaeba1ea6ece1709
---
 .../keyboard-shortcut-behavior.html           |  8 +++---
 .../keyboard-shortcut-behavior_test.html      | 26 +++++++++++++++++++
 .../gr-change-list/gr-change-list.html        |  2 +-
 .../change/gr-change-view/gr-change-view.html |  2 +-
 .../change/gr-file-list/gr-file-list.html     |  2 +-
 .../core/gr-search-bar/gr-search-bar.html     |  2 +-
 .../diff/gr-diff-view/gr-diff-view.html       |  2 +-
 .../gr-selection-action-box.html              |  2 +-
 polygerrit-ui/app/elements/gr-app.html        |  2 +-
 .../elements/shared/gr-button/gr-button.html  |  2 +-
 polygerrit-ui/app/test/index.html             |  1 +
 11 files changed, 40 insertions(+), 11 deletions(-)
 rename polygerrit-ui/app/behaviors/{ => keyboard-shortcut-behavior}/keyboard-shortcut-behavior.html (81%)
 rename polygerrit-ui/app/behaviors/{ => keyboard-shortcut-behavior}/keyboard-shortcut-behavior_test.html (75%)

diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior.html b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html
similarity index 81%
rename from polygerrit-ui/app/behaviors/keyboard-shortcut-behavior.html
rename to polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html
index f2c6ab81c1..3d99cec0a9 100644
--- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior.html
+++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html
@@ -13,20 +13,22 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 See the License for the specific language governing permissions and
 limitations under the License.
 -->
-<link rel="import" href="../bower_components/polymer/polymer.html">
-<link rel="import" href="../bower_components/iron-a11y-keys-behavior/iron-a11y-keys-behavior.html">
+<link rel="import" href="../../bower_components/polymer/polymer.html">
+<link rel="import" href="../../bower_components/iron-a11y-keys-behavior/iron-a11y-keys-behavior.html">
 
 <script>
 (function(window) {
   'use strict';
 
   var getKeyboardEvent = function(e) {
-    return Polymer.dom(e.detail ? e.detail.keyboardEvent : e).event;
+    return Polymer.dom(e.detail ? e.detail.keyboardEvent : e);
   };
 
   var KeyboardShortcutBehaviorImpl = {
     modifierPressed: function(e) {
       e = getKeyboardEvent(e);
+      // When e is a keyboardEvent, e.event is not null.
+      if (e.event) { e = e.event; }
       return e.altKey || e.ctrlKey || e.metaKey || e.shiftKey;
     },
 
diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior_test.html b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html
similarity index 75%
rename from polygerrit-ui/app/behaviors/keyboard-shortcut-behavior_test.html
rename to polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html
index 457c5f25de..a72eb75356 100644
--- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior_test.html
+++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html
@@ -42,6 +42,7 @@ limitations under the License.
   suite('keyboard-shortcut-behavior tests', function() {
     var element;
     var overlay;
+    var sandbox;
 
     suiteSetup(function() {
       // Define a Polymer element that uses this behavior.
@@ -58,6 +59,11 @@ limitations under the License.
     setup(function() {
       element = fixture('basic');
       overlay = fixture('within-overlay');
+      sandbox = sinon.sandbox.create();
+    });
+
+    teardown(function() {
+      sandbox.restore();
     });
 
     test('doesn’t block kb shortcuts for non-whitelisted els', function(done) {
@@ -101,5 +107,25 @@ limitations under the License.
       MockInteractions.keyDownOn(divEl, 75, null, 'k');
     });
 
+    test('modifierPressed returns accurate values', function() {
+      var spy = sandbox.spy(element, 'modifierPressed');
+      element._handleKey = function(e) {
+        element.modifierPressed(e);
+      };
+      MockInteractions.keyDownOn(element, 75, 'shift', 'k');
+      assert.isTrue(spy.lastCall.returnValue);
+      MockInteractions.keyDownOn(element, 75, null, 'k');
+      assert.isFalse(spy.lastCall.returnValue);
+      MockInteractions.keyDownOn(element, 75, 'ctrl', 'k');
+      assert.isTrue(spy.lastCall.returnValue);
+      MockInteractions.keyDownOn(element, 75, null, 'k');
+      assert.isFalse(spy.lastCall.returnValue);
+      MockInteractions.keyDownOn(element, 75, 'meta', 'k');
+      assert.isTrue(spy.lastCall.returnValue);
+      MockInteractions.keyDownOn(element, 75, null, 'k');
+      assert.isFalse(spy.lastCall.returnValue);
+      MockInteractions.keyDownOn(element, 75, 'alt', 'k');
+      assert.isTrue(spy.lastCall.returnValue);
+    });
   });
 </script>
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.html b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.html
index bab2014f4e..bd81a484d3 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.html
@@ -15,7 +15,7 @@ limitations under the License.
 -->
 
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
-<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior.html">
+<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
 <link rel="import" href="../../../behaviors/rest-client-behavior.html">
 <link rel="import" href="../../../styles/gr-change-list-styles.html">
 <link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index 85c3fb8962..50b889ceea 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -16,7 +16,7 @@ limitations under the License.
 
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
-<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior.html">
+<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
 <link rel="import" href="../../../behaviors/rest-client-behavior.html">
 <link rel="import" href="../../shared/gr-account-link/gr-account-link.html">
 <link rel="import" href="../../shared/gr-select/gr-select.html">
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
index 6425acb7a6..7931d94940 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 -->
 
-<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior.html">
+<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
 <link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
 <link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html
index a79e8305f0..7a638106c1 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html
@@ -15,7 +15,7 @@ limitations under the License.
 -->
 
 <link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
-<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior.html">
+<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../shared/gr-autocomplete/gr-autocomplete.html">
 <link rel="import" href="../../shared/gr-button/gr-button.html">
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
index 402cd36e94..52ac524979 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
@@ -17,7 +17,7 @@ limitations under the License.
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../../bower_components/iron-dropdown/iron-dropdown.html">
 <link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
-<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior.html">
+<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
 <link rel="import" href="../../../behaviors/rest-client-behavior.html">
 <link rel="import" href="../../shared/gr-button/gr-button.html">
 <link rel="import" href="../../shared/gr-overlay/gr-overlay.html">
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.html b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.html
index 98f6b01aaf..5f74f1fde9 100644
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.html
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.html
@@ -15,7 +15,7 @@ limitations under the License.
 -->
 
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
-<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior.html">
+<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
 
 <dom-module id="gr-selection-action-box">
   <template>
diff --git a/polygerrit-ui/app/elements/gr-app.html b/polygerrit-ui/app/elements/gr-app.html
index a403e6c72d..c0c967d297 100644
--- a/polygerrit-ui/app/elements/gr-app.html
+++ b/polygerrit-ui/app/elements/gr-app.html
@@ -15,7 +15,7 @@ limitations under the License.
 -->
 
 <link rel="import" href="../bower_components/polymer/polymer.html">
-<link rel="import" href="../behaviors/keyboard-shortcut-behavior.html">
+<link rel="import" href="../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
 <link rel="import" href="../styles/app-theme.html">
 
 <link rel="import" href="./core/gr-error-manager/gr-error-manager.html">
diff --git a/polygerrit-ui/app/elements/shared/gr-button/gr-button.html b/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
index 8ca0e72c5f..c8dfea375d 100644
--- a/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
+++ b/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
@@ -16,7 +16,7 @@ limitations under the License.
 
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../../behaviors/gr-tooltip-behavior/gr-tooltip-behavior.html">
-<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior.html">
+<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
 
 <dom-module id="gr-button">
   <template strip-whitespace>
diff --git a/polygerrit-ui/app/test/index.html b/polygerrit-ui/app/test/index.html
index 48ddaf22b6..ffd3b8ed70 100644
--- a/polygerrit-ui/app/test/index.html
+++ b/polygerrit-ui/app/test/index.html
@@ -110,6 +110,7 @@ limitations under the License.
     'gr-patch-set-behavior/gr-patch-set-behavior_test.html',
     'gr-path-list-behavior/gr-path-list-behavior_test.html',
   ].forEach(function(file) {
+    // Behaviors do not utilize the DOM, so no shadow DOM test is necessary.
     file = behaviorsPath + file;
     testFiles.push(file);
   });