Fix DomEventListener filter (and other stuff) removal

flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementListenerMap.java

@@ -130,7 +130,17 @@ public void remove() {
             if (unregisterHandlers != null) {
                 unregisterHandlers.forEach(SerializableRunnable::run);
             }
+
             listenerMap.removeListener(type, this);
+
+            // update settings after removal. If we have listeners of the
+            // same type registered, we want to remove settings set by this
+            // particular listener from the overall set
+            // fixes #5090
+            if (listenerMap.listeners != null
+                    && listenerMap.listeners.containsKey(type)) {
+                listenerMap.updateEventSettings(type);
+            }
         }
 
         @Override
@@ -365,15 +375,15 @@ private Map<String, ExpressionSettings> collectEventExpressions(
     }
 
     private void updateEventSettings(String eventType) {
-        Map<String, ExpressionSettings> eventSettings = collectEventExpressions(
-                eventType);
-        JsonObject eventSettingsJson = JsonUtils.createObject(eventSettings,
-                ExpressionSettings::toJson);
+            Map<String, ExpressionSettings> eventSettings = collectEventExpressions(
+                    eventType);
+            JsonObject eventSettingsJson = JsonUtils.createObject(eventSettings,
+                    ExpressionSettings::toJson);
 
-        ConstantPoolKey constantPoolKey = new ConstantPoolKey(
-                eventSettingsJson);
+            ConstantPoolKey constantPoolKey = new ConstantPoolKey(
+                    eventSettingsJson);
 
-        put(eventType, constantPoolKey);
+            put(eventType, constantPoolKey);
     }
 
     private void removeListener(String eventType,

flow-server/src/test/java/com/vaadin/flow/internal/nodefeature/ElementListenersTest.java

@@ -15,14 +15,18 @@
  */
 package com.vaadin.flow.internal.nodefeature;
 
+import java.io.Serializable;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.apache.commons.lang3.SerializationUtils;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
+import org.mockito.Mockito;
 
 import com.vaadin.flow.dom.DisabledUpdateMode;
 import com.vaadin.flow.dom.DomEvent;
@@ -34,14 +38,23 @@
 
 import elemental.json.Json;
 import elemental.json.JsonObject;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
 
 public class ElementListenersTest
         extends AbstractNodeFeatureTest<ElementListenerMap> {
     private static final DomEventListener noOp = e -> {
         // no op
     };
 
-    private ElementListenerMap ns = createFeature();
+    private ElementListenerMap ns;
+
+    @Before
+    public void init() {
+        ns = createFeature();
+    }
 
     @Test
     public void addedListenerGetsEvent() {
@@ -107,7 +120,77 @@ public void handleEvent(DomEvent event) {
         expressions = getExpressions("eventType");
         Assert.assertTrue(expressions.contains("data1"));
         Assert.assertTrue(expressions.contains("data2"));
-        // data3 might still be there, but we don't care
+        // due to fix to #5090, data3 won't be present after removal
+        Assert.assertFalse(expressions.contains("data3"));
+    }
+
+    @Test
+    public void settingsAreOnlyUpdated_should_ListenersSharingTheTypeOfRemovedListenerExist() {
+        ns = spy(createFeature());
+        DomEventListener del1 = event -> {};
+        DomEventListener del2 = event -> {};
+        DomEventListener del3 = event -> {};
+        Registration handle1 = ns.add("eventType", del1).addEventData("data1");
+        Registration handle2 = ns.add("eventType", del2).addEventData("data2");
+        Registration handle3 = ns.add("eventTypeOther", del3)
+                .addEventData("data3");
+        Mockito.reset(ns);
+
+        Set<String> expressions = getExpressions("eventType");
+        expressions.addAll(getExpressions("eventTypeOther"));
+
+        Assert.assertTrue(expressions.contains("data1"));
+        Assert.assertTrue(expressions.contains("data2"));
+        Assert.assertTrue(expressions.contains("data3"));
+
+        handle1.remove();
+
+        Mockito.verify(ns, times(1)).put(eq("eventType"),
+                any(Serializable.class));
+
+        expressions = getExpressions("eventType");
+        expressions.addAll(getExpressions("eventTypeOther"));
+
+        Assert.assertFalse(expressions.contains("data1"));
+        Assert.assertTrue(expressions.contains("data2"));
+        Assert.assertTrue(expressions.contains("data3"));
+
+        handle2.remove();
+        // updating settings does not take place a second time
+        Mockito.verify(ns, times(1)).put(eq("eventType"),
+                any(Serializable.class));
+
+        expressions = getExpressions("eventType");
+        expressions.addAll(getExpressions("eventTypeOther"));
+
+        Assert.assertFalse(expressions.contains("data1"));
+        Assert.assertFalse(expressions.contains("data2"));
+        Assert.assertTrue(expressions.contains("data3"));
+    }
+
+    @Test
+    public void addingRemovingAndAddingListenerOfTheSameType() {
+        DomEventListener del1 = event -> {};
+        DomEventListener del2 = event -> {};
+        Registration handle = ns.add("eventType", del1).addEventData("data1");
+
+        Set<String> expressions = getExpressions("eventType");
+        Assert.assertTrue(expressions.contains("data1"));
+
+        handle.remove();
+        expressions = getExpressions("eventType");
+        Assert.assertFalse(expressions.contains("data1"));
+
+        // re-add a listener for "eventType", using different eventData
+        handle = ns.add("eventType", del2).addEventData("data2");
+        expressions = getExpressions("eventType");
+        Assert.assertFalse(expressions.contains("data1"));
+        Assert.assertTrue(expressions.contains("data2"));
+
+        handle.remove();
+        expressions = getExpressions("eventType");
+        Assert.assertFalse(expressions.contains("data1"));
+        Assert.assertFalse(expressions.contains("data2"));
     }
 
     @Test
@@ -186,7 +269,7 @@ public void serializable() {
     }
 
     @Test
-    public void synchronizePropery_hasSynchronizedProperty() {
+    public void synchronizeProperty_hasSynchronizedProperty() {
         DomListenerRegistration registration = ns.add("foo", noOp);
 
         Assert.assertNull(ns.getPropertySynchronizationMode("name"));
@@ -202,7 +285,7 @@ public void synchronizePropery_hasSynchronizedProperty() {
     }
 
     @Test
-    public void synchronizePropery_alwaysMode() {
+    public void synchronizeProperty_alwaysMode() {
         DomListenerRegistration registration = ns.add("foo", noOp)
                 .setDisabledUpdateMode(DisabledUpdateMode.ALWAYS);
 
@@ -213,7 +296,7 @@ public void synchronizePropery_alwaysMode() {
     }
 
     @Test
-    public void synchronizePropery_bothModes() {
+    public void synchronizeProperty_bothModes() {
         DomListenerRegistration registration1 = ns.add("foo", noOp)
                 .setDisabledUpdateMode(DisabledUpdateMode.ALWAYS);
 
@@ -227,7 +310,7 @@ public void synchronizePropery_bothModes() {
     }
 
     @Test
-    public void synchronizePropery_hasExpressionToken() {
+    public void synchronizeProperty_hasExpressionToken() {
         DomListenerRegistration registration = ns.add("foo", noOp);
 
         Assert.assertEquals(Collections.emptySet(), getExpressions("foo"));
@@ -241,14 +324,14 @@ public void synchronizePropery_hasExpressionToken() {
     }
 
     @Test(expected = IllegalArgumentException.class)
-    public void synchronizePropery_nullArgument_illegalArgumentException() {
+    public void synchronizeProperty_nullArgument_illegalArgumentException() {
         DomListenerRegistration registration = ns.add("foo", noOp);
 
         registration.synchronizeProperty(null);
     }
 
     @Test(expected = IllegalArgumentException.class)
-    public void synchronizePropery_emptyArgument_illegalArgumentException() {
+    public void synchronizeProperty_emptyArgument_illegalArgumentException() {
         DomListenerRegistration registration = ns.add("foo", noOp);
 
         registration.synchronizeProperty("");
@@ -257,7 +340,7 @@ public void synchronizePropery_emptyArgument_illegalArgumentException() {
     // Helper for accessing package private API from other tests
     public static Set<String> getExpressions(
             ElementListenerMap elementListenerMap, String eventName) {
-        return elementListenerMap.getExpressions(eventName);
+        return new HashSet<>(elementListenerMap.getExpressions(eventName));
     }
 
     private Set<String> getExpressions(String name) {

flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/DomEventFilterView.java

@@ -15,13 +15,22 @@
  */
 package com.vaadin.flow.uitest.ui;
 
+import java.util.concurrent.atomic.AtomicReference;
+
 import com.vaadin.flow.component.Component;
 import com.vaadin.flow.component.ComponentEvent;
 import com.vaadin.flow.component.ComponentEventListener;
+import com.vaadin.flow.component.ComponentUtil;
 import com.vaadin.flow.component.DomEvent;
 import com.vaadin.flow.component.EventData;
+import com.vaadin.flow.component.KeyDownEvent;
 import com.vaadin.flow.component.Tag;
+import com.vaadin.flow.component.html.Input;
+import com.vaadin.flow.component.html.NativeButton;
+import com.vaadin.flow.component.html.Paragraph;
+import com.vaadin.flow.data.value.ValueChangeMode;
 import com.vaadin.flow.dom.DebouncePhase;
+import com.vaadin.flow.dom.DomListenerRegistration;
 import com.vaadin.flow.dom.Element;
 import com.vaadin.flow.router.Route;
 import com.vaadin.flow.shared.Registration;
@@ -90,6 +99,38 @@ public DomEventFilterView() {
         messages.setAttribute("id", "messages");
         getElement().appendChild(space, debounce, component.getElement(),
                 messages);
+
+        // tests for#5090
+        final AtomicReference<DomListenerRegistration> atomicReference = new AtomicReference<>();
+        final Paragraph resultParagraph = new Paragraph();
+        resultParagraph.setId("result-paragraph");
+
+        NativeButton removalButton = new NativeButton("Remove DOM listener", event -> {
+            resultParagraph.setText("REMOVED");
+            atomicReference.get().remove();
+        });
+        removalButton.setId("listener-removal-button");
+
+        Input listenerInput = new Input(ValueChangeMode.ON_CHANGE);
+        listenerInput.setId("listener-input");
+
+        /*
+        The event.preventDefault() is here to make sure that the listener
+         has been cleaned on the client-side as well. The server-side
+         cleaning is not really in question.
+         */
+        ComponentUtil.addListener(listenerInput, KeyDownEvent.class,
+                event -> resultParagraph.setText("A"), registration -> {
+                    atomicReference.set(registration);
+                    registration.setFilter("event.key === 'a' && " +
+                            "(event.preventDefault() || true)");
+                });
+        ComponentUtil.addListener(listenerInput, KeyDownEvent.class,
+                event -> resultParagraph.setText("B"),
+                registration -> registration.setFilter("event.key === 'b' && " +
+                        "(event.preventDefault() || true)"));
+
+        add(listenerInput, removalButton, resultParagraph);
     }
 
     private void addMessage(String message) {

flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/DomEventFilterIT.java

@@ -106,6 +106,55 @@ public void componentWithDebounce() throws InterruptedException {
 
     }
 
+    @Test
+    public void twoListeners_removingOne_should_cleanItsFilter() {
+        open();
+
+        WebElement paragraph = findElement(By.id("result-paragraph"));
+        WebElement button = findElement(By.id("listener-removal-button"));
+        WebElement input = findElement(By.id("listener-input"));
+
+        Assert.assertEquals("Result paragraph should be empty", "",
+                paragraph.getText());
+
+        input.sendKeys("a");
+        Assert.assertEquals(
+                "Filter should have prevented default, and input is empty", "",
+                input.getAttribute("value"));
+        Assert.assertEquals(
+                "Event was sent to server and paragraph should be 'A'", "A",
+                paragraph.getText());
+
+        input.sendKeys("b");
+        Assert.assertEquals(
+                "Filter should have prevented default, and input is empty", "",
+                input.getAttribute("value"));
+        Assert.assertEquals(
+                "Event was sent to server and paragraph should be 'B'", "B",
+                paragraph.getText());
+
+        // remove keybind for A
+        button.click();
+        Assert.assertEquals("Result paragraph should be 'REMOVED'", "REMOVED",
+                paragraph.getText());
+
+        // keybind for A should no longer work
+        input.sendKeys("a");
+        Assert.assertEquals("Filter should be removed, and input has 'a'", "a",
+                input.getAttribute("value"));
+        Assert.assertEquals("Result paragraph should still be 'REMOVED'",
+                "REMOVED", paragraph.getText());
+
+        // b should still be functional
+        input.sendKeys("b");
+        Assert.assertEquals(
+                "Filter should have prevented default, and input has only 'a'",
+                "a", input.getAttribute("value"));
+        Assert.assertEquals(
+                "Event was sent to server and paragraph should be 'B'", "B",
+                paragraph.getText());
+    }
+
     private void assertMessages(int skip, String... expectedTail) {
         List<WebElement> messages = getMessages();
         if (messages.size() < skip) {