fix: remove detach listeners from listenOn components on detach owner (…

…#11956) fixes #11939
vaadin · Oct 1, 2021 · b99a029 · b99a029
1 parent e27b646
commit b99a029
Show file tree

Hide file tree

Showing 2 changed files with 99 additions and 5 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
@@ -92,6 +92,8 @@ public class ShortcutRegistration implements Registration, Serializable {
         if (listenOnComponents == null) {
             initListenOnComponent();
         }
+        addListenOnDetachListeners();
+
         boolean reinit = false;
         /*
          * In PreserveOnRefersh case the UI instance is not detached immediately
@@ -111,6 +113,7 @@ public class ShortcutRegistration implements Registration, Serializable {
         if (reinit) {
             removeAllListenerRegistrations();
             initListenOnComponent();
+            addListenOnDetachListeners();
         }
 
         for (int i = 0; i < listenOnComponents.length; i++) {
@@ -620,7 +623,7 @@ private void registerLifecycleOwner(Component owner) {
 
         // remove shortcut listener when detached
         Registration detachRegistration = owner
-                .addDetachListener(e -> removeListenerRegistration());
+                .addDetachListener(e -> removeLifecycleOwnerRegistrations());
 
         lifecycleRegistration = new CompoundRegistration(attachRegistration,
                 detachRegistration);
@@ -659,7 +662,7 @@ private Component[] registerOwnerListeners() {
                                         listenOnIndex)));
                 listenOnAttachListenerRegistrations[i]
                         .addRegistration(component.addDetachListener(
-                                detachEvent -> removeListenerRegistration()));
+                                detachEvent -> removeLifecycleOwnerRegistrations()));
             }
 
             // either the scope is an active UI, or the component is attached to
@@ -679,13 +682,16 @@ private void removeAllListenerRegistrations() {
             }
             listenOnAttachListenerRegistrations = null;
         }
+        removeLifecycleOwnerRegistrations();
+        listenOnComponents = null;
+    }
+
+    private void removeListenOnDetachListeners() {
         registrations.forEach(Registration::remove);
         registrations.clear();
-        removeListenerRegistration();
-        listenOnComponents = null;
     }
 
-    private void removeListenerRegistration() {
+    private void removeLifecycleOwnerRegistrations() {
         if (shortcutListenerRegistrations != null) {
             for (CompoundRegistration shortcutListenerRegistration : shortcutListenerRegistrations) {
                 if (shortcutListenerRegistration != null)
@@ -694,6 +700,7 @@ private void removeListenerRegistration() {
             shortcutListenerRegistrations = null;
         }
         shortcutActive = false;
+        removeListenOnDetachListeners();
     }
 
     private void queueBeforeExecutionCallback() {
@@ -823,6 +830,12 @@ public String toString() {
 
     private void initListenOnComponent() {
         listenOnComponents = registerOwnerListeners();
+    }
+
+    private void addListenOnDetachListeners() {
+        if (!registrations.isEmpty()) {
+            return;
+        }
         for (Component component : listenOnComponents) {
             Registration registration = component.addDetachListener(
                     event -> removeAllListenerRegistrations());

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
@@ -19,6 +19,7 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.junit.Assert;
@@ -514,6 +515,86 @@ public void constructedRegistration_lifecycleOnwerHasInvisibleParent_shorcutEven
         Assert.assertNull(event.get());
     }
 
+    @Test
+    public void constructedRegistration_lifeCycleOwnerIsDetached_detachListenerIsDeregisteredFromListenOnComponents() {
+        AtomicReference<ComponentEventListener> detachListener = new AtomicReference<>();
+        Mockito.doAnswer(invocaation -> {
+            detachListener.set(
+                    invocaation.getArgument(0, ComponentEventListener.class));
+            return mock(Registration.class);
+        }).when(lifecycleOwner).addDetachListener(any());
+
+        new ShortcutRegistration(lifecycleOwner, () -> listenOn, event -> {
+        }, Key.KEY_A);
+
+        Registration registration = Mockito.mock(Registration.class);
+        for (Component component : listenOn) {
+            Mockito.when(component.addDetachListener(Mockito.any()))
+                    .thenReturn(registration);
+        }
+
+        clientResponse();
+
+        detachListener.get().onComponentEvent(new DetachEvent(lifecycleOwner));
+
+        Mockito.verify(registration, Mockito.times(3)).remove();
+    }
+
+    @Test
+    public void reattachComponent_detachListenerIsAddedOnEveryAttach_listenOnUIIsClosing_eventIsPopulatedForANewUI() {
+        UI ui = Mockito.spy(UI.class);
+        Component owner = new FakeComponent();
+
+        Registration registration = Mockito.mock(Registration.class);
+        AtomicInteger count = new AtomicInteger();
+        Mockito.when(ui.addDetachListener(any())).thenAnswer(invocation -> {
+            count.incrementAndGet();
+            return registration;
+        });
+
+        Component[] components = new Component[] { ui };
+
+        ui.add(owner);
+        new ShortcutRegistration(owner, () -> components, event -> {
+        }, Key.KEY_A);
+
+        ArgumentCaptor<SerializableConsumer> captor = ArgumentCaptor
+                .forClass(SerializableConsumer.class);
+        verify(ui, atLeastOnce()).beforeClientResponse(eq(owner),
+                captor.capture());
+
+        SerializableConsumer consumer = captor.getValue();
+
+        // Fake beforeClientExecution call.
+        consumer.accept(mock(ExecutionContext.class));
+        Assert.assertEquals(1, count.get());
+
+        ui.remove(owner);
+
+        // reattach
+        ui.add(owner);
+
+        // Fake beforeClientExecution call.
+        consumer.accept(mock(ExecutionContext.class));
+        Assert.assertEquals(2, count.get());
+
+        UI newUI = Mockito.spy(UI.class);
+        // close the previous UI
+        ui.close();
+        components[0] = newUI;
+
+        owner.getElement().removeFromTree();
+        newUI.add(owner);
+
+        verify(newUI, atLeastOnce()).beforeClientResponse(eq(owner),
+                captor.capture());
+        // Fake beforeClientExecution call.
+        captor.getValue().accept(mock(ExecutionContext.class));
+
+        // the new UI should now also have expression with KeyA
+        Assert.assertTrue(hasKeyAInKeyDownExpression(newUI));
+    }
+
     private Element mockLifecycle(boolean visible) {
         Mockito.when(lifecycleOwner.isVisible()).thenReturn(visible);
         Element element = ElementFactory.createAnchor();