From 165d5d33b5e64cb36ec0bb6a031c14554897d841 Mon Sep 17 00:00:00 2001 From: Wyatt Allen Date: Tue, 7 Jun 2016 16:23:29 -0700 Subject: [PATCH] Restores a11y tabbing behavior to gr-reply-dialog The iron-overlay-behavior tries to prevent the user from tabbing outside of the overlay by recording the first and last child elements which appear to be focusable. It intercepts [Tab] and [Shift+Tab] keystrokes to wrap the tab-order of its controls. However, the way it determines the first and last such element is 1) not composable with child elements that are Polymer components and 2) not composable with dynamically changing children. Because gr-reply-dialog is both 1) built with Polymer components and 2) partially dynamically generated, the tab order is broken when the overlay behavior tries to intercept and constrain it. The workaround here is to remove the iron-overlay-behavior's guesswork and manually set the first and last focusable inputs with knowledge of what those elements would be in gr-reply-dialog. gr-overlay's #open method now returns a promise that resolves when the overlay is actually displayed. Bug: Issue 4165 Change-Id: Ib1cdea4c5e99f821db699c89743240caab751f03 --- .../change/gr-change-view/gr-change-view.js | 14 +++++--- .../gr-reply-dialog/gr-reply-dialog.html | 5 ++- .../change/gr-reply-dialog/gr-reply-dialog.js | 7 ++++ .../elements/shared/gr-overlay/gr-overlay.js | 36 +++++++++++++++++-- 4 files changed, 55 insertions(+), 7 deletions(-) diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js index 347352a4e2..e6d1a64512 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js @@ -268,7 +268,7 @@ _handleReplyTap: function(e) { e.preventDefault(); - this.$.replyOverlay.open(); + this._openReplyDialog(); }, _handleDownloadTap: function(e) { @@ -285,7 +285,7 @@ var quoteStr = msg.split('\n').map( function(line) { return '> ' + line; }).join('\n') + '\n\n'; this.$.replyDialog.draft += quoteStr; - this.$.replyOverlay.open(); + this._openReplyDialog(); }, _handleReplyOverlayOpen: function(e) { @@ -350,7 +350,7 @@ if (!loggedIn) { return; } if (this.viewState.showReplyDialog) { - this.$.replyOverlay.open(); + this._openReplyDialog(); this.async(function() { this.$.replyOverlay.center(); }, 1); this.set('viewState.showReplyDialog', false); } @@ -471,7 +471,7 @@ if (!this._loggedIn) { return; } e.preventDefault(); - this.$.replyOverlay.open(); + this._openReplyDialog(); break; case 85: // 'u' e.preventDefault(); @@ -480,6 +480,12 @@ } }, + _openReplyDialog: function() { + this.$.replyOverlay.open().then(function() { + this.$.replyOverlay.setFocusStops(this.$.replyDialog.getFocusStops()); + }.bind(this)); + }, + _handleReloadChange: function() { page.show(this.changePath(this._changeNum)); }, diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html index 7fb998b65b..a364c79c9d 100644 --- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html +++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html @@ -148,7 +148,10 @@ limitations under the License.
Send - Cancel + Cancel
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js index c89cb93288..a70289960c 100644 --- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js +++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js @@ -65,6 +65,13 @@ }.bind(this)); }, + getFocusStops: function() { + return { + start: this.$.textarea.$.textarea, + end: this.$.cancelButton, + }; + }, + _computeShowLabels: function(patchNum, revisions) { var num = parseInt(patchNum, 10); for (var rev in revisions) { diff --git a/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.js b/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.js index 5fa33eabab..fa32039e93 100644 --- a/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.js +++ b/polygerrit-ui/app/elements/shared/gr-overlay/gr-overlay.js @@ -14,6 +14,9 @@ (function() { 'use strict'; + var AWAIT_MAX_ITERS = 10; + var AWAIT_STEP = 5; + Polymer({ is: 'gr-overlay', @@ -27,8 +30,11 @@ }, open: function() { - Gerrit.KeyboardShortcutBehavior.enabled = false; - Polymer.IronOverlayBehaviorImpl.open.apply(this, arguments); + return new Promise(function(resolve) { + Gerrit.KeyboardShortcutBehavior.enabled = false; + Polymer.IronOverlayBehaviorImpl.open.apply(this, arguments); + this._awaitOpen(resolve); + }.bind(this)); }, close: function() { @@ -40,5 +46,31 @@ Gerrit.KeyboardShortcutBehavior.enabled = true; Polymer.IronOverlayBehaviorImpl.cancel.apply(this, arguments); }, + + /** + * Override the focus stops that iron-overlay-behavior tries to find. + */ + setFocusStops: function(stops) { + this.__firstFocusableNode = stops.start; + this.__lastFocusableNode = stops.end; + }, + + /** + * NOTE: (wyatta) Slightly hacky way to listen to the overlay actually + * opening. Eventually replace with a direct way to listen to the overlay. + */ + _awaitOpen: function(fn) { + var iters = 0; + function step() { + this.async(function() { + if (this.style.display !== 'none') { + fn.call(this); + } else if (iters++ < AWAIT_MAX_ITERS) { + step.call(this); + } + }.bind(this), AWAIT_STEP); + }; + step.call(this); + }, }); })();