From 745af9c78f4d76fefafe4cfb2cac864a751a35d4 Mon Sep 17 00:00:00 2001 From: Vaadin Bot Date: Mon, 14 Mar 2022 09:43:31 +0200 Subject: [PATCH] fix: set shortcut listener to modal only if owner is child (#13272) (#13276) 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. Co-authored-by: Johannes Eriksson --- .../flow/component/ShortcutRegistration.java | 38 +++++++++++++------ .../component/ShortcutRegistrationTest.java | 4 +- .../flow/uitest/ui/ModalDialogView.java | 25 +++++++++++- .../vaadin/flow/uitest/ui/ModalDialogIT.java | 32 ++++++++++++++-- 4 files changed, 81 insertions(+), 18 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/component/ShortcutRegistration.java b/flow-server/src/main/java/com/vaadin/flow/component/ShortcutRegistration.java index b627a96882c..a7d2409128a 100644 --- a/flow-server/src/main/java/com/vaadin/flow/component/ShortcutRegistration.java +++ b/flow-server/src/main/java/com/vaadin/flow/component/ShortcutRegistration.java @@ -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]; @@ -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); @@ -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 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()) { diff --git a/flow-server/src/test/java/com/vaadin/flow/component/ShortcutRegistrationTest.java b/flow-server/src/test/java/com/vaadin/flow/component/ShortcutRegistrationTest.java index 1055743f513..f44bfe429a8 100644 --- a/flow-server/src/test/java/com/vaadin/flow/component/ShortcutRegistrationTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/component/ShortcutRegistrationTest.java @@ -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); @@ -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); diff --git a/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/ModalDialogView.java b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/ModalDialogView.java index 655568dc238..d7b1008651a 100644 --- a/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/ModalDialogView.java +++ b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/ModalDialogView.java @@ -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; @@ -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 { public static final String EVENT_LOG = "event-log"; public static final String UI_BUTTON = "ui-button"; @@ -58,6 +65,20 @@ public ModalDialogView() { setId("main-div"); } + @Override + public void setParameter(BeforeEvent event, + @OptionalParameter String parameter) { + Location location = event.getLocation(); + Map> 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")))); @@ -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 { diff --git a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/ModalDialogIT.java b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/ModalDialogIT.java index 1d8b64f82e9..5440927a9db 100644 --- a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/ModalDialogIT.java +++ b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/ModalDialogIT.java @@ -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); @@ -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); @@ -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); @@ -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); @@ -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);