Skip to content

Commit

Permalink
fix: make property change listener trigger on removeProperty (#19196)
Browse files Browse the repository at this point in the history 
Removing element property by calling com.vaadin.flow.internal.nodefeature.AbstractPropertyMap#removeProperty does not trigger property change listener because of com.vaadin.flow.internal.nodefeature.ElementPropertyMap#remove method is skipped due to AbstractPropertyMap#removeProperty method calls super:

    public void removeProperty(String name) {
        super.remove(name);
    }

Fixes #3994
  • Loading branch information
@akhodyko
akhodyko committed May 2, 2024
1 parent 90d3990 commit 63e6dfd
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 24 deletions.
3 changes: 1 addition & 2 deletions flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2613,8 +2613,7 @@ public void readBean_converterThrows_exceptionHandlerSet_bindingExceptionIsThrow
throw new NullPointerException();
}, name -> name))
.bind(Person::getFirstName, Person::setFirstName)
.read(new Person());

.read(Person.createTestPerson1());
}

@Test(expected = BindingException.class)
Expand Down
2 changes: 1 addition & 1 deletion flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/AbstractPropertyMap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public boolean hasProperty(String name) {
* the name of the property to remove
*/
public void removeProperty(String name) {
super.remove(name);
remove(name);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/ElementPropertyMap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ protected Serializable remove(String key) {
Serializable oldValue = super.remove(key);

fireEvent(new PropertyChangeEvent(Element.get(getNode()), key, oldValue,
true));
false));

return oldValue;
}
Expand Down
55 changes: 37 additions & 18 deletions flow-server/src/test/java/com/vaadin/flow/component/AbstractSinglePropertyFieldTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@
import com.vaadin.flow.dom.Element;
import com.vaadin.flow.function.SerializableRunnable;
import com.vaadin.flow.internal.JsonUtils;
import com.vaadin.flow.server.VaadinService;
import com.vaadin.flow.server.VaadinSession;
import com.vaadin.flow.server.MockServletServiceSessionSetup.TestVaadinServlet;
import com.vaadin.flow.server.MockServletServiceSessionSetup.TestVaadinServletService;
import com.vaadin.tests.PublicApiAnalyzer;
import com.vaadin.tests.util.MockUI;
Expand Down Expand Up @@ -86,9 +83,7 @@ public void stringField_basicCases() {
Assert.assertEquals("bar", field.getValue());
monitor.assertEvent(false, "foo", "bar");

// Cannot do removeProperty because
// https://github.com/vaadin/flow/issues/3994
field.getElement().setProperty("property", null);
field.getElement().removeProperty("property");
Assert.assertEquals("", field.getValue());
monitor.assertEvent(false, "bar", "");
}
Expand Down Expand Up @@ -188,6 +183,38 @@ public void stringNullField_basicCases() {
monitor.assertEvent(false, "", null);
}

@Tag("tag")
private static class StringNullFieldWithDefaultEmpty extends
AbstractSinglePropertyField<StringNullFieldWithDefaultEmpty, String> {
public StringNullFieldWithDefaultEmpty() {
super("property", "", true);
}
}

@Test
public void stringNullFieldWithDefaultEmpty_basicCases() {
StringNullFieldWithDefaultEmpty field = new StringNullFieldWithDefaultEmpty();
ValueChangeMonitor<String> monitor = new ValueChangeMonitor<>(field);

Assert.assertEquals("", field.getValue());
Assert.assertFalse(field.getElement().hasProperty("property"));
monitor.assertNoEvent();

field.setValue("foo");
Assert.assertEquals("foo", field.getValue());
monitor.assertEvent(false, "", "foo");

field.setValue("");
Assert.assertEquals("", field.getValue());
Assert.assertTrue(field.getElement().hasProperty("property"));
monitor.assertEvent(false, "foo", "");

field.setValue(null);
Assert.assertFalse(field.getElement().hasProperty("property"));
Assert.assertEquals("", field.getValue());
monitor.assertNoEvent();
}

@Tag("tag")
private static class DoubleField
extends AbstractSinglePropertyField<DoubleField, Double> {
Expand All @@ -214,9 +241,7 @@ public void doubleField_basicCases() {
Assert.assertEquals(1.1, field.getValue(), 0);
monitor.assertEvent(false, 10.1, 1.1);

// Cannot do removeProperty because
// https://github.com/vaadin/flow/issues/3994
field.getElement().setProperty("property", null);
field.getElement().removeProperty("property");
Assert.assertEquals(0.0, field.getValue(), 0);
monitor.assertEvent(false, 1.1, 0.0);
}
Expand Down Expand Up @@ -246,9 +271,7 @@ public void integerField_basicCases() {
Assert.assertEquals(1, field.getValue().intValue());
monitor.assertEvent(false, 0, 1);

// Cannot do removeProperty because
// https://github.com/vaadin/flow/issues/3994
field.getElement().setProperty("property", null);
field.getElement().removeProperty("property");
Assert.assertEquals(42, field.getValue().intValue());
monitor.assertEvent(false, 1, 42);
}
Expand Down Expand Up @@ -282,9 +305,7 @@ public void booleanField_basicCases() {
field.setValue(true);
monitor.discard();

// Cannot do removeProperty because
// https://github.com/vaadin/flow/issues/3994
field.getElement().setProperty("property", null);
field.getElement().removeProperty("property");
Assert.assertFalse(field.getValue());
monitor.assertEvent(false, true, false);
}
Expand Down Expand Up @@ -332,9 +353,7 @@ public void dateField_basicCases() {
monitor.assertEvent(false, LocalDate.of(2018, 4, 25),
LocalDate.of(2017, 3, 24));

// Cannot do removeProperty because
// https://github.com/vaadin/flow/issues/3994
field.getElement().setProperty("property", null);
field.getElement().removeProperty("property");
Assert.assertEquals(null, field.getValue());
monitor.assertEvent(false, LocalDate.of(2017, 3, 24), null);
}
Expand Down
105 changes: 105 additions & 0 deletions flow-server/src/test/java/com/vaadin/flow/component/PropertyDescriptorsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.vaadin.flow.component;

import java.util.Optional;
import java.util.concurrent.atomic.AtomicBoolean;

import org.junit.Assert;
import org.junit.Before;
Expand Down Expand Up @@ -321,6 +322,110 @@ public void booleanPropertyDefaultTrue_setToNull() {
booleanPropertyDefaultTrue.set(component, null);
}

@Test
public void booleanPropertyDefaultFalse_resetToDefault_propertyChangeListenerTriggered() {
booleanPropertyDefaultFalse.set(component, true);

AtomicBoolean listenerTriggered = new AtomicBoolean(false);
component.getElement().addPropertyChangeListener(TEST_PROPERTY,
event -> listenerTriggered.set(true));

booleanPropertyDefaultFalse.set(component, false);

Assert.assertTrue(listenerTriggered.get());
}

@Test
public void booleanPropertyDefaultTrue_resetToDefault_propertyChangeListenerTriggered() {
booleanPropertyDefaultTrue.set(component, false);

AtomicBoolean listenerTriggered = new AtomicBoolean(false);
component.getElement().addPropertyChangeListener(TEST_PROPERTY,
event -> listenerTriggered.set(true));

booleanPropertyDefaultTrue.set(component, true);

Assert.assertTrue(listenerTriggered.get());
}

@Test
public void stringPropertyDefaultEmpty_resetToDefault_propertyChangeListenerTriggered() {
stringPropertyDefaultEmpty.set(component, SOME_STRING_VALUE);

AtomicBoolean listenerTriggered = new AtomicBoolean(false);
component.getElement().addPropertyChangeListener(TEST_PROPERTY,
event -> listenerTriggered.set(true));

stringPropertyDefaultEmpty.set(component, EMPTY_STRING);

Assert.assertTrue(listenerTriggered.get());
}

@Test
public void stringPropertyDefaultFoo_resetToDefault_propertyChangeListenerTriggered() {
stringPropertyDefaultFoo.set(component, SOME_STRING_VALUE);

AtomicBoolean listenerTriggered = new AtomicBoolean(false);
component.getElement().addPropertyChangeListener(TEST_PROPERTY,
event -> listenerTriggered.set(true));

stringPropertyDefaultFoo.set(component, FOO);

Assert.assertTrue(listenerTriggered.get());
}

@Test
public void integerPropertyDefaultZero_resetToDefault_propertyChangeListenerTriggered() {
integerPropertyDefaultZero.set(component, SOME_INTEGER_VALUE);

AtomicBoolean listenerTriggered = new AtomicBoolean(false);
component.getElement().addPropertyChangeListener(TEST_PROPERTY,
event -> listenerTriggered.set(true));

integerPropertyDefaultZero.set(component, ZERO);

Assert.assertTrue(listenerTriggered.get());
}

@Test
public void integerPropertyDefaultOne_resetToDefault_propertyChangeListenerTriggered() {
integerPropertyDefaultOne.set(component, SOME_INTEGER_VALUE);

AtomicBoolean listenerTriggered = new AtomicBoolean(false);
component.getElement().addPropertyChangeListener(TEST_PROPERTY,
event -> listenerTriggered.set(true));

integerPropertyDefaultOne.set(component, ONE);

Assert.assertTrue(listenerTriggered.get());
}

@Test
public void doublePropertyDefaultZero_resetToDefault_propertyChangeListenerTriggered() {
doublePropertyDefaultZero.set(component, SOME_DOUBLE_VALUE);

AtomicBoolean listenerTriggered = new AtomicBoolean(false);
component.getElement().addPropertyChangeListener(TEST_PROPERTY,
event -> listenerTriggered.set(true));

doublePropertyDefaultZero.set(component, ZERO_DOUBLE);

Assert.assertTrue(listenerTriggered.get());
}

@Test
public void doublePropertyDefaultOne_resetToDefault_propertyChangeListenerTriggered() {
doublePropertyDefaultOne.set(component, SOME_DOUBLE_VALUE);

AtomicBoolean listenerTriggered = new AtomicBoolean(false);
component.getElement().addPropertyChangeListener(TEST_PROPERTY,
event -> listenerTriggered.set(true));

doublePropertyDefaultOne.set(component, ONE_DOUBLE);

Assert.assertTrue(listenerTriggered.get());
}

@Test
public void stringAttributeDefaultEmpty_initial() {
Assert.assertEquals(EMPTY_STRING,
Expand Down
4 changes: 2 additions & 2 deletions flow-server/src/test/java/com/vaadin/flow/internal/nodefeature/ElementPropertyMapTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,14 @@ public void removeProperty_fireEvent_listenerIsNotNotified() {
event.set(ev);
};
map.addPropertyChangeListener("foo", listener);
map.removeProperty("foo");

map.remove("foo");
Assert.assertNull(event.get().getValue());
Assert.assertEquals("bar", event.get().getOldValue());
Assert.assertEquals("foo", event.get().getPropertyName());
Assert.assertEquals(Element.get(map.getNode()),
event.get().getSource());
Assert.assertTrue(event.get().isUserOriginated());
Assert.assertFalse(event.get().isUserOriginated());
}

@Test
Expand Down

0 comments on commit 63e6dfd

Please sign in to comment.