Skip to content

Commit 24271eb

Browse files
web-padawanweb-padawan
authored
fix!: update multi-select-combo-box to synchronize on change (CP: 25.1) (#9686)
This PR cherry-picks changes from the original PR #9631 to branch 25.1. --- > ## Description > > Fixes #9611 > > With `@Push` and two push frames around a server-side `setValue`, MultiSelectComboBox could reset its value to empty because the `selected-items-changed` notification also fires for programmatic changes and gets echoed back as a spurious client value change. > > - Synchronized the value on the web component `change` event instead of `selected-items-changed`, so only genuine user gestures are treated as client changes > - Updated TestBench helpers to dispatch `change` (and `deselectAll` to use `clear()`) > - Added integration tests for the push scenario and user value commits > > > [!WARNING] > > Client-side changes to the web component `selectedItems` property no longer synchronize to the server unless a `change` event is dispatched. Server-side `setValue` and end-user interactions are unaffected. > > ## Type of change > > - Behavior altering fix > > --- > > 🤖 Generated with Claude Code
1 parent 28120f0 commit 24271eb

6 files changed

Lines changed: 195 additions & 22 deletions

File tree

‎vaadin-combo-box-flow-parent/vaadin-combo-box-flow-integration-tests/src/main/java/com/vaadin/flow/component/combobox/test/MultiSelectComboBoxPushValueChangePage.java‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
/*
2+
* Copyright 2000-2026 Vaadin Ltd.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
5+
* use this file except in compliance with the License. You may obtain a copy of
6+
* the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
12+
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
13+
* License for the specific language governing permissions and limitations under
14+
* the License.
15+
*/
16+
package com.vaadin.flow.component.combobox.test;
17+
18+
import java.util.List;
19+
import java.util.Set;
20+
21+
import com.vaadin.flow.component.UI;
22+
import com.vaadin.flow.component.combobox.MultiSelectComboBox;
23+
import com.vaadin.flow.component.html.Div;
24+
import com.vaadin.flow.component.html.NativeButton;
25+
import com.vaadin.flow.router.Route;
26+
import com.vaadin.flow.shared.communication.PushMode;
27+
28+
/**
29+
* View that runs a server-side {@code setValue} between two push frames, which
30+
* must not relay a spurious client-initiated value change with an empty value
31+
* back to the server.
32+
*/
33+
@Route("vaadin-multi-select-combo-box/push-value-change")
34+
public class MultiSelectComboBoxPushValueChangePage extends Div {
35+
public MultiSelectComboBoxPushValueChangePage() {
36+
UI.getCurrent().getPushConfiguration().setPushMode(PushMode.AUTOMATIC);
37+
38+
Div log = new Div();
39+
log.setId("value-change-log");
40+
41+
NativeButton runScenario = new NativeButton("Run scenario", event -> {
42+
MultiSelectComboBox<String> comboBox = new MultiSelectComboBox<>();
43+
comboBox.setItems(List.of("1", "2", "3"));
44+
45+
comboBox.addValueChangeListener(e -> {
46+
Div entry = new Div();
47+
entry.setText("isFromClient=%s old=%s new=%s value=%s"
48+
.formatted(e.isFromClient(), e.getOldValue(),
49+
e.getValue(), comboBox.getValue()));
50+
log.add(entry);
51+
});
52+
53+
add(comboBox);
54+
55+
UI ui = UI.getCurrent();
56+
ui.push();
57+
comboBox.setValue(Set.of("1"));
58+
ui.push();
59+
});
60+
runScenario.setId("run-scenario");
61+
62+
add(runScenario, log);
63+
}
64+
}

‎vaadin-combo-box-flow-parent/vaadin-combo-box-flow-integration-tests/src/test/java/com/vaadin/flow/component/combobox/test/MultiSelectComboBoxPushValueChangeIT.java‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
/*
2+
* Copyright 2000-2026 Vaadin Ltd.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
5+
* use this file except in compliance with the License. You may obtain a copy of
6+
* the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
12+
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
13+
* License for the specific language governing permissions and limitations under
14+
* the License.
15+
*/
16+
package com.vaadin.flow.component.combobox.test;
17+
18+
import java.util.List;
19+
20+
import org.junit.Assert;
21+
import org.junit.Before;
22+
import org.junit.Test;
23+
24+
import com.vaadin.flow.testutil.TestPath;
25+
import com.vaadin.testbench.TestBenchElement;
26+
import com.vaadin.tests.AbstractComponentIT;
27+
28+
/**
29+
* Verifies that with {@code @Push} enabled, a server-side {@code setValue}
30+
* surrounded by two push frames does not produce a spurious client-initiated
31+
* value change with an empty value.
32+
*/
33+
@TestPath("vaadin-multi-select-combo-box/push-value-change")
34+
public class MultiSelectComboBoxPushValueChangeIT extends AbstractComponentIT {
35+
private TestBenchElement runScenario;
36+
private TestBenchElement log;
37+
38+
@Before
39+
public void init() {
40+
open();
41+
runScenario = $("button").waitForFirst();
42+
log = $("div").id("value-change-log");
43+
}
44+
45+
@Test
46+
public void setValueWithPushFrames_noSpuriousEmptyClientValueChange() {
47+
runScenario.click();
48+
49+
// Wait for the value change triggered by the server-side setValue
50+
waitUntil(driver -> !log.getText().isEmpty());
51+
52+
List<String> entries = getLogEntries();
53+
54+
// No client-initiated value change with an empty value must occur
55+
for (String entry : entries) {
56+
boolean isFromClient = entry.contains("isFromClient=true");
57+
boolean isEmptyValue = entry.contains("value=[]");
58+
Assert.assertFalse(
59+
"Spurious client-initiated value change with empty value: "
60+
+ entry,
61+
isFromClient && isEmptyValue);
62+
}
63+
64+
// The final server value must remain ["1"]
65+
String last = entries.get(entries.size() - 1);
66+
Assert.assertTrue(
67+
"Expected final value to remain [1], but log was: " + entries,
68+
last.contains("value=[1]"));
69+
}
70+
71+
private List<String> getLogEntries() {
72+
return log.$("div").all().stream().map(TestBenchElement::getText)
73+
.toList();
74+
}
75+
}

‎vaadin-combo-box-flow-parent/vaadin-combo-box-flow-integration-tests/src/test/java/com/vaadin/flow/component/combobox/test/MultiSelectComboBoxValueChangeIT.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,16 @@ public void selectItems_clearValueServerSide_valueCleared() {
102102
assertValueChange(Collections.emptySet(), "server");
103103
}
104104

105+
@Test
106+
public void selectItems_deselectAll_valueCleared() {
107+
comboBox.selectByText("Item 1");
108+
comboBox.selectByText("Item 10");
109+
comboBox.deselectAll();
110+
111+
assertSelectedItems(Collections.emptySet());
112+
assertValueChange(Collections.emptySet(), "client");
113+
}
114+
105115
private void assertSelectedItems(Set<String> items) {
106116
List<String> selectedTexts = comboBox.getSelectedTexts();
107117
Assert.assertEquals("Number of selected items does not match",

‎vaadin-combo-box-flow-parent/vaadin-combo-box-flow/src/main/java/com/vaadin/flow/component/combobox/MultiSelectComboBox.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,10 @@ public MultiSelectComboBox(int pageSize) {
135135
MultiSelectComboBox::presentationToModel,
136136
MultiSelectComboBox::modelToPresentation);
137137

138+
// Use change instead of the default selected-items-changed
139+
// event to avoid notification for programmatic value changes.
140+
setSynchronizedEvent("change");
141+
138142
// Create the selection model that manages the currently selected items.
139143
// The model ensures that items are compared based on their data
140144
// provider identify, and that the selection only changes if items

‎vaadin-combo-box-flow-parent/vaadin-combo-box-flow/src/test/java/com/vaadin/flow/component/combobox/MultiSelectComboBoxTest.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@
3333
import com.vaadin.flow.component.Unit;
3434
import com.vaadin.flow.component.shared.HasThemeVariant;
3535
import com.vaadin.flow.component.shared.InputField;
36+
import com.vaadin.flow.internal.nodefeature.ElementListenerMap;
37+
import com.vaadin.flow.shared.JsonConstants;
3638

3739
import tools.jackson.databind.node.ArrayNode;
3840

@@ -49,6 +51,22 @@ public void initialValue() {
4951
Assert.assertEquals(Collections.emptySet(), comboBox.getValue());
5052
}
5153

54+
@Test
55+
public void valueSynchronizedOnChangeEvent() {
56+
MultiSelectComboBox<String> comboBox = new MultiSelectComboBox<>();
57+
ElementListenerMap listeners = comboBox.getElement().getNode()
58+
.getFeature(ElementListenerMap.class);
59+
String syncExpression = JsonConstants.SYNCHRONIZE_PROPERTY_TOKEN
60+
+ "selectedItems";
61+
62+
Assert.assertTrue("value should synchronize on the change event",
63+
listeners.getExpressions("change").contains(syncExpression));
64+
Assert.assertFalse(
65+
"value should not synchronize on selected-items-changed",
66+
listeners.getExpressions("selected-items-changed")
67+
.contains(syncExpression));
68+
}
69+
5270
@Test(expected = UnsupportedOperationException.class)
5371
public void getValue_returnsImmutableSet() {
5472
MultiSelectComboBox<String> comboBox = new MultiSelectComboBox<>();

‎vaadin-combo-box-flow-parent/vaadin-combo-box-testbench/src/main/java/com/vaadin/flow/component/combobox/testbench/MultiSelectComboBoxElement.java‎

Lines changed: 24 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -93,18 +93,17 @@ public List<String> getOptions() {
9393
*/
9494
public void selectByText(String label) {
9595
setFilter(label);
96-
//@formatter:off
97-
String script =
98-
"const combobox = arguments[0];" +
99-
"const label = arguments[1];" +
100-
"const itemToSelect = combobox.filteredItems.find(item => item.label === label);" +
101-
"if (!itemToSelect) return false;" +
102-
"const isSelected = combobox.selectedItems.some(item => item.key === itemToSelect.key);" +
103-
"if (!isSelected) {" +
104-
" combobox.selectedItems = [...combobox.selectedItems, itemToSelect];" +
105-
"}" +
106-
"return true;";
107-
//@formatter:on
96+
String script = """
97+
const combobox = arguments[0];
98+
const label = arguments[1];
99+
const itemToSelect = combobox.filteredItems.find(item => item.label === label);
100+
if (!itemToSelect) return false;
101+
const isSelected = combobox.selectedItems.some(item => item.key === itemToSelect.key);
102+
if (!isSelected) {
103+
combobox.selectedItems = [...combobox.selectedItems, itemToSelect];
104+
combobox.dispatchEvent(new CustomEvent('change', { bubbles: true }));
105+
}
106+
return true;""";
108107
Boolean success = (Boolean) executeScript(script, this, label);
109108
closePopup();
110109
if (!success) {
@@ -121,23 +120,26 @@ public void selectByText(String label) {
121120
* The label of the item to deselect
122121
*/
123122
public void deselectByText(String label) {
124-
//@formatter:off
125-
String script =
126-
"const combobox = arguments[0];" +
127-
"const label = arguments[1];" +
128-
"const isSelected = combobox.selectedItems.some(item => item.label === label);" +
129-
"if (isSelected) {" +
130-
" combobox.selectedItems = combobox.selectedItems.filter(item => item.label !== label);" +
131-
"}";
132-
//@formatter:on
123+
String script = """
124+
const combobox = arguments[0];
125+
const label = arguments[1];
126+
const isSelected = combobox.selectedItems.some(item => item.label === label);
127+
if (isSelected) {
128+
combobox.selectedItems = combobox.selectedItems.filter(item => item.label !== label);
129+
combobox.dispatchEvent(new CustomEvent('change', { bubbles: true }));
130+
}""";
133131
executeScript(script, this, label);
134132
}
135133

136134
/**
137135
* Deselects all items, effectively clearing the value.
138136
*/
139137
public void deselectAll() {
140-
String script = "const combobox = arguments[0]; combobox.selectedItems = [];";
138+
String script = """
139+
const combobox = arguments[0];
140+
if (combobox.selectedItems.length) {
141+
combobox.clear();
142+
}""";
141143
executeScript(script, this);
142144
}
143145

0 commit comments

Comments
 (0)