Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: set shortcut listener to modal only if owner is child #13272

Merged
merged 2 commits into from
Mar 11, 2022
+81 −18
Merged

fix: set shortcut listener to modal only if owner is child #13272

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4154d14
fix: set shortcut listener to modal only if owner is child
Mar 10, 2022
68c17cd
Add assert, fix comment
Mar 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 27 additions & 11 deletions flow-server/src/main/java/com/vaadin/flow/component/ShortcutRegistration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,16 +532,7 @@ private String filterText() {
}

private void updateHandlerListenerRegistration(int listenOnIndex) {
Component component = listenOnComponents[listenOnIndex];
assert component != null;

if (component instanceof UI) {
UIInternals uiInternals = ((UI) component).getInternals();
if (uiInternals.hasModalComponent()) {
component = uiInternals.getActiveModalComponent();
}
}
final Component source = component;
final Component component = getComponentEventSource(listenOnIndex);

if (shortcutListenerRegistrations == null) {
shortcutListenerRegistrations = new CompoundRegistration[listenOnComponents.length];
Expand All @@ -552,7 +543,8 @@ private void updateHandlerListenerRegistration(int listenOnIndex) {
shortcutListenerRegistrations[listenOnIndex] = new CompoundRegistration();
Registration keyDownRegistration = ComponentUtil.addListener(
component, KeyDownEvent.class,
event -> fireShortcutEvent(source), domRegistration -> {
event -> fireShortcutEvent(component),
domRegistration -> {
shortcutListenerRegistrations[listenOnIndex]
.addRegistration(domRegistration);
configureHandlerListenerRegistration(listenOnIndex);
Expand All @@ -566,6 +558,30 @@ private void updateHandlerListenerRegistration(int listenOnIndex) {
}
}

private Component getComponentEventSource(int listenOnIndex) {
Component component = listenOnComponents[listenOnIndex];
assert component != null;

if (component instanceof UI) {
UIInternals uiInternals = ((UI) component).getInternals();
// Shortcut events go to UI by default, which will be inert from
// receiving RPCs if there is an active modal component. In this
// case, check if the owner component is a child of the modal
// component, and if so, send via the modal component.
if (uiInternals.hasModalComponent()) {
Optional<Component> maybeAncestor = Optional.of(lifecycleOwner);
while (maybeAncestor.isPresent()) {
Component ancestor = maybeAncestor.get();
if (ancestor == uiInternals.getActiveModalComponent()) {
return ancestor;
}
maybeAncestor = ancestor.getParent();
}
}
}
return component;
}

private void fireShortcutEvent(Component component) {
if (ancestorsOrSelfAreVisible(lifecycleOwner)
&& lifecycleOwner.getElement().isEnabled()) {
Expand Down
4 changes: 2 additions & 2 deletions flow-server/src/test/java/com/vaadin/flow/component/ShortcutRegistrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,7 @@ public void uiRegistration_uiHasModalComponent_eventIsSentFromModalComponentInst
when(modal.getUI()).thenReturn(Optional.of(ui));
when(modal.getEventBus()).thenReturn(new ComponentEventBus(modal));
when(modal.getElement()).thenReturn(new Element("tag"));
when(modal.isVisible()).thenReturn(true);

UIInternals uiInternals = Mockito.mock(UIInternals.class);
Mockito.when(uiInternals.hasModalComponent()).thenReturn(true);
Expand All @@ -515,8 +516,7 @@ public void uiRegistration_uiHasModalComponent_eventIsSentFromModalComponentInst
Key.KEY_A);

mockLifecycle(true);
Mockito.when(lifecycleOwner.getParent())
.thenReturn(Optional.of(new FakeComponent()));
Mockito.when(lifecycleOwner.getParent()).thenReturn(Optional.of(modal));

clientResponse(listenOn);

Expand Down
25 changes: 23 additions & 2 deletions flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/ModalDialogView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

package com.vaadin.flow.uitest.ui;

import java.util.List;
import java.util.Map;

import com.vaadin.flow.component.ClickEvent;
import com.vaadin.flow.component.Component;
import com.vaadin.flow.component.ComponentEventListener;
Expand All @@ -25,10 +28,14 @@
import com.vaadin.flow.component.html.Div;
import com.vaadin.flow.component.html.Input;
import com.vaadin.flow.component.html.NativeButton;
import com.vaadin.flow.router.BeforeEvent;
import com.vaadin.flow.router.HasUrlParameter;
import com.vaadin.flow.router.Location;
import com.vaadin.flow.router.OptionalParameter;
import com.vaadin.flow.router.Route;

@Route(value = "com.vaadin.flow.uitest.ui.ModalDialogView")
public class ModalDialogView extends Div {
public class ModalDialogView extends Div implements HasUrlParameter<String> {

public static final String EVENT_LOG = "event-log";
public static final String UI_BUTTON = "ui-button";
Expand Down Expand Up @@ -58,6 +65,20 @@ public ModalDialogView() {
setId("main-div");
}

@Override
public void setParameter(BeforeEvent event,
@OptionalParameter String parameter) {
Location location = event.getLocation();
Map<String, List<String>> queryParameters = location
.getQueryParameters().getParameters();
if (queryParameters.containsKey("open_dialog")) {
boolean modal = queryParameters.get("open_dialog")
.contains("modal");
final Dialog dialog = new Dialog(modal);
dialog.open();
}
}

private void logClickEvent(ClickEvent<?> event) {
eventLog.addComponentAsFirst(new Div(new Text((eventCounter++) + "-"
+ event.getSource().getId().orElse("NO-SOURCE-ID"))));
Expand Down Expand Up @@ -124,7 +145,7 @@ public Dialog(boolean modal) {
}

public void open() {
final UI ui = ModalDialogView.this.getUI().get();
final UI ui = UI.getCurrent();
if (modal) {
ui.addModal(this);
} else {
Expand Down
30 changes: 27 additions & 3 deletions flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/ModalDialogIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ public class ModalDialogIT extends ChromeBrowserTest {
private TestBenchElement modalDialogButton;
private TestBenchElement modelessDialogButton;

@Before
public void init() {
open();
@Override
protected void open(String... parameters) {
super.open(parameters);
eventLog = $(DivElement.class).id(ModalDialogView.EVENT_LOG);
modalDialogButton = $(NativeButtonElement.class)
.id(ModalDialogView.OPEN_MODAL_BUTTON);
Expand All @@ -29,6 +29,8 @@ public void init() {
// #7799
@Test
public void modalDialogOpened_sameShortcutsListeningOnUi_noShortcutTriggered() {
open();

pressShortcutKey(
$(NativeButtonElement.class).id(ModalDialogView.UI_BUTTON));
validateLatestShortcutEvent(0, ModalDialogView.UI_BUTTON);
Expand All @@ -49,8 +51,26 @@ public void modalDialogOpened_sameShortcutsListeningOnUi_noShortcutTriggered() {
validateLatestShortcutEvent(2, ModalDialogView.UI_BUTTON);
}

@Test
public void modalDialogOpenInitially_dialogClosed_shortcutsInViewTrigger() {
open("open_dialog=modal");

// shortcuts on view should not trigger while dialog is open
pressShortcutKey(
$(NativeButtonElement.class).id(ModalDialogView.UI_BUTTON));

caalador marked this conversation as resolved.
Show resolved Hide resolved
closeDialog();

// shortcuts on view should not trigger when dialog has been closed
joheriks marked this conversation as resolved.
Show resolved Hide resolved
caalador marked this conversation as resolved.
Show resolved Hide resolved
pressShortcutKey(
$(NativeButtonElement.class).id(ModalDialogView.UI_BUTTON));
validateLatestShortcutEvent(0, ModalDialogView.UI_BUTTON);
}

@Test
public void modalDialogOpened_sameShortcutListeningOnUiAndDialog_onlyDialogShortcutExecuted() {
open();

pressShortcutKey(
$(NativeButtonElement.class).id(ModalDialogView.UI_BUTTON));
validateLatestShortcutEvent(0, ModalDialogView.UI_BUTTON);
Expand All @@ -72,6 +92,8 @@ public void modalDialogOpened_sameShortcutListeningOnUiAndDialog_onlyDialogShort

@Test
public void modelessDialogOpened_sharesShortcutWithUI_bothExecuted() {
open();

pressShortcutKey(
$(NativeButtonElement.class).id(ModalDialogView.UI_BUTTON));
validateLatestShortcutEvent(0, ModalDialogView.UI_BUTTON);
Expand All @@ -91,6 +113,8 @@ public void modelessDialogOpened_sharesShortcutWithUI_bothExecuted() {

@Test
public void modelessDialogOpened_sameShortcutListeningOnUiAndDialog_bothExecuted() {
open();

pressShortcutKey(
$(NativeButtonElement.class).id(ModalDialogView.UI_BUTTON));
validateLatestShortcutEvent(0, ModalDialogView.UI_BUTTON);
Expand Down