Skip to content

Commit

Permalink
Fix DomEventListener filter (and other stuff) removal (#6427)
Browse files Browse the repository at this point in the history 
* Make sure, that settings set by a event listener are removed when the listener is removed
* Integration test for the fix
* addAndRemoveEventData test validates that associated event data is removed
* Unit test for verifying when settings are updated when remove is called
* Add a test for adding, removing, and adding a listener for the same type

(cherry picked from commit a9dbe94)
  • Loading branch information
@ujoni @mehdi-vaadin
ujoni authored and mehdi-vaadin committed Sep 13, 2019
1 parent 467d36f commit fdbb6f7
Show file tree
Hide file tree
Showing 4 changed files with 199 additions and 16 deletions.
24 changes: 17 additions & 7 deletions flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementListenerMap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
101 changes: 92 additions & 9 deletions flow-server/src/test/java/com/vaadin/flow/internal/nodefeature/ElementListenersTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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() {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"));
Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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"));
Expand All @@ -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("");
Expand All @@ -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) {
Expand Down
41 changes: 41 additions & 0 deletions flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/DomEventFilterView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
49 changes: 49 additions & 0 deletions flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/DomEventFilterIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit fdbb6f7

Please sign in to comment.