Skip to content

Commit

Permalink
fix: set shortcut listener to modal only if owner is child (#13272)
Browse files Browse the repository at this point in the history 
The previous fix for #13205 (PR #13263) moved all shortcut
registration event sources from UI to the modal component
if one was active; this caused shortcuts owned by components
not under the modal tree to also be moved if they were registered
when a modal component was active, and hence they stopped
working when the modal component tree was removed. This
modifies the previous fix to only change the event source if the
shortcut owner is under the an active modal tree.
  • Loading branch information
Johannes Eriksson authored Mar 11, 2022
1 parent 428e7bc commit 4b10fd4
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 18 deletions.
38 changes: 27 additions & 11 deletions flow-server/src/main/java/com/vaadin/flow/component/ShortcutRegistration.java
View file
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
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
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
32 changes: 29 additions & 3 deletions flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/ModalDialogIT.java
View file
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,28 @@ 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));
Assert.assertTrue("No event should be logged",
eventLog.$(DivElement.class).all().isEmpty());

closeDialog();

// shortcuts on view should trigger when dialog has been closed
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 +94,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 +115,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

0 comments on commit 4b10fd4

Please sign in to comment.