issue in #11343 and #11385 is not reproducible on top this patch Three different bugs are involved in this fix: we bring the old fix(#11094) back and fixed the other related issues: 1) allow adding the same new item after dataProvider got reset, This is cause by the client side parameter `LastNewItemString`, it saves the value added before resetting. 2) clear the pending newItem eagerly, so that the same value will not be added again.tags/8.8.0
@@ -23,7 +23,6 @@ import java.util.HashSet; | |||
import java.util.List; | |||
import java.util.Locale; | |||
import java.util.Set; | |||
import java.util.UUID; | |||
import java.util.logging.Logger; | |||
import com.google.gwt.animation.client.AnimationScheduler; | |||
@@ -122,7 +121,7 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, | |||
* icon URI or null | |||
*/ | |||
public ComboBoxSuggestion(String key, String caption, String style, | |||
String untranslatedIconUri) { | |||
String untranslatedIconUri) { | |||
this.key = key; | |||
this.caption = caption; | |||
this.style = style; | |||
@@ -1642,6 +1641,11 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, | |||
performSelection(selectedKey, oldSuggestionTextMatchTheOldSelection, | |||
!isWaitingForFilteringResponse() || popupOpenerClicked); | |||
// currentSuggestion should be set to match the value of the | |||
// ComboBox | |||
resetCurrentSuggestionBasedOnServerResponse(selectedKey, | |||
selectedCaption, selectedIconUri); | |||
cancelPendingPostFiltering(); | |||
setSelectedCaption(selectedCaption); | |||
@@ -1649,6 +1653,22 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, | |||
setSelectedItemIcon(selectedIconUri); | |||
} | |||
/* | |||
* Updates the current suggestion based on values provided by the | |||
* server. | |||
*/ | |||
private void resetCurrentSuggestionBasedOnServerResponse( | |||
String selectedKey, String selectedCaption, | |||
String selectedIconUri) { | |||
if (currentSuggestion == null | |||
&& (selectedKey != null || selectedCaption != null)) { | |||
currentSuggestion = new ComboBoxSuggestion(selectedKey, | |||
selectedCaption, "", selectedIconUri); | |||
} else if (selectedKey == null && selectedCaption == null) { | |||
currentSuggestion = null; | |||
} | |||
} | |||
} | |||
// TODO decide whether this should change - affects themes and v7 | |||
@@ -1707,7 +1727,9 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, | |||
/** For internal use only. May be removed or replaced in the future. */ | |||
public boolean initDone = false; | |||
/** For internal use only. May be removed or replaced in the future. */ | |||
/** | |||
* For internal use only. May be removed or replaced in the future. | |||
*/ | |||
public String lastFilter = ""; | |||
/** | |||
@@ -1802,6 +1824,16 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, | |||
return s.getMarginWidth() + s.getBorderWidth() + s.getPaddingWidth(); | |||
} | |||
/** | |||
* This method will reset the saved item string which is added last time. | |||
*/ | |||
public void resetLastNewItemString() { | |||
// Clean the temp string eagerly in order to re-add the same value again | |||
// after data provider got reset. | |||
// Fixes issue https://github.com/vaadin/framework/issues/11317 | |||
lastNewItemString = null; | |||
} | |||
/* | |||
* (non-Javadoc) | |||
* |
@@ -130,11 +130,9 @@ public class ComboBoxConnector extends AbstractListingConnector | |||
getWidget().selectedOptionKey = null; | |||
getWidget().currentSuggestion = null; | |||
} | |||
if (isNewItemStillPending() | |||
&& pendingNewItemValue == getState().selectedItemCaption) { | |||
// no automated selection handling required | |||
clearNewItemHandling(); | |||
} | |||
clearNewItemHandlingIfMatch(getState().selectedItemCaption); | |||
getDataReceivedHandler().updateSelectionFromServer( | |||
getState().selectedItemKey, getState().selectedItemCaption, | |||
getState().selectedItemIcon); | |||
@@ -188,10 +186,11 @@ public class ComboBoxConnector extends AbstractListingConnector | |||
if (itemValue != null && !itemValue.equals(pendingNewItemValue)) { | |||
// clear any previous handling as outdated | |||
clearNewItemHandling(); | |||
pendingNewItemValue = itemValue; | |||
rpc.createNewItem(itemValue); | |||
getDataReceivedHandler().clearPendingNavigation(); | |||
} | |||
pendingNewItemValue = itemValue; | |||
rpc.createNewItem(itemValue); | |||
getDataReceivedHandler().clearPendingNavigation(); | |||
} | |||
/** | |||
@@ -359,7 +358,7 @@ public class ComboBoxConnector extends AbstractListingConnector | |||
updateSuggestions(start, end); | |||
getWidget().setTotalSuggestions(getDataSource().size()); | |||
getWidget().resetLastNewItemString(); | |||
getDataReceivedHandler().dataReceived(); | |||
} | |||
@@ -451,10 +450,7 @@ public class ComboBoxConnector extends AbstractListingConnector | |||
* versions. | |||
*/ | |||
private void clearNewItemHandling() { | |||
// never clear pending value before it has been handled | |||
if (!isNewItemStillPending()) { | |||
pendingNewItemValue = null; | |||
} | |||
pendingNewItemValue = null; | |||
} | |||
/** |
@@ -191,6 +191,15 @@ public class ComboBox<T> extends AbstractSingleSelect<T> | |||
if (getNewItemProvider() != null) { | |||
Optional<T> item = getNewItemProvider().apply(itemValue); | |||
added = item.isPresent(); | |||
// Fixes issue https://github.com/vaadin/framework/issues/11343 | |||
// Update the internal selection state immediately to avoid | |||
// client side hanging. This is needed for cases that user | |||
// interaction fires multi events (like adding and deleting) | |||
// on a new item during the same round trip. | |||
item.ifPresent(value -> { | |||
setSelectedItem(value, true); | |||
getDataCommunicator().reset(); | |||
}); | |||
} else if (getNewItemHandler() != null) { | |||
getNewItemHandler().accept(itemValue); | |||
// Up to the user to tell if no item was added. | |||
@@ -446,7 +455,7 @@ public class ComboBox<T> extends AbstractSingleSelect<T> | |||
* @since 8.0 | |||
*/ | |||
public void setDataProvider(CaptionFilter captionFilter, | |||
ListDataProvider<T> listDataProvider) { | |||
ListDataProvider<T> listDataProvider) { | |||
Objects.requireNonNull(listDataProvider, | |||
"List data provider cannot be null"); | |||
@@ -610,12 +619,11 @@ public class ComboBox<T> extends AbstractSingleSelect<T> | |||
* <p> | |||
* The empty string {@code ""} is the default empty selection caption. | |||
* | |||
* @return the empty selection caption, not {@code null} | |||
* @see #setEmptySelectionAllowed(boolean) | |||
* @see #isEmptySelectionAllowed() | |||
* @see #setEmptySelectionCaption(String) | |||
* @see #isSelected(Object) | |||
* | |||
* @return the empty selection caption, not {@code null} | |||
* @since 8.0 | |||
*/ | |||
public String getEmptySelectionCaption() { |
@@ -0,0 +1,99 @@ | |||
package com.vaadin.tests.components.combobox; | |||
import java.util.ArrayList; | |||
import java.util.Collections; | |||
import java.util.List; | |||
import java.util.Optional; | |||
import com.vaadin.server.VaadinRequest; | |||
import com.vaadin.tests.components.AbstractTestUIWithLog; | |||
import com.vaadin.ui.Button; | |||
import com.vaadin.ui.CheckBox; | |||
import com.vaadin.ui.ComboBox; | |||
import com.vaadin.ui.HorizontalLayout; | |||
import com.vaadin.ui.Label; | |||
import com.vaadin.ui.Notification; | |||
public class ComboBoxAddNewItemAndResetProviderAtSameRound | |||
extends AbstractTestUIWithLog { | |||
ComboBox<String> comboBox; | |||
List<String> items = new ArrayList<>(); | |||
Label resetLabel = new Label("Reset Label"); | |||
Label valueLabel = new Label("Value Label"); | |||
CheckBox delay = new CheckBox("Slow adding process", false); | |||
@Override | |||
protected void setup(VaadinRequest request) { | |||
initItems(); | |||
comboBox = new ComboBox(null, items); | |||
comboBox.setTextInputAllowed(true); | |||
comboBox.setEmptySelectionAllowed(true); | |||
comboBox.addValueChangeListener(event -> { | |||
valueLabel.setValue(comboBox.getValue()); | |||
log("ComboBox value : " + comboBox.getValue()); | |||
}); | |||
configureNewItemHandling(); | |||
Button checkButton = new Button("Check ComboBox value", e -> { | |||
Notification.show("selected: " + comboBox.getValue()); | |||
}); | |||
Button resetButton = new Button("Reset options", e -> { | |||
comboBox.setValue(null); | |||
initItems(); | |||
comboBox.getDataProvider().refreshAll(); | |||
resetLabel.setValue("Reset"); | |||
valueLabel.setValue("Value is reset"); | |||
log("DataProvider has been reset"); | |||
}); | |||
resetLabel.setId("reset-label"); | |||
valueLabel.setId("value-label"); | |||
delay.setId("delay"); | |||
resetButton.setId("reset"); | |||
Button button = new Button("Button for clicking only"); | |||
button.setId("button-for-click"); | |||
HorizontalLayout hl = new HorizontalLayout(checkButton, button); | |||
addComponents(comboBox, resetLabel, valueLabel, hl, resetButton, delay); | |||
} | |||
private void configureNewItemHandling() { | |||
comboBox.setNewItemProvider(text -> { | |||
if (delay.getValue()) { | |||
try { | |||
Thread.sleep(2000); | |||
} catch (InterruptedException e1) { | |||
e1.printStackTrace(); | |||
} | |||
} | |||
items.add(text); | |||
Collections.sort(items); | |||
comboBox.getDataProvider().refreshAll(); | |||
log("New item has been added"); | |||
return Optional.of(text); | |||
}); | |||
} | |||
private void initItems() { | |||
items.clear(); | |||
StringBuilder builder = new StringBuilder(); | |||
for (char c = 'a'; c <= 'z'; c++) { | |||
for (int i = 0; i < 100; i++) { | |||
builder.setLength(0); | |||
items.add(builder.append(c).append(i).toString()); | |||
} | |||
} | |||
} | |||
@Override | |||
protected Integer getTicketNumber() { | |||
return 11343; | |||
} | |||
} |
@@ -0,0 +1,155 @@ | |||
package com.vaadin.tests.components.combobox; | |||
import org.junit.Test; | |||
import org.openqa.selenium.Keys; | |||
import org.openqa.selenium.interactions.Actions; | |||
import com.vaadin.testbench.By; | |||
import com.vaadin.testbench.elements.ButtonElement; | |||
import com.vaadin.testbench.elements.CheckBoxElement; | |||
import com.vaadin.testbench.elements.ComboBoxElement; | |||
import com.vaadin.testbench.elements.LabelElement; | |||
import com.vaadin.tests.tb3.SingleBrowserTest; | |||
import static org.junit.Assert.assertTrue; | |||
public class ComboBoxAddNewItemAndResetProviderAtSameRoundTest | |||
extends SingleBrowserTest { | |||
protected enum SelectionType { | |||
ENTER, TAB, CLICK_OUT; | |||
} | |||
private ComboBoxElement comboBoxElement; | |||
private LabelElement valueLabelElement; | |||
private String inputValue = "000"; | |||
@Override | |||
public void setup() throws Exception { | |||
super.setup(); | |||
openTestURL(); | |||
waitForElementPresent(By.className("v-filterselect")); | |||
waitForElementPresent(By.id("reset-label")); | |||
waitForElementPresent(By.id("value-label")); | |||
comboBoxElement = $(ComboBoxElement.class).first(); | |||
} | |||
/** | |||
* Scenario: add new item and reset the data provider in the same round, | |||
* then add the same value again with ENTER | |||
*/ | |||
@Test | |||
public void addNewItemAndReset_reAddWithEnter() { | |||
itemHandling(SelectionType.ENTER, inputValue); | |||
} | |||
/** | |||
* Scenario: add new item and reset the data provider in the same round, | |||
* then add the same value again with TAB | |||
*/ | |||
@Test | |||
public void addNewItemAndReset_reAddWithTab() { | |||
itemHandling(SelectionType.TAB, inputValue); | |||
} | |||
/** | |||
* Scenario: add new item and reset the data provider in the same round, | |||
* then add the same value again with clicking out side of the CB | |||
*/ | |||
@Test | |||
public void addNewItemAndReset_reAddWithClickOut() { | |||
itemHandling(SelectionType.CLICK_OUT, inputValue); | |||
} | |||
/** | |||
* Scenario: add new item and reset the data provider in the same round with | |||
* 2 seconds delay, then add the same value again with ENTER | |||
*/ | |||
@Test | |||
public void slowAddNewItemAndReset_reAddWithEnter() { | |||
delay(true); | |||
itemHandling(SelectionType.ENTER, inputValue); | |||
} | |||
/** | |||
* Scenario: add new item and reset the data provider in the same round with | |||
* 2 seconds delay, then add the same value again with TAB | |||
*/ | |||
@Test | |||
public void slowAddNewItemAndReset_reAddWithTab() { | |||
delay(true); | |||
itemHandling(SelectionType.TAB, inputValue); | |||
} | |||
/** | |||
* Scenario: add new item and reset the data provider in the same round with | |||
* 2 seconds delay, then add the same value again with clicking out side | |||
*/ | |||
@Test | |||
public void slowAddNewItemAndReset_reAddWithClickOut() { | |||
delay(true); | |||
itemHandling(SelectionType.CLICK_OUT, inputValue); | |||
} | |||
private void itemHandling(SelectionType selectionType, String input) { | |||
assertValueLabelText("Value Label"); | |||
sendKeysToInput(input); | |||
sleep(1000); | |||
// reset the dataProvider | |||
reset(); | |||
// re-add the same value and select | |||
sendKeysToInput(input); | |||
performSelect(selectionType); | |||
assertLogMessage(); | |||
} | |||
private void assertLogMessage() { | |||
sleep(2000); | |||
//current test is not stable for collecting all the logs, | |||
//so that we need to do the assertion with full log and contents. | |||
assertTrue("The full log should contain the following text", | |||
getLogs().toString().contains("ComboBox value : 000")); | |||
assertTrue("The full log should contain the following text", | |||
getLogs().toString().contains("New item has been added")); | |||
assertTrue("The full log should contain the following text", | |||
getLogs().toString().contains("DataProvider has been reset")); | |||
} | |||
private void sendKeysToInput(CharSequence... keys) { | |||
new Actions(getDriver()).moveToElement(comboBoxElement).perform(); | |||
comboBoxElement.sendKeys(keys); | |||
} | |||
private void performSelect(SelectionType selectionType) { | |||
switch (selectionType) { | |||
case ENTER: | |||
sendKeysToInput(Keys.ENTER); | |||
break; | |||
case TAB: | |||
sendKeysToInput(Keys.TAB); | |||
break; | |||
case CLICK_OUT: | |||
$(ButtonElement.class).id("button-for-click").click(); | |||
break; | |||
} | |||
} | |||
private void assertValueLabelText(String value) { | |||
valueLabelElement = $(LabelElement.class).id("value-label"); | |||
waitUntil(driver -> value.equals(valueLabelElement.getText())); | |||
} | |||
private void delay(boolean delay) { | |||
CheckBoxElement checkBox = $(CheckBoxElement.class).id("delay"); | |||
if (delay != checkBox.isChecked()) { | |||
checkBox.click(); | |||
} | |||
} | |||
private void reset() { | |||
$(ButtonElement.class).id("reset").click(); | |||
} | |||
} |