From f3ce4f7c33438342c90e2c1869dc938791bf3b29 Mon Sep 17 00:00:00 2001
From: Kasper Nilsson <kaspern@google.com>
Date: Wed, 7 Jun 2017 17:07:51 -0700
Subject: [PATCH] a11y improvements for gr-file-list

- Made file list rows focusable for screen reader
- Removed enter as a file list keyboard shortcut
- Added a11ySuite to gr-file-list_test

Bug: Issue 6435
Change-Id: I4885370ffcf0e430c776cc83493e32f7fc6d2cf5
---
 .../change/gr-file-list/gr-file-list.html      |  3 ++-
 .../change/gr-file-list/gr-file-list.js        |  8 ++------
 .../change/gr-file-list/gr-file-list_test.html | 18 +++++-------------
 .../gr-keyboard-shortcuts-dialog.html          |  2 +-
 4 files changed, 10 insertions(+), 21 deletions(-)

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 81135aed87..c0371eeb29 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
@@ -246,7 +246,7 @@ limitations under the License.
           as="file"
           initial-count="[[fileListIncrement]]"
           target-framerate="1">
-        <div class="file-row row" data-path$="[[file.__path]]">
+        <div class="file-row row" data-path$="[[file.__path]]" tabindex="-1">
           <div class="reviewed" hidden$="[[!_loggedIn]]" hidden>
             <input type="checkbox" checked="[[file.isReviewed]]"
                 class="reviewed" aria-label="Reviewed checkbox">
@@ -401,6 +401,7 @@ limitations under the License.
     <gr-cursor-manager
         id="fileCursor"
         scroll-behavior="keep-visible"
+        focus-on-move
         cursor-target-class="selected"></gr-cursor-manager>
     <gr-reporting id="reporting"></gr-reporting>
   </template>
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
index 7621a9d9c3..0bcd9fd558 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -135,7 +135,7 @@
       'c': '_handleCKey',
       '[': '_handleLeftBracketKey',
       ']': '_handleRightBracketKey',
-      'o enter': '_handleEnterKey',
+      'o': '_handleOKey',
       'n': '_handleNKey',
       'p': '_handlePKey',
       'shift+a': '_handleCapitalAKey',
@@ -537,14 +537,10 @@
       this._openSelectedFile(0);
     },
 
-    _handleEnterKey(e) {
+    _handleOKey(e) {
       if (this.shouldSuppressKeyboardShortcut(e) ||
           this.modifierPressed(e)) { return; }
 
-      // Use native handling if an anchor is selected. @see Issue 5754
-      if (e.detail && e.detail.keyboardEvent && e.detail.keyboardEvent.target &&
-          e.detail.keyboardEvent.target.tagName === 'A') { return; }
-
       e.preventDefault();
       if (this._showInlineDiffs) {
         this._openCursorFile();
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
index f0f5403422..4577a05d10 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
@@ -331,9 +331,7 @@ limitations under the License.
         const showStub = sandbox.stub(page, 'show');
         assert.equal(element.$.fileCursor.index, 2);
         assert.equal(element.selectedIndex, 2);
-        MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'enter');
-        assert(showStub.lastCall.calledWith('/c/42/2/myfile.txt'),
-            'Should navigate to /c/42/2/myfile.txt');
+
         // k with a modifier should not move the cursor.
         MockInteractions.pressAndReleaseKeyOn(element, 75, 'shift', 'k');
         assert.equal(element.$.fileCursor.index, 2);
@@ -384,7 +382,7 @@ limitations under the License.
         }
       });
 
-      suite('_handleEnterKey', () => {
+      suite('_handleOKey', () => {
         let interact;
 
         setup(() => {
@@ -402,7 +400,7 @@ limitations under the License.
 
             const e = new CustomEvent('fake-keyboard-event', opt_payload);
             sinon.stub(e, 'preventDefault');
-            element._handleEnterKey(e);
+            element._handleOKey(e);
             assert.isTrue(e.preventDefault.called);
             const result = {};
             if (openCursorStub.called) {
@@ -440,14 +438,6 @@ limitations under the License.
           element._userPrefs.expand_inline_diffs = true;
           assert.deepEqual(interact(), {expanded: true});
         });
-
-        test('noop when anchor focused', () => {
-          const e = new CustomEvent('fake-keyboard-event',
-              {detail: {keyboardEvent: {target: document.createElement('a')}}});
-          sinon.stub(e, 'preventDefault');
-          element._handleEnterKey(e);
-          assert.isFalse(e.preventDefault.called);
-        });
       });
     });
 
@@ -1242,4 +1232,6 @@ limitations under the License.
       assert.isFalse(element._displayLine);
     });
   });
+
+  a11ySuite('basic');
 </script>
diff --git a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html
index 1396a2b344..fef0f87c19 100644
--- a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html
+++ b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html
@@ -231,7 +231,7 @@ limitations under the License.
             <td>Select previous file</td>
           </tr>
           <tr>
-            <td><span class="key">Enter</span> or <span class="key">o</span></td>
+            <td><span class="key">o</span></td>
             <td>Show selected file</td>
           </tr>
           <tr>