From 545d8deef0ccedb460ded8a7ae75a5da5264fc02 Mon Sep 17 00:00:00 2001 From: Anna Koskinen Date: Tue, 30 Jan 2018 11:53:10 +0200 Subject: [PATCH] Selection handling fix when adding new item to ComboBox. (#10445) Fixes #10284 --- .../java/com/vaadin/client/ui/VComboBox.java | 39 ++- .../client/ui/combobox/ComboBoxConnector.java | 88 +++++++ .../src/main/java/com/vaadin/ui/ComboBox.java | 23 +- .../shared/ui/combobox/ComboBoxClientRpc.java | 34 +++ .../ComboBoxSelectingNewItemValueChange.java | 138 +++++++++++ ...mboBoxSelectingNewItemValueChangeTest.java | 231 ++++++++++++++++++ 6 files changed, 544 insertions(+), 9 deletions(-) create mode 100644 shared/src/main/java/com/vaadin/shared/ui/combobox/ComboBoxClientRpc.java create mode 100644 uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxSelectingNewItemValueChange.java create mode 100644 uitest/src/test/java/com/vaadin/tests/components/combobox/ComboBoxSelectingNewItemValueChangeTest.java diff --git a/client/src/main/java/com/vaadin/client/ui/VComboBox.java b/client/src/main/java/com/vaadin/client/ui/VComboBox.java index bd0c0809b4..863b56decc 100644 --- a/client/src/main/java/com/vaadin/client/ui/VComboBox.java +++ b/client/src/main/java/com/vaadin/client/ui/VComboBox.java @@ -1040,6 +1040,8 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, } }); + private String handledNewItem = null; + /** * Default constructor */ @@ -1134,7 +1136,11 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, public void actOnEnteredValueAfterFiltering(String enteredItemValue) { debug("VComboBox.SM: doPostFilterSelectedItemAction()"); final MenuItem item = getSelectedItem(); - + boolean handledOnServer = handledNewItem == enteredItemValue; + if (handledOnServer) { + // clear value to mark it as handled + handledNewItem = null; + } // check for exact match in menu int p = getItems().size(); if (p > 0) { @@ -1151,13 +1157,17 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, doItemAction(potentialExactMatch, true); } suggestionPopup.hide(); + lastNewItemString = null; + connector.clearNewItemHandlingIfMatch(enteredItemValue); return; } } } - if ("".equals(enteredItemValue) && nullSelectionAllowed) { + + if (!handledOnServer && "".equals(enteredItemValue) + && nullSelectionAllowed) { onNullSelected(); - } else if (allowNewItems) { + } else if (!handledOnServer && allowNewItems) { if (!enteredItemValue.equals(lastNewItemString)) { // Store last sent new item string to avoid double sends lastNewItemString = enteredItemValue; @@ -1182,6 +1192,10 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, } } suggestionPopup.hide(); + + if (handledOnServer || !allowNewItems) { + lastNewItemString = null; + } } private static final String SUBPART_PREFIX = "item"; @@ -1319,6 +1333,10 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, } } } + + public void markNewItemsHandled(String handledNewItem) { + this.handledNewItem = handledNewItem; + } } private String getSuggestionKey(MenuItem item) { @@ -1442,9 +1460,15 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, waitingForFilteringResponse = false; if (pendingUserInput != null) { + boolean pendingHandled = suggestionPopup.menu.handledNewItem == pendingUserInput; suggestionPopup.menu .actOnEnteredValueAfterFiltering(pendingUserInput); - pendingUserInput = null; + if (!allowNewItems || (pendingHandled + && suggestionPopup.menu.handledNewItem == null)) { + pendingUserInput = null; + } else { + waitingForFilteringResponse = true; + } } else if (popupOpenerClicked) { // make sure the current item is selected in the popup suggestionPopup.menu.highlightSelectedItem(); @@ -1474,6 +1498,10 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, filterOptions(0, value); } + public boolean isPending(String value) { + return value != null && value.equals(pendingUserInput); + } + /* * This method navigates to the proper item in the combobox page. This * should be executed after setSuggestions() method which is called from @@ -1541,7 +1569,6 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, */ public void serverReplyHandled() { popupOpenerClicked = false; - lastNewItemString = null; // if (!initDone) { // debug("VComboBox: init done, updating widths"); @@ -2542,7 +2569,7 @@ public class VComboBox extends Composite implements Field, KeyDownHandler, // Send new items when clicking out with the mouse. if (!readonly) { if (textInputEnabled && allowNewItems - && (currentSuggestion == null || tb.getText().equals( + && (currentSuggestion == null || !tb.getText().equals( currentSuggestion.getReplacementString()))) { dataReceivedHandler.reactOnInputWhenReady(tb.getText()); } else { diff --git a/client/src/main/java/com/vaadin/client/ui/combobox/ComboBoxConnector.java b/client/src/main/java/com/vaadin/client/ui/combobox/ComboBoxConnector.java index dd13e36205..334b630724 100644 --- a/client/src/main/java/com/vaadin/client/ui/combobox/ComboBoxConnector.java +++ b/client/src/main/java/com/vaadin/client/ui/combobox/ComboBoxConnector.java @@ -35,6 +35,7 @@ import com.vaadin.shared.communication.FieldRpc.FocusAndBlurServerRpc; import com.vaadin.shared.data.DataCommunicatorConstants; import com.vaadin.shared.data.selection.SelectionServerRpc; import com.vaadin.shared.ui.Connect; +import com.vaadin.shared.ui.combobox.ComboBoxClientRpc; import com.vaadin.shared.ui.combobox.ComboBoxConstants; import com.vaadin.shared.ui.combobox.ComboBoxServerRpc; import com.vaadin.shared.ui.combobox.ComboBoxState; @@ -55,10 +56,27 @@ public class ComboBoxConnector extends AbstractListingConnector private Registration dataChangeHandlerRegistration; + /** + * new item value that has been sent to server but selection handling hasn't + * been performed for it yet + */ + private String pendingNewItemValue = null; + @Override protected void init() { super.init(); getWidget().connector = this; + registerRpc(ComboBoxClientRpc.class, new ComboBoxClientRpc() { + @Override + public void newItemNotAdded(String itemValue) { + if (itemValue != null && itemValue.equals(pendingNewItemValue) + && isNewItemStillPending()) { + // handled but not added, perform (de-)selection handling + // immediately + completeNewItemHandling(); + } + } + }); } @Override @@ -158,6 +176,11 @@ public class ComboBoxConnector extends AbstractListingConnector * user entered string value for the new item */ public void sendNewItem(String itemValue) { + if (itemValue != null && !itemValue.equals(pendingNewItemValue)) { + // clear any previous handling as outdated + clearNewItemHandling(); + } + pendingNewItemValue = itemValue; rpc.createNewItem(itemValue); getDataReceivedHandler().clearPendingNavigation(); } @@ -180,6 +203,19 @@ public class ComboBoxConnector extends AbstractListingConnector } } + /** + * Confirm with the widget that the pending new item value is still pending. + * + * This method is for internal use only and may be removed in future + * versions. + * + * @return {@code true} if the value is still pending, {@code false} if + * there is no pending value or it doesn't match + */ + private boolean isNewItemStillPending() { + return getDataReceivedHandler().isPending(pendingNewItemValue); + } + /** * Send a message to the server to request a page of items with the current * filter. @@ -377,6 +413,58 @@ public class ComboBoxConnector extends AbstractListingConnector } } + /** + * If previous calls to refreshData haven't sorted out the selection yet, + * enforce it. + * + * This method is for internal use only and may be removed in future + * versions. + */ + private void completeNewItemHandling() { + // ensure the widget hasn't got a new selection in the meantime + if (isNewItemStillPending()) { + // mark new item for selection handling on the widget + getWidget().suggestionPopup.menu + .markNewItemsHandled(pendingNewItemValue); + // clear pending value + pendingNewItemValue = null; + // trigger the final selection handling + refreshData(); + } else { + clearNewItemHandling(); + } + } + + /** + * Clears the pending new item value if the widget's pending value no longer + * matches. + * + * This method is for internal use only and may be removed in future + * versions. + */ + private void clearNewItemHandling() { + // never clear pending value before it has been handled + if (!isNewItemStillPending()) { + pendingNewItemValue = null; + } + } + + /** + * Clears the new item handling variables if the given value matches the + * pending value. + * + * This method is for internal use only and may be removed in future + * versions. + * + * @param value + * already handled value + */ + public void clearNewItemHandlingIfMatch(String value) { + if (value != null && value.equals(pendingNewItemValue)) { + pendingNewItemValue = null; + } + } + private static final Logger LOGGER = Logger .getLogger(ComboBoxConnector.class.getName()); diff --git a/server/src/main/java/com/vaadin/ui/ComboBox.java b/server/src/main/java/com/vaadin/ui/ComboBox.java index 6c6c55e4fc..f8468d1acd 100644 --- a/server/src/main/java/com/vaadin/ui/ComboBox.java +++ b/server/src/main/java/com/vaadin/ui/ComboBox.java @@ -50,6 +50,7 @@ import com.vaadin.server.SerializableFunction; import com.vaadin.server.SerializableToIntFunction; import com.vaadin.shared.Registration; import com.vaadin.shared.data.DataCommunicatorConstants; +import com.vaadin.shared.ui.combobox.ComboBoxClientRpc; import com.vaadin.shared.ui.combobox.ComboBoxConstants; import com.vaadin.shared.ui.combobox.ComboBoxServerRpc; import com.vaadin.shared.ui.combobox.ComboBoxState; @@ -105,6 +106,16 @@ public class ComboBox extends AbstractSingleSelect /** * Handler that adds a new item based on user input when the new items * allowed mode is active. + *

+ * NOTE 1: If the new item is rejected the client must be notified of the + * fact via ComboBoxClientRpc or selection handling won't complete. + *

+ *

+ * NOTE 2: Selection handling must be completed separately if filtering the + * data source with the same value won't include the new item in the initial + * list of suggestions. Failing to do so will lead to selection handling + * never completing and previous selection remaining on the server. + *

* * @since 8.0 */ @@ -154,9 +165,15 @@ public class ComboBox extends AbstractSingleSelect @Override public void createNewItem(String itemValue) { // New option entered - if (getNewItemHandler() != null && itemValue != null - && !itemValue.isEmpty()) { - getNewItemHandler().accept(itemValue); + if (itemValue != null && !itemValue.isEmpty()) { + if (getNewItemHandler() != null) { + getNewItemHandler().accept(itemValue); + } else { + // selection handling is needed at the client even if + // NewItemHandler is missing + getRpcProxy(ComboBoxClientRpc.class) + .newItemNotAdded(itemValue); + } } } diff --git a/shared/src/main/java/com/vaadin/shared/ui/combobox/ComboBoxClientRpc.java b/shared/src/main/java/com/vaadin/shared/ui/combobox/ComboBoxClientRpc.java new file mode 100644 index 0000000000..51e949d0e1 --- /dev/null +++ b/shared/src/main/java/com/vaadin/shared/ui/combobox/ComboBoxClientRpc.java @@ -0,0 +1,34 @@ +/* + * Copyright 2000-2016 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.vaadin.shared.ui.combobox; + +import com.vaadin.shared.communication.ClientRpc; + +/** + * Server to client RPC interface for ComboBox. + * + * @since + */ +public interface ComboBoxClientRpc extends ClientRpc { + + /** + * Signal the client that attempt to add a new item failed. + * + * @param itemValue + * user entered string value for the new item + */ + public void newItemNotAdded(String itemValue); +} diff --git a/uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxSelectingNewItemValueChange.java b/uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxSelectingNewItemValueChange.java new file mode 100644 index 0000000000..a65e34be63 --- /dev/null +++ b/uitest/src/main/java/com/vaadin/tests/components/combobox/ComboBoxSelectingNewItemValueChange.java @@ -0,0 +1,138 @@ +package com.vaadin.tests.components.combobox; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; + +import com.vaadin.server.VaadinRequest; +import com.vaadin.shared.ui.ContentMode; +import com.vaadin.shared.ui.combobox.ComboBoxClientRpc; +import com.vaadin.ui.Button; +import com.vaadin.ui.CheckBox; +import com.vaadin.ui.ComboBox; +import com.vaadin.ui.Label; +import com.vaadin.ui.Notification; + +public class ComboBoxSelectingNewItemValueChange extends ComboBoxSelecting { + + private final class CustomComboBox extends ComboBox { + private CustomComboBox(String caption, Collection options) { + super(caption, options); + } + + public ComboBoxClientRpc getComboBoxClientRpc() { + return getRpcProxy(ComboBoxClientRpc.class); + } + } + + CustomComboBox comboBox; + List items = new ArrayList<>(); + int valueChangeEventCount = 0; + int selectionChangeEventCount = 0; + Label valueLabel = new Label(); + Label valueChangeLabel = new Label(null, ContentMode.HTML); + CheckBox delay = new CheckBox("Slow adding process", false); + CheckBox reject = new CheckBox("Reject new values", false); + CheckBox noSelection = new CheckBox("Don't select new values", false); + + @Override + protected void setup(VaadinRequest request) { + initItems(); + comboBox = new CustomComboBox(null, items); + comboBox.setTextInputAllowed(true); + comboBox.setEmptySelectionAllowed(true); + + comboBox.addValueChangeListener(event -> { + String value = event.getValue(); + if (value != null) { + valueLabel.setValue(value); + } else { + valueLabel.setValue("null"); + } + }); + + comboBox.setNewItemHandler(text -> { + if (Boolean.TRUE.equals(delay.getValue())) { + try { + Thread.sleep(2000); + } catch (InterruptedException e1) { + e1.printStackTrace(); + } + } + if (Boolean.TRUE.equals(reject.getValue())) { + valueChangeLabel.setValue("item " + text + " discarded"); + comboBox.getComboBoxClientRpc().newItemNotAdded(text); + } else { + items.add(text); + Collections.sort(items); + valueChangeLabel + .setValue("adding new item... count: " + items.size()); + if (Boolean.TRUE.equals(noSelection.getValue())) { + comboBox.getComboBoxClientRpc().newItemNotAdded(text); + } + comboBox.getDataProvider().refreshAll(); + } + }); + + comboBox.addValueChangeListener(e -> { + ++valueChangeEventCount; + updateLabel(e.isUserOriginated()); + }); + + comboBox.addSelectionListener(e -> { + ++selectionChangeEventCount; + updateLabel(e.isUserOriginated()); + }); + + 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(); + valueLabel.setValue(""); + valueChangeLabel.setValue("Reset"); + valueChangeEventCount = 0; + selectionChangeEventCount = 0; + }); + + valueLabel.setId("value"); + valueChangeLabel.setId("change"); + delay.setId("delay"); + reject.setId("reject"); + noSelection.setId("noSelection"); + resetButton.setId("reset"); + + addComponents(comboBox, valueLabel, valueChangeLabel, checkButton, + resetButton, delay, reject, noSelection); + } + + private void initItems() { + items.clear(); + for (char c = 'a'; c <= 'z'; c++) { + for (int i = 0; i < 100; i++) { + items.add("" + c + i); + } + } + } + + private void updateLabel(boolean userOriginated) { + valueChangeLabel.setValue("Value change count: " + valueChangeEventCount + + "\nSelection change count: " + selectionChangeEventCount + + "\nuser originated: " + userOriginated); + } + + @Override + protected String getTestDescription() { + return "New item should trigger value change when accepted " + + "and restore the field to previous value when rejected."; + } + + @Override + protected Integer getTicketNumber() { + return 10284; + } +} diff --git a/uitest/src/test/java/com/vaadin/tests/components/combobox/ComboBoxSelectingNewItemValueChangeTest.java b/uitest/src/test/java/com/vaadin/tests/components/combobox/ComboBoxSelectingNewItemValueChangeTest.java new file mode 100644 index 0000000000..581d546397 --- /dev/null +++ b/uitest/src/test/java/com/vaadin/tests/components/combobox/ComboBoxSelectingNewItemValueChangeTest.java @@ -0,0 +1,231 @@ +package com.vaadin.tests.components.combobox; + +import static org.junit.Assert.assertTrue; + +import org.junit.Test; +import org.openqa.selenium.Keys; +import org.openqa.selenium.WebDriver; +import org.openqa.selenium.interactions.Actions; +import org.openqa.selenium.support.ui.ExpectedCondition; + +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.testbench.parallel.BrowserUtil; +import com.vaadin.tests.tb3.MultiBrowserTest; + +public class ComboBoxSelectingNewItemValueChangeTest extends MultiBrowserTest { + + private enum SelectionType { + ENTER, TAB, CLICK_OUT; + } + + private ComboBoxElement comboBoxElement; + private LabelElement valueLabelElement; + private LabelElement changeLabelElement; + private String[] defaultInputs = new String[] { "foo", "bar", "baz", + "fie" }; + private String[] shortInputs = new String[] { "a", "b", "c", "d" }; + + @Override + public void setup() throws Exception { + super.setup(); + + openTestURL(); + waitForElementPresent(By.className("v-filterselect")); + comboBoxElement = $(ComboBoxElement.class).first(); + valueLabelElement = $(LabelElement.class).id("value"); + changeLabelElement = $(LabelElement.class).id("change"); + } + + @Test + public void newItemHandlingWithEnter() { + itemHandling(SelectionType.ENTER, defaultInputs); + } + + @Test + public void newItemHandlingWithTab() { + itemHandling(SelectionType.TAB, defaultInputs); + } + + @Test + public void newItemHandlingWithClickingOut() { + itemHandling(SelectionType.CLICK_OUT, defaultInputs); + } + + @Test + public void slowNewItemHandlingWithEnter() { + delay(true); + itemHandling(SelectionType.ENTER, defaultInputs); + } + + @Test + public void slowNewItemHandlingWithTab() { + delay(true); + itemHandling(SelectionType.TAB, defaultInputs); + } + + @Test + public void slowNewItemHandlingWithClickingOut() { + delay(true); + itemHandling(SelectionType.CLICK_OUT, defaultInputs); + } + + @Test + public void shortNewItemHandlingWithEnter() { + itemHandling(SelectionType.ENTER, shortInputs); + } + + @Test + public void shortNewItemHandlingWithTab() { + itemHandling(SelectionType.TAB, shortInputs); + } + + @Test + public void shortNewItemHandlingWithClickingOut() { + itemHandling(SelectionType.CLICK_OUT, shortInputs); + } + + public void itemHandling(SelectionType selectionType, String[] inputs) { + assertThatSelectedValueIs(""); + + // new item, no existing selection + typeInputAndSelect(inputs[0], selectionType); + assertThatSelectedValueIs(inputs[0]); + assertValueChange(1); + + // new item, existing selection + typeInputAndSelect(inputs[1], selectionType); + assertThatSelectedValueIs(inputs[1]); + assertValueChange(2); + + reject(true); + + // item adding blocked, existing selection + typeInputAndSelect(inputs[2], selectionType); + assertThatSelectedValueIs(inputs[1]); + assertRejected(inputs[2]); + + reset(); + + // item adding blocked, no existing selection + typeInputAndSelect(inputs[2], selectionType); + assertThatSelectedValueIs(""); + assertRejected(inputs[2]); + + reject(false); + blockSelection(true); + + // item adding allowed, selection blocked, no existing selection + typeInputAndSelect(inputs[2], selectionType); + assertThatSelectedValueIs(""); + assertItemCount(2601); + + // second attempt selects + typeInputAndSelect(inputs[2], selectionType); + assertThatSelectedValueIs(inputs[2]); + assertValueChange(1); + + // item adding allowed, selection blocked, existing selection + typeInputAndSelect(inputs[3], selectionType); + assertThatSelectedValueIs(inputs[2]); + assertItemCount(2602); + } + + private void typeInputAndSelect(String input, SelectionType selectionType) { + comboBoxElement.clear(); + sendKeysToInput(input); + switch (selectionType) { + case ENTER: + sendKeysToInput(getReturn()); + break; + case TAB: + sendKeysToInput(Keys.TAB); + break; + case CLICK_OUT: + new Actions(getDriver()).moveToElement(comboBoxElement, 10, 10) + .moveByOffset(comboBoxElement.getSize().getWidth(), 0) + .click().perform(); + break; + } + } + + private void sendKeysToInput(CharSequence... keys) { + comboBoxElement.sendKeys(keys); + } + + private Keys getReturn() { + if (BrowserUtil.isPhantomJS(getDesiredCapabilities())) { + return Keys.ENTER; + } else { + return Keys.RETURN; + } + } + + private void assertThatSelectedValueIs(final String value) { + waitUntil(new ExpectedCondition() { + private String actualComboBoxValue; + private String actualLabelValue; + + @Override + public Boolean apply(WebDriver input) { + actualLabelValue = valueLabelElement.getText(); + actualComboBoxValue = comboBoxElement.getText(); + return actualComboBoxValue.equals(value) + && actualLabelValue.equals(value); + } + + @Override + public String toString() { + // Timed out after 10 seconds waiting for ... + return String.format( + "combobox and label value to match '%s' (was: '%s' and '%s')", + value, actualComboBoxValue, actualLabelValue); + } + }); + } + + private void assertValueChange(int count) { + assertTrue(changeLabelElement.getText().equals(String.format( + "Value change count: %s Selection change count: %s user originated: true", + count, count))); + } + + private void assertRejected(String value) { + assertTrue(changeLabelElement.getText() + .equals(String.format("item %s discarded", value))); + } + + private void assertItemCount(int count) { + assertTrue(changeLabelElement.getText() + .equals(String.format("adding new item... count: %s", count))); + } + + private void reject(boolean reject) { + CheckBoxElement checkBox = $(CheckBoxElement.class).id("reject"); + if (reject != checkBox.isChecked()) { + checkBox.click(); + } + } + + private void delay(boolean delay) { + CheckBoxElement checkBox = $(CheckBoxElement.class).id("delay"); + if (delay != checkBox.isChecked()) { + checkBox.click(); + } + } + + private void blockSelection(boolean noSelection) { + CheckBoxElement checkBox = $(CheckBoxElement.class).id("noSelection"); + if (noSelection != checkBox.isChecked()) { + checkBox.click(); + } + } + + private void reset() { + $(ButtonElement.class).id("reset").click(); + } + +} -- 2.39.5