Commit f58842dd authored by Kyle Horimoto's avatar Kyle Horimoto Committed by Commit Bot

[CrOS Network] Fix InternetDetailPage keyboard navigation issue.

Before this CL, tabbing to the subpage arrow of a network list item and
pressing Enter would cause two "show-detail" events to be fired instead
of one. In the settings page, the outcome of this error was that users
would be navigated to the details page but would have to click the back
arrow twice to go to the previous page.

This issue was caused by the use of <iron-a11y-keys>; this element
handled the "keydown" event by dispatching its own "keys-pressed" event,
which was handled by <cr-network-list-item> to navigate to the next
page. <cr-network-list-item> stopped propagation of the event, but this
was not good enough, since the event it was stopping was the
"keys-pressed" event, not the "keydown" event. Since the default action
of pressing Enter when a button is focused is to simulate a click, this
still occurred and caused the subpage arrow's click handler to be
called.

Additionally, a second issue was caused by the interaction of
<iron-a11y-keys> and <iron-list>. Since both of these handle the
"keydown" event and <cr-network-list-item> only handles the
"keys-pressed" event, the <cr-network-list-item> could not stop
propagation onf the original "keydown" event, resulting in the
<iron-list>'s handler also being invoked.

This CL fixes these issues by:
(1) Removing use of the <iron-a11y-keys> element and replacing it with
    explicit "keydown" listeners via this.listen().
(2) Stopping the propagation of the "keydown" event instead of of the
    "keys-pressed" event.
(3) Preventing the default action of the "keydown" event.

Bug: 736963
Change-Id: Ibb1c8c0484a1bc243586ebdee95fe1e3e357b94a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1540676Reviewed-by: default avatarSteven Bennetts <stevenjb@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#644880}
parent ba9dcdc4
...@@ -7,7 +7,6 @@ ...@@ -7,7 +7,6 @@
<link rel="import" href="../../policy/cr_policy_network_behavior.html"> <link rel="import" href="../../policy/cr_policy_network_behavior.html">
<link rel="import" href="../../shared_style_css.html"> <link rel="import" href="../../shared_style_css.html">
<link rel="import" href="../../shared_vars_css.html"> <link rel="import" href="../../shared_vars_css.html">
<link rel="import" href="chrome://resources/polymer/v1_0/iron-a11y-keys/iron-a11y-keys.html">
<link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout-classes.html"> <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout-classes.html">
<link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button-light.html"> <link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button-light.html">
...@@ -81,13 +80,10 @@ ...@@ -81,13 +80,10 @@
</cr-policy-indicator> </cr-policy-indicator>
</template> </template>
<template is="dom-if" <template is="dom-if"
if="[[isSubpageButtonVisible_(networkState, showButtons)]]"> if="[[isSubpageButtonVisible_(networkState, showButtons)]]" restamp>
<!-- iron-list captures 'enter' so handle it here explicitly. -->
<iron-a11y-keys keys="enter" on-keys-pressed="fireShowDetails_">
</iron-a11y-keys>
<paper-icon-button-light class="subpage-arrow"> <paper-icon-button-light class="subpage-arrow">
<button on-tap="fireShowDetails_" tabindex$="[[tabindex]]" <button id="subpage-button" on-click="onSubpageArrowClick_"
aria-label$="[[ariaLabel]]"> tabindex$="[[tabindex]]" aria-label$="[[ariaLabel]]">
</button> </button>
</paper-icon-button-light> </paper-icon-button-light>
</template> </template>
......
...@@ -63,6 +63,16 @@ Polymer({ ...@@ -63,6 +63,16 @@ Polymer({
behaviors: [CrPolicyNetworkBehavior], behaviors: [CrPolicyNetworkBehavior],
/** @override */
attached: function() {
this.listen(this, 'keydown', 'onKeydown_');
},
/** @override */
detached: function() {
this.unlisten(this, 'keydown', 'onKeydown_');
},
/** @private */ /** @private */
itemChanged_: function() { itemChanged_: function() {
if (this.item && !this.item.hasOwnProperty('customItemName')) { if (this.item && !this.item.hasOwnProperty('customItemName')) {
...@@ -142,7 +152,7 @@ Polymer({ ...@@ -142,7 +152,7 @@ Polymer({
}, },
/** /**
* @param {!CrOnc.NetworkStateProperties} networkState * @param {!CrOnc.NetworkStateProperties|undefined} networkState
* @param {boolean} showButtons * @param {boolean} showButtons
* @return {boolean} * @return {boolean}
* @private * @private
...@@ -160,9 +170,39 @@ Polymer({ ...@@ -160,9 +170,39 @@ Polymer({
this.networkState.ConnectionState == CrOnc.ConnectionState.CONNECTED; this.networkState.ConnectionState == CrOnc.ConnectionState.CONNECTED;
}, },
/**
* @param {!KeyboardEvent} event
* @private
*/
onKeydown_: function(event) {
// The only key event handled by this element is pressing Enter when the
// subpage arrow is focused.
if (event.key != 'Enter' ||
!this.isSubpageButtonVisible_(this.networkState, this.showButtons) ||
this.$$('#subpage-button') != this.shadowRoot.activeElement) {
return;
}
this.fireShowDetails_(event);
// The default event for pressing Enter on a focused button is to simulate a
// click on the button. Prevent this action, since it would navigate a
// second time to the details page and cause an unnecessary entry to be
// added to the back stack. See https://crbug.com/736963.
event.preventDefault();
},
/**
* @param {!MouseEvent} event
* @private
*/
onSubpageArrowClick_: function(event) {
this.fireShowDetails_(event);
},
/** /**
* Fires a 'show-details' event with |this.networkState| as the details. * Fires a 'show-details' event with |this.networkState| as the details.
* @param {Event} event * @param {!Event} event
* @private * @private
*/ */
fireShowDetails_: function(event) { fireShowDetails_: function(event) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment