Skip to content

Commit a777672

Browse files
Pekka Hyvönenvaadin-bot
Pekka Hyvönen
authored and
vaadin-bot
committed
fix: Make shortcuts not break writing on inputs
Only passes the delegated event forward when it maps to the shortcut key. For the shortcut key, preventDefault() is the default behavior and default can be allowed with allowBrowserDefault() in the ShortcutRegistration. Fixes #10362
1 parent 796d98b commit a777672

4 files changed

Lines changed: 165 additions & 49 deletions

File tree

‎flow-server/src/main/java/com/vaadin/flow/component/ShortcutRegistration.java‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,12 @@ public class ShortcutRegistration implements Registration, Serializable {
5151
+ "const delegate=%1$s;" // the output of the JsLocator
5252
+ "if (delegate) {"
5353
+ "delegate.addEventListener('keydown', function(event) {"
54+
+ "if (%2$s) {" // the filter text to match the key
5455
+ "const new_event = new event.constructor(event.type, event);"
5556
+ "listenOn.dispatchEvent(new_event);"
56-
+ "event.preventDefault();" // the propagation and default actions are left for the new event
57-
+ "event.stopPropagation();});"
57+
+ "%3$s" // the new event allows default if desired
58+
+ "event.stopPropagation();}" // the new event bubbles if desired
59+
+ "});" // end matches filter
5860
+ "} else {"
5961
+ "throw \"Shortcut listenOn element not found with JS locator string '%1$s'\""
6062
+ "}";//@formatter:on
@@ -766,8 +768,14 @@ private void setupKeydownEventDelegateIfNeeded(Component listenOn) {
766768
final String elementLocatorJs = (String) ComponentUtil.getData(listenOn,
767769
Shortcuts.ELEMENT_LOCATOR_JS_KEY);
768770
if (elementLocatorJs != null) {
771+
// #10362 only prevent default when key filter matches to not block
772+
// typing or existing shortcuts
773+
final String filterText = filterText();
774+
// enable default actions if desired
775+
final String preventDefault = allowDefaultBehavior ? ""
776+
: "event.preventDefault();";
769777
final String jsExpression = String.format(ELEMENT_LOCATOR_JS,
770-
elementLocatorJs);
778+
elementLocatorJs, filterText, preventDefault);
771779
listenOn.getElement().executeJs(jsExpression);
772780
}
773781
}

‎flow-server/src/test/java/com/vaadin/flow/component/ShortcutRegistrationTest.java‎

Lines changed: 76 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -368,52 +368,96 @@ public void listenOnItemsAreChangedAfterCallingListenOnShouldNotHaveAnyEffect()
368368

369369
@Test
370370
public void listenOnComponentHasElementLocatorJs_jsExecutionScheduled() {
371-
VaadinSession session = Mockito.mock(VaadinSession.class);
372-
Mockito.when(session.hasLock()).thenReturn(true);
373-
UI ui = Mockito.spy(UI.class);
374-
ui.getInternals().setSession(session);
371+
final ElementLocatorTestFixture fixture = new ElementLocatorTestFixture();
372+
final Key key = Key.KEY_A;
373+
fixture.createNewShortcut(key);
375374

376-
Component owner = new FakeComponent();
377-
Component initialComponentToListenOn = new FakeComponent();
375+
List<PendingJavaScriptInvocation> pendingJavaScriptInvocations = fixture
376+
.writeResponse();
378377

379-
Component[] components = new Component[] { initialComponentToListenOn };
378+
final PendingJavaScriptInvocation js = pendingJavaScriptInvocations
379+
.get(0);
380+
final String expression = js.getInvocation().getExpression();
381+
Assert.assertTrue(
382+
"element locator string " + fixture.elementLocatorJs
383+
+ " missing from JS execution string " + expression,
384+
expression.contains(
385+
"const delegate=" + fixture.elementLocatorJs + ";"));
386+
Assert.assertTrue(
387+
"JS execution string should have event.preventDefault() in it"
388+
+ expression,
389+
expression.contains("event.preventDefault();"));
390+
Assert.assertTrue(
391+
"JS execution string should always have event.stopPropagation() in it"
392+
+ expression,
393+
expression.contains("event.stopPropagation();"));
394+
Assert.assertTrue("JS execution string missing the key" + key,
395+
expression.contains(key.getKeys().get(0)));
380396

381-
ui.add(owner);
382-
ui.add(initialComponentToListenOn);
397+
fixture.registration.remove();
383398

384-
final String elementLocatorJs = "foobar";
385-
final Registration registration = Shortcuts
386-
.setShortcutListenOnElement(elementLocatorJs,
387-
initialComponentToListenOn);
399+
fixture.createNewShortcut(Key.KEY_X);
388400

389-
new ShortcutRegistration(owner, () -> components, event -> {
390-
}, Key.KEY_A);
401+
pendingJavaScriptInvocations = fixture.writeResponse();
402+
Assert.assertEquals(0, pendingJavaScriptInvocations.size());
403+
}
391404

392-
ui.getInternals().getStateTree().runExecutionsBeforeClientResponse();
405+
@Test
406+
public void listenOnComponentHasElementLocatorJs_allowBrowserDefault_JsExecutionDoesNotPreventDefault() {
407+
final ElementLocatorTestFixture fixture = new ElementLocatorTestFixture();
408+
final Key key = Key.KEY_A;
409+
fixture.createNewShortcut(key).allowBrowserDefault();
393410

394-
List<PendingJavaScriptInvocation> pendingJavaScriptInvocations = ui
395-
.getInternals().dumpPendingJavaScriptInvocations();
396-
Assert.assertEquals(1, pendingJavaScriptInvocations.size());
411+
List<PendingJavaScriptInvocation> pendingJavaScriptInvocations = fixture
412+
.writeResponse();
397413

398414
final PendingJavaScriptInvocation js = pendingJavaScriptInvocations
399415
.get(0);
400-
final String expectedExecutionString = "return (function() { "
401-
+ String.format(ShortcutRegistration.ELEMENT_LOCATOR_JS,
402-
elementLocatorJs)
403-
+ "}).apply($0)";
404-
Assert.assertEquals(expectedExecutionString,
405-
js.getInvocation().getExpression());
416+
final String expression = js.getInvocation().getExpression();
417+
Assert.assertFalse(
418+
"JS execution string should NOT have event.preventDefault() in it"
419+
+ expression,
420+
expression.contains("event.preventDefault();"));
421+
}
406422

407-
registration.remove();
423+
class ElementLocatorTestFixture {
408424

409-
new ShortcutRegistration(owner, () -> components, event -> {
410-
}, Key.KEY_A);
425+
final Registration registration;
426+
final Component owner;
427+
private final String elementLocatorJs;
428+
private final Component[] components;
429+
private final UI ui;
411430

412-
ui.getInternals().getStateTree().runExecutionsBeforeClientResponse();
431+
ElementLocatorTestFixture() {
432+
VaadinSession session = Mockito.mock(VaadinSession.class);
433+
Mockito.when(session.hasLock()).thenReturn(true);
434+
ui = Mockito.spy(UI.class);
435+
ui.getInternals().setSession(session);
436+
437+
owner = new FakeComponent();
438+
Component initialComponentToListenOn = new FakeComponent();
439+
components = new Component[] { initialComponentToListenOn };
440+
441+
ui.add(owner);
442+
ui.add(initialComponentToListenOn);
443+
444+
elementLocatorJs = "foobar";
445+
registration = Shortcuts.setShortcutListenOnElement(
446+
elementLocatorJs, initialComponentToListenOn);
447+
}
448+
449+
List<PendingJavaScriptInvocation> writeResponse() {
450+
ui.getInternals().getStateTree()
451+
.runExecutionsBeforeClientResponse();
452+
453+
return ui.getInternals().dumpPendingJavaScriptInvocations();
454+
}
455+
456+
ShortcutRegistration createNewShortcut(Key key) {
457+
return new ShortcutRegistration(owner, () -> components, event -> {
458+
}, key);
459+
}
413460

414-
pendingJavaScriptInvocations = ui.getInternals()
415-
.dumpPendingJavaScriptInvocations();
416-
Assert.assertEquals(0, pendingJavaScriptInvocations.size());
417461
}
418462

419463
/**

‎flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/DialogShortcutView.java‎

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -45,29 +45,38 @@ public class DialogShortcutView extends Div {
4545
public static final String LISTEN_CLICK_ON_UI_BUTTON = "listen-click-on-ui-button";
4646
public static final String LISTEN_CLICK_ON_DIALOG_BUTTON = "listen-click-on-dialog-button";
4747
public static final String REUSABLE_DIALOG_BUTTON = "reusable-dialog-button";
48+
public static final String ALLOW_BROWSER_DEFAULT_BUTTON = "allow-browser-default";
4849
public static final String UI_ID = "UI-ID";
4950
public static final String CONTENT_ID = "CONTENT";
51+
public static final String KEY_STRING = "x";
5052
public static final Key SHORTCUT_KEY = Key.KEY_X;
5153
public static final int REUSABLE_DIALOG_ID = 999;
5254

53-
private AtomicInteger dialogCounter = new AtomicInteger(-1);
55+
private final AtomicInteger dialogCounter = new AtomicInteger(-1);
5456
private int eventCounter;
5557
private final Div eventLog;
58+
private boolean allowBrowserDefault;
5659

5760
private Dialog reusedDialog;
5861

5962
public DialogShortcutView() {
6063
eventLog = new Div(new Text("Click events and their sources:"));
6164
eventLog.setId(EVENT_LOG_ID);
6265

66+
final NativeButton allowBrowserDefaultButton = new NativeButton(
67+
"Allow Browser Default", event -> {
68+
allowBrowserDefault = true;
69+
event.getSource().setEnabled(false);
70+
});
71+
allowBrowserDefaultButton.setId(ALLOW_BROWSER_DEFAULT_BUTTON);
6372
final NativeButton testButton = createButton(
6473
"UI level button with shortcut", this::logClickEvent);
6574
testButton.setId(UI_BUTTON);
6675
testButton.addClickShortcut(SHORTCUT_KEY);
6776

68-
add(new Div(new Text("Shortcut key: "
69-
+ SHORTCUT_KEY.getKeys().stream().findFirst().orElse(""))),
70-
createOpenDialogButton(OPEN_BUTTON), testButton, eventLog);
77+
add(new Div(new Text("Shortcut key: " + KEY_STRING)),
78+
createOpenDialogButton(OPEN_BUTTON), testButton,
79+
allowBrowserDefaultButton, eventLog);
7180
setId("main-div");
7281

7382
final NativeButton reusableDialogButton = new NativeButton(
@@ -139,17 +148,22 @@ public Dialog(int index) {
139148
.setId(LISTEN_CLICK_ON_DIALOG_BUTTON + index);
140149
final NativeButton uiScopeShortcutButton = new NativeButton(
141150
"Add shortcut with listenOn(UI) (default)", event -> {
142-
Shortcuts.addShortcutListener(Dialog.this,
143-
DialogShortcutView.this::logClickEvent,
144-
SHORTCUT_KEY);
151+
Shortcuts
152+
.addShortcutListener(Dialog.this,
153+
DialogShortcutView.this::logClickEvent,
154+
SHORTCUT_KEY)
155+
.setBrowserDefaultAllowed(allowBrowserDefault);
145156
event.getSource().setEnabled(false);
146157
});
147158
uiScopeShortcutButton.setId(LISTEN_ON_UI_BUTTON + index);
148159
final NativeButton dialogScopeShortcutButton = new NativeButton(
149160
"Add shortcut with listenOn(Dialog)", event -> {
150-
Shortcuts.addShortcutListener(Dialog.this,
151-
DialogShortcutView.this::logClickEvent,
152-
SHORTCUT_KEY).listenOn(Dialog.this);
161+
Shortcuts
162+
.addShortcutListener(Dialog.this,
163+
DialogShortcutView.this::logClickEvent,
164+
SHORTCUT_KEY)
165+
.listenOn(Dialog.this)
166+
.setBrowserDefaultAllowed(allowBrowserDefault);
153167
event.getSource().setEnabled(false);
154168
});
155169
dialogScopeShortcutButton.setId(LISTEN_ON_DIALOG_BUTTON + index);

‎flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/DialogShortcutIT.java‎

Lines changed: 54 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import java.util.concurrent.atomic.AtomicInteger;
44

55
import com.vaadin.flow.component.html.testbench.DivElement;
6+
import com.vaadin.flow.component.html.testbench.InputTextElement;
67
import com.vaadin.flow.component.html.testbench.NativeButtonElement;
78
import com.vaadin.flow.testutil.ChromeBrowserTest;
89
import com.vaadin.testbench.TestBenchElement;
@@ -124,6 +125,53 @@ public void twoDialogsOpenedWithSameShortcutKeyOnListenOn_dialogWithFocusExecute
124125
validateLatestShortcutEvent(3, DialogShortcutView.UI_BUTTON);
125126
}
126127

128+
// #10362
129+
@Test
130+
public void shortcutAddedWithPreventDefault_inputFocused_enteringOtherKeysToInputWorks() {
131+
final int firstDialogIndex = openNewDialog();
132+
listenToShortcutOnDialog(firstDialogIndex);
133+
134+
final InputTextElement dialogInput = getDialogInput(firstDialogIndex);
135+
pressShortcutKey(dialogInput);
136+
validateLatestDialogShortcut(0, firstDialogIndex);
137+
Assert.assertNotEquals(
138+
"Entered shortcut key should not be visible in input due to prevent default",
139+
DialogShortcutView.KEY_STRING, dialogInput.getValue());
140+
141+
// use another key
142+
dialogInput.focus();
143+
dialogInput.sendKeys("fooxbar");
144+
// only x triggers event and value changes
145+
validateLatestDialogShortcut(1, firstDialogIndex);
146+
Assert.assertEquals("Entered value should be visible in input",
147+
"foobar", dialogInput.getValue());
148+
}
149+
150+
// #10362
151+
@Test
152+
public void shortcutAddedWithAllowDefault_inputFocused_allKeysAcceptedToInput() {
153+
$(NativeButtonElement.class)
154+
.id(DialogShortcutView.ALLOW_BROWSER_DEFAULT_BUTTON).click();
155+
final int firstDialogIndex = openNewDialog();
156+
listenToShortcutOnDialog(firstDialogIndex);
157+
158+
final InputTextElement dialogInput = getDialogInput(firstDialogIndex);
159+
pressShortcutKey(dialogInput);
160+
validateLatestDialogShortcut(0, firstDialogIndex);
161+
Assert.assertEquals(
162+
"Entered shortcut key should be visible in input due to allow default",
163+
DialogShortcutView.KEY_STRING, dialogInput.getValue());
164+
dialogInput.clear();
165+
166+
dialogInput.focus();
167+
dialogInput.sendKeys("foo" + DialogShortcutView.KEY_STRING + "bar");
168+
// only x triggers event and value changes
169+
validateLatestDialogShortcut(1, firstDialogIndex);
170+
Assert.assertEquals("Entered value should be visible in input",
171+
"foo" + DialogShortcutView.KEY_STRING + "bar",
172+
dialogInput.getValue());
173+
}
174+
127175
protected void openReusedDialog() {
128176
findElement(By.id(DialogShortcutView.REUSABLE_DIALOG_BUTTON)).click();
129177
}
@@ -154,7 +202,9 @@ private void validateShortcutEvent(int indexFromTop, int eventCounter,
154202
String eventSourceId) {
155203
final WebElement latestEvent = eventLog.findElements(By.tagName("div"))
156204
.get(indexFromTop);
157-
Assert.assertEquals("Invalid latest event",
205+
Assert.assertEquals(
206+
"Invalid latest event with " + indexFromTop + ":" + ":"
207+
+ eventSourceId,
158208
eventCounter + "-" + eventSourceId, latestEvent.getText());
159209
}
160210

@@ -163,10 +213,10 @@ protected void pressShortcutKey(TestBenchElement elementToFocus) {
163213
elementToFocus.sendKeys("x");
164214
}
165215

166-
protected TestBenchElement getDialogInput(int dialogIndex) {
216+
protected InputTextElement getDialogInput(int dialogIndex) {
167217
return $(DivElement.class)
168-
.id(DialogShortcutView.CONTENT_ID + dialogIndex).$("input")
169-
.first();
218+
.id(DialogShortcutView.CONTENT_ID + dialogIndex)
219+
.$(InputTextElement.class).first();
170220
}
171221

172222
private void listenToShortcutOnUI(int dialogIndex) {

0 commit comments

Comments
 (0)