Skip to content

Commit

Permalink
fix: Make shortcuts not break writing on inputs
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@pleku @vaadin-bot
pleku authored and vaadin-bot committed Mar 25, 2021
1 parent 87bae05 commit 9a2e018
Show file tree
Hide file tree
Showing 4 changed files with 165 additions and 49 deletions.
14 changes: 11 additions & 3 deletions flow-server/src/main/java/com/vaadin/flow/component/ShortcutRegistration.java
View file
Expand Up @@ -51,10 +51,12 @@ public class ShortcutRegistration implements Registration, Serializable {
+ "const delegate=%1$s;" // the output of the JsLocator
+ "if (delegate) {"
+ "delegate.addEventListener('keydown', function(event) {"
+ "if (%2$s) {" // the filter text to match the key
+ "const new_event = new event.constructor(event.type, event);"
+ "listenOn.dispatchEvent(new_event);"
+ "event.preventDefault();" // the propagation and default actions are left for the new event
+ "event.stopPropagation();});"
+ "%3$s" // the new event allows default if desired
+ "event.stopPropagation();}" // the new event bubbles if desired
+ "});" // end matches filter
+ "} else {"
+ "throw \"Shortcut listenOn element not found with JS locator string '%1$s'\""
+ "}";//@formatter:on
Expand Down Expand Up @@ -728,8 +730,14 @@ private void setupKeydownEventDelegateIfNeeded(Component listenOn) {
final String elementLocatorJs = (String) ComponentUtil.getData(listenOn,
Shortcuts.ELEMENT_LOCATOR_JS_KEY);
if (elementLocatorJs != null) {
// #10362 only prevent default when key filter matches to not block
// typing or existing shortcuts
final String filterText = filterText();
// enable default actions if desired
final String preventDefault = allowDefaultBehavior ? ""
: "event.preventDefault();";
final String jsExpression = String.format(ELEMENT_LOCATOR_JS,
elementLocatorJs);
elementLocatorJs, filterText, preventDefault);
listenOn.getElement().executeJs(jsExpression);
}
}
Expand Down
108 changes: 76 additions & 32 deletions flow-server/src/test/java/com/vaadin/flow/component/ShortcutRegistrationTest.java
View file
Expand Up @@ -367,52 +367,96 @@ public void listenOnItemsAreChangedAfterCallingListenOnShouldNotHaveAnyEffect()

@Test
public void listenOnComponentHasElementLocatorJs_jsExecutionScheduled() {
VaadinSession session = Mockito.mock(VaadinSession.class);
Mockito.when(session.hasLock()).thenReturn(true);
UI ui = Mockito.spy(UI.class);
ui.getInternals().setSession(session);
final ElementLocatorTestFixture fixture = new ElementLocatorTestFixture();
final Key key = Key.KEY_A;
fixture.createNewShortcut(key);

Component owner = new FakeComponent();
Component initialComponentToListenOn = new FakeComponent();
List<PendingJavaScriptInvocation> pendingJavaScriptInvocations = fixture
.writeResponse();

Component[] components = new Component[] { initialComponentToListenOn };
final PendingJavaScriptInvocation js = pendingJavaScriptInvocations
.get(0);
final String expression = js.getInvocation().getExpression();
Assert.assertTrue(
"element locator string " + fixture.elementLocatorJs
+ " missing from JS execution string " + expression,
expression.contains(
"const delegate=" + fixture.elementLocatorJs + ";"));
Assert.assertTrue(
"JS execution string should have event.preventDefault() in it"
+ expression,
expression.contains("event.preventDefault();"));
Assert.assertTrue(
"JS execution string should always have event.stopPropagation() in it"
+ expression,
expression.contains("event.stopPropagation();"));
Assert.assertTrue("JS execution string missing the key" + key,
expression.contains(key.getKeys().get(0)));

ui.add(owner);
ui.add(initialComponentToListenOn);
fixture.registration.remove();

final String elementLocatorJs = "foobar";
final Registration registration = Shortcuts
.setShortcutListenOnElement(elementLocatorJs,
initialComponentToListenOn);
fixture.createNewShortcut(Key.KEY_X);

new ShortcutRegistration(owner, () -> components, event -> {
}, Key.KEY_A);
pendingJavaScriptInvocations = fixture.writeResponse();
Assert.assertEquals(0, pendingJavaScriptInvocations.size());
}

ui.getInternals().getStateTree().runExecutionsBeforeClientResponse();
@Test
public void listenOnComponentHasElementLocatorJs_allowBrowserDefault_JsExecutionDoesNotPreventDefault() {
final ElementLocatorTestFixture fixture = new ElementLocatorTestFixture();
final Key key = Key.KEY_A;
fixture.createNewShortcut(key).allowBrowserDefault();

List<PendingJavaScriptInvocation> pendingJavaScriptInvocations = ui
.getInternals().dumpPendingJavaScriptInvocations();
Assert.assertEquals(1, pendingJavaScriptInvocations.size());
List<PendingJavaScriptInvocation> pendingJavaScriptInvocations = fixture
.writeResponse();

final PendingJavaScriptInvocation js = pendingJavaScriptInvocations
.get(0);
final String expectedExecutionString = "return (function() { "
+ String.format(ShortcutRegistration.ELEMENT_LOCATOR_JS,
elementLocatorJs)
+ "}).apply($0)";
Assert.assertEquals(expectedExecutionString,
js.getInvocation().getExpression());
final String expression = js.getInvocation().getExpression();
Assert.assertFalse(
"JS execution string should NOT have event.preventDefault() in it"
+ expression,
expression.contains("event.preventDefault();"));
}

registration.remove();
class ElementLocatorTestFixture {

new ShortcutRegistration(owner, () -> components, event -> {
}, Key.KEY_A);
final Registration registration;
final Component owner;
private final String elementLocatorJs;
private final Component[] components;
private final UI ui;

ui.getInternals().getStateTree().runExecutionsBeforeClientResponse();
ElementLocatorTestFixture() {
VaadinSession session = Mockito.mock(VaadinSession.class);
Mockito.when(session.hasLock()).thenReturn(true);
ui = Mockito.spy(UI.class);
ui.getInternals().setSession(session);

owner = new FakeComponent();
Component initialComponentToListenOn = new FakeComponent();
components = new Component[] { initialComponentToListenOn };

ui.add(owner);
ui.add(initialComponentToListenOn);

elementLocatorJs = "foobar";
registration = Shortcuts.setShortcutListenOnElement(
elementLocatorJs, initialComponentToListenOn);
}

List<PendingJavaScriptInvocation> writeResponse() {
ui.getInternals().getStateTree()
.runExecutionsBeforeClientResponse();

return ui.getInternals().dumpPendingJavaScriptInvocations();
}

ShortcutRegistration createNewShortcut(Key key) {
return new ShortcutRegistration(owner, () -> components, event -> {
}, key);
}

pendingJavaScriptInvocations = ui.getInternals()
.dumpPendingJavaScriptInvocations();
Assert.assertEquals(0, pendingJavaScriptInvocations.size());
}

/**
Expand Down
34 changes: 24 additions & 10 deletions flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/DialogShortcutView.java
View file
Expand Up @@ -45,29 +45,38 @@ public class DialogShortcutView extends Div {
public static final String LISTEN_CLICK_ON_UI_BUTTON = "listen-click-on-ui-button";
public static final String LISTEN_CLICK_ON_DIALOG_BUTTON = "listen-click-on-dialog-button";
public static final String REUSABLE_DIALOG_BUTTON = "reusable-dialog-button";
public static final String ALLOW_BROWSER_DEFAULT_BUTTON = "allow-browser-default";
public static final String UI_ID = "UI-ID";
public static final String CONTENT_ID = "CONTENT";
public static final String KEY_STRING = "x";
public static final Key SHORTCUT_KEY = Key.KEY_X;
public static final int REUSABLE_DIALOG_ID = 999;

private AtomicInteger dialogCounter = new AtomicInteger(-1);
private final AtomicInteger dialogCounter = new AtomicInteger(-1);
private int eventCounter;
private final Div eventLog;
private boolean allowBrowserDefault;

private Dialog reusedDialog;

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

final NativeButton allowBrowserDefaultButton = new NativeButton(
"Allow Browser Default", event -> {
allowBrowserDefault = true;
event.getSource().setEnabled(false);
});
allowBrowserDefaultButton.setId(ALLOW_BROWSER_DEFAULT_BUTTON);
final NativeButton testButton = createButton(
"UI level button with shortcut", this::logClickEvent);
testButton.setId(UI_BUTTON);
testButton.addClickShortcut(SHORTCUT_KEY);

add(new Div(new Text("Shortcut key: "
+ SHORTCUT_KEY.getKeys().stream().findFirst().orElse(""))),
createOpenDialogButton(OPEN_BUTTON), testButton, eventLog);
add(new Div(new Text("Shortcut key: " + KEY_STRING)),
createOpenDialogButton(OPEN_BUTTON), testButton,
allowBrowserDefaultButton, eventLog);
setId("main-div");

final NativeButton reusableDialogButton = new NativeButton(
Expand Down Expand Up @@ -139,17 +148,22 @@ public Dialog(int index) {
.setId(LISTEN_CLICK_ON_DIALOG_BUTTON + index);
final NativeButton uiScopeShortcutButton = new NativeButton(
"Add shortcut with listenOn(UI) (default)", event -> {
Shortcuts.addShortcutListener(Dialog.this,
DialogShortcutView.this::logClickEvent,
SHORTCUT_KEY);
Shortcuts
.addShortcutListener(Dialog.this,
DialogShortcutView.this::logClickEvent,
SHORTCUT_KEY)
.setBrowserDefaultAllowed(allowBrowserDefault);
event.getSource().setEnabled(false);
});
uiScopeShortcutButton.setId(LISTEN_ON_UI_BUTTON + index);
final NativeButton dialogScopeShortcutButton = new NativeButton(
"Add shortcut with listenOn(Dialog)", event -> {
Shortcuts.addShortcutListener(Dialog.this,
DialogShortcutView.this::logClickEvent,
SHORTCUT_KEY).listenOn(Dialog.this);
Shortcuts
.addShortcutListener(Dialog.this,
DialogShortcutView.this::logClickEvent,
SHORTCUT_KEY)
.listenOn(Dialog.this)
.setBrowserDefaultAllowed(allowBrowserDefault);
event.getSource().setEnabled(false);
});
dialogScopeShortcutButton.setId(LISTEN_ON_DIALOG_BUTTON + index);
Expand Down
58 changes: 54 additions & 4 deletions flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/DialogShortcutIT.java
View file
Expand Up @@ -3,6 +3,7 @@
import java.util.concurrent.atomic.AtomicInteger;

import com.vaadin.flow.component.html.testbench.DivElement;
import com.vaadin.flow.component.html.testbench.InputTextElement;
import com.vaadin.flow.component.html.testbench.NativeButtonElement;
import com.vaadin.flow.testutil.ChromeBrowserTest;
import com.vaadin.testbench.TestBenchElement;
Expand Down Expand Up @@ -123,6 +124,53 @@ public void twoDialogsOpenedWithSameShortcutKeyOnListenOn_dialogWithFocusExecute
validateLatestShortcutEvent(3, DialogShortcutView.UI_BUTTON);
}

// #10362
@Test
public void shortcutAddedWithPreventDefault_inputFocused_enteringOtherKeysToInputWorks() {
final int firstDialogIndex = openNewDialog();
listenToShortcutOnDialog(firstDialogIndex);

final InputTextElement dialogInput = getDialogInput(firstDialogIndex);
pressShortcutKey(dialogInput);
validateLatestDialogShortcut(0, firstDialogIndex);
Assert.assertNotEquals(
"Entered shortcut key should not be visible in input due to prevent default",
DialogShortcutView.KEY_STRING, dialogInput.getValue());

// use another key
dialogInput.focus();
dialogInput.sendKeys("fooxbar");
// only x triggers event and value changes
validateLatestDialogShortcut(1, firstDialogIndex);
Assert.assertEquals("Entered value should be visible in input",
"foobar", dialogInput.getValue());
}

// #10362
@Test
public void shortcutAddedWithAllowDefault_inputFocused_allKeysAcceptedToInput() {
$(NativeButtonElement.class)
.id(DialogShortcutView.ALLOW_BROWSER_DEFAULT_BUTTON).click();
final int firstDialogIndex = openNewDialog();
listenToShortcutOnDialog(firstDialogIndex);

final InputTextElement dialogInput = getDialogInput(firstDialogIndex);
pressShortcutKey(dialogInput);
validateLatestDialogShortcut(0, firstDialogIndex);
Assert.assertEquals(
"Entered shortcut key should be visible in input due to allow default",
DialogShortcutView.KEY_STRING, dialogInput.getValue());
dialogInput.clear();

dialogInput.focus();
dialogInput.sendKeys("foo" + DialogShortcutView.KEY_STRING + "bar");
// only x triggers event and value changes
validateLatestDialogShortcut(1, firstDialogIndex);
Assert.assertEquals("Entered value should be visible in input",
"foo" + DialogShortcutView.KEY_STRING + "bar",
dialogInput.getValue());
}

protected void openReusedDialog() {
findElement(By.id(DialogShortcutView.REUSABLE_DIALOG_BUTTON)).click();
}
Expand Down Expand Up @@ -153,7 +201,9 @@ private void validateShortcutEvent(int indexFromTop, int eventCounter,
String eventSourceId) {
final WebElement latestEvent = eventLog.findElements(By.tagName("div"))
.get(indexFromTop);
Assert.assertEquals("Invalid latest event",
Assert.assertEquals(
"Invalid latest event with " + indexFromTop + ":" + ":"
+ eventSourceId,
eventCounter + "-" + eventSourceId, latestEvent.getText());
}

Expand All @@ -162,10 +212,10 @@ protected void pressShortcutKey(TestBenchElement elementToFocus) {
elementToFocus.sendKeys("x");
}

protected TestBenchElement getDialogInput(int dialogIndex) {
protected InputTextElement getDialogInput(int dialogIndex) {
return $(DivElement.class)
.id(DialogShortcutView.CONTENT_ID + dialogIndex).$("input")
.first();
.id(DialogShortcutView.CONTENT_ID + dialogIndex)
.$(InputTextElement.class).first();
}

private void listenToShortcutOnUI(int dialogIndex) {
Expand Down

0 comments on commit 9a2e018

Please sign in to comment.