Skip to content

Commit

Permalink
webui: make cr-dialog capture enter key presses from any input element
Browse files Browse the repository at this point in the history
Currently, the cr-dialog key press handler only triggers an action
button click when an enter keypress event has an 'CR-INPUT' target. This
means that if a cr-input is embedded in another custom element inside
the cr-dialog, enter key presses to that cr-input  will be ignored
because the corresponding event gets retargeted to the custom element.

Instead, make the keypress handler check that the first element in the
composedPath() is an 'INPUT'.

Bug: 955859
Change-Id: I486158fadbc32b95c3c3284240bfe4ae690d78d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677597
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672635}
  • Loading branch information
kreichgauer authored and Commit Bot committed Jun 26, 2019
1 parent b1fca6e commit 6e6760f
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 9 deletions.
70 changes: 64 additions & 6 deletions chrome/test/data/webui/cr_elements/cr_dialog_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,34 +220,92 @@ suite('cr-dialog', function() {
assertTrue(clicked);
});

test('enter keys from cr-inputs (only) are processed', function() {
test('enter keys from certain inputs only are processed', function() {
document.body.innerHTML = `
<cr-dialog>
<div slot="title">title</div>
<div slot="body">
<cr-input></cr-input>
<input></input>
<input type="text"></input>
<input type="password"></input>
<input type="checkbox"></input>
<foobar></foobar>
<button class="action-button">active</button>
<cr-input></cr-input>
</div>
</cr-dialog>`;

const dialog = document.body.querySelector('cr-dialog');

const inputElement = document.body.querySelector('cr-input');
const inputElement = document.body.querySelector('input:not([type])');
const inputTextElement = document.body.querySelector('input[type="text"]');
const inputPasswordElement =
document.body.querySelector('input[type="password"]');
const inputCheckboxElement =
document.body.querySelector('input[type="checkbox"]');
const otherElement = document.body.querySelector('foobar');
const actionButton = document.body.querySelector('.action-button');
assertTrue(!!inputElement);
assertTrue(!!otherElement);
assertTrue(!!actionButton);
const crInputElement = document.body.querySelector('cr-input');

// MockInteractions triggers event listeners synchronously.
let clickedCounter = 0;
actionButton.addEventListener('click', function() {
clickedCounter++;
});

// Only certain types of <input> trigger a dialog submit.
pressEnter(otherElement);
assertEquals(0, clickedCounter);
// "type" defaults to text, which triggers the click.
pressEnter(inputElement);
assertEquals(1, clickedCounter);
pressEnter(inputTextElement);
assertEquals(2, clickedCounter);
pressEnter(inputPasswordElement);
assertEquals(3, clickedCounter);
pressEnter(inputCheckboxElement);
assertEquals(3, clickedCounter);
// Also trigger dialog submit if code synthesizes enter on a cr-input
// without targeting the underlying input.
pressEnter(crInputElement);
assertEquals(4, clickedCounter);
});

// Test that enter key presses trigger an action button click, even if the
// even was retargeted, e.g. because the input was really a cr-input, the
// cr-input was part of another custom element, etc.
test('enter keys are processed even if event was retargeted', function() {
document.body.innerHTML = `
<dom-module id="test-element">
<template><input></input></template>
</dom-module>
<cr-dialog>
<div slot="title">title</div>
<div slot="body">
<test-element></test-element>
<button class="action-button">active</button>
</div>
</cr-dialog>`;

Polymer({
is: 'test-element',
});

const dialog = document.body.querySelector('cr-dialog');

const inputWrapper = document.body.querySelector('test-element');
assertTrue(!!inputWrapper);
const inputElement = inputWrapper.shadowRoot.querySelector('input');
const actionButton = document.body.querySelector('.action-button');
assertTrue(!!inputElement);
assertTrue(!!actionButton);

// MockInteractions triggers event listeners synchronously.
let clickedCounter = 0;
actionButton.addEventListener('click', function() {
clickedCounter++;
});

pressEnter(inputElement);
assertEquals(1, clickedCounter);
Expand Down
12 changes: 9 additions & 3 deletions ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -265,11 +265,17 @@ Polymer({
return;
}

// Accept Enter keys from either the dialog, or a child input element.
if (e.target != this && e.target.tagName != 'CR-INPUT') {
// Accept Enter keys from either the dialog, or a child input or cr-input
// element (but consider that the event may have been retargeted).
const origTarget = e.composedPath()[0];
const accept = e.target == this || origTarget.tagName == 'CR-INPUT' ||
(origTarget.tagName == 'INPUT' &&
// <cr-input> can only be text-like; apply the same limit to <input> so
// that e.g. enter on a checkbox does not submit the dialog.
['text', 'password', 'number', 'search'].includes(origTarget.type));
if (!accept) {
return;
}

const actionButton =
this.querySelector('.action-button:not([disabled]):not([hidden])');
if (actionButton) {
Expand Down

0 comments on commit 6e6760f

Please sign in to comment.