Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DomEventListener filter (and other stuff) removal #6427

Merged
merged 8 commits into from Sep 12, 2019
Expand Up @@ -130,7 +130,14 @@ 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
listenerMap.updateEventSettings(type);
}

@Override
Expand Down Expand Up @@ -365,15 +372,17 @@ private Map<String, ExpressionSettings> collectEventExpressions(
}

private void updateEventSettings(String eventType) {
Map<String, ExpressionSettings> eventSettings = collectEventExpressions(
eventType);
JsonObject eventSettingsJson = JsonUtils.createObject(eventSettings,
ExpressionSettings::toJson);
if (listeners != null && listeners.containsKey(eventType)) {
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
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
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