Refactor the usage of KeyMapper in ActionManager. (#11265)

* Add Test for closing window with focused textfield

verify issue #10642

* Refactor the code about using keyMapper

4 files changed, 108 insertions, 21 deletions

diff --git a/server/src/main/java/com/vaadin/event/ActionManager.java b/server/src/main/java/com/vaadin/event/ActionManager.java
index 7ae89950b0..b695822305 100644
--- a/server/src/main/java/com/vaadin/event/ActionManager.java
+++ b/server/src/main/java/com/vaadin/event/ActionManager.java
@@ -56,7 +56,7 @@ public class ActionManager implements Action.Handler, Action.Notifier {
     protected HashSet<Handler> actionHandlers = null;
 
     /** Action mapper. */
-    protected KeyMapper<Action> actionMapper = null;
+    protected KeyMapper<Action> actionMapper;
 
     protected Component viewer;
 
@@ -157,16 +157,19 @@ public class ActionManager implements Action.Handler, Action.Notifier {
     public void paintActions(Object actionTarget, PaintTarget paintTarget)
             throws PaintException {
 
-        actionMapper = null;
-
         LinkedHashSet<Action> actions = getActionSet(actionTarget, viewer);
 
+        if (actionMapper == null) {
+            actionMapper = new KeyMapper<>();
+        }
+
+        actionMapper.merge(actions);
+
         /*
          * Must repaint whenever there are actions OR if all actions have been
          * removed but still exist on client side
          */
         if (!actions.isEmpty() || clientHasActions) {
-            actionMapper = new ActionKeyMapper();
 
             paintTarget.addVariable((VariableOwner) viewer, "action", "");
             paintTarget.startTag("actions");
@@ -215,6 +218,7 @@ public class ActionManager implements Action.Handler, Action.Notifier {
             final String key = (String) variables.get("action");
             final Action action = actionMapper.get(key);
             final Object target = variables.get("actiontarget");
+
             if (action != null) {
                 handleAction(action, sender, target);
             }
@@ -257,20 +261,4 @@ public class ActionManager implements Action.Handler, Action.Notifier {
         }
         return actions;
     }
-
-    /**
-     * Extension of KeyMapper that avoids reusing keys even across different
-     * instances.
-     *
-     * @since 8.1.2
-     */
-    private static class ActionKeyMapper extends KeyMapper<Action> {
-        private static AtomicInteger lastKey = new AtomicInteger(0);
-
-        @Override
-        protected String createKey() {
-            return String.valueOf(lastKey.incrementAndGet());
-        }
-    }
-
 }
diff --git a/server/src/main/java/com/vaadin/server/KeyMapper.java b/server/src/main/java/com/vaadin/server/KeyMapper.java
index 9283b373e7..cb43bfdaa5 100644
--- a/server/src/main/java/com/vaadin/server/KeyMapper.java
+++ b/server/src/main/java/com/vaadin/server/KeyMapper.java
@@ -16,11 +16,17 @@
 
 package com.vaadin.server;
 
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
+import java.util.Set;
+import java.util.function.Predicate;
 
 import com.vaadin.data.ValueProvider;
 import com.vaadin.data.provider.DataKeyMapper;
+import com.vaadin.event.Action;
 
 /**
  * <code>KeyMapper</code> is the simple two-way map for generating textual keys
@@ -90,7 +96,7 @@ public class KeyMapper<V> implements DataKeyMapper<V> {
 
     /**
      * Creates a key for a new item.
-     *
+     * <p>
      * This method can be overridden to customize the keys used.
      *
      * @return new key
@@ -142,6 +148,33 @@ public class KeyMapper<V> implements DataKeyMapper<V> {
     }
 
     /**
+     * Merge Objects into the mapper.
+     * <p>
+     * This method will add the new objects to the mapper and remove inactive
+     * objects from it.
+     *
+     * @param objects
+     *            new objects set needs to be merged.
+     * @since
+     */
+    public void merge(Set<V> objects) {
+        final Set<String> keys = new HashSet<>(keyObjectMap.size());
+
+        for (V object : objects) {
+            if (object == null) {
+                continue;
+            }
+            String key = key(object);
+            keys.add(key);
+        }
+
+        keyObjectMap.entrySet()
+                .removeIf(entry -> !keys.contains(entry.getKey()));
+        objectIdKeyMap.entrySet()
+                .removeIf(entry -> !keys.contains(entry.getValue()));
+    }
+
+    /**
      * Checks if the given key is mapped to an object.
      *
      * @param key
diff --git a/uitest/src/main/java/com/vaadin/tests/components/window/CloseWindowWithFocusedTextField.java b/uitest/src/main/java/com/vaadin/tests/components/window/CloseWindowWithFocusedTextField.java
new file mode 100644
index 0000000000..2c0b2e4acf
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/window/CloseWindowWithFocusedTextField.java
@@ -0,0 +1,37 @@
+package com.vaadin.tests.components.window;
+
+import com.vaadin.event.ShortcutAction;
+import com.vaadin.event.ShortcutListener;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.shared.ui.ValueChangeMode;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Label;
+import com.vaadin.ui.TextField;
+import com.vaadin.ui.VerticalLayout;
+import com.vaadin.ui.Window;
+
+public class CloseWindowWithFocusedTextField extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        final VerticalLayout layout = new VerticalLayout();
+
+        Button button1 = new Button("open window");
+        button1.addClickListener(event -> {
+
+            Window window = new Window();
+            window.setModal(true);
+
+            TextField textField = new TextField("focus me");
+            textField.focus();
+
+            window.setContent(textField);
+            getUI().addWindow(window);
+        });
+
+        layout.addComponents(button1);
+
+        addComponent(layout);
+    }
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/window/CloseWindowWithFocusedTextFieldTest.java b/uitest/src/test/java/com/vaadin/tests/components/window/CloseWindowWithFocusedTextFieldTest.java
new file mode 100644
index 0000000000..898e15ce51
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/window/CloseWindowWithFocusedTextFieldTest.java
@@ -0,0 +1,29 @@
+package com.vaadin.tests.components.window;
+
+import org.junit.Test;
+import org.openqa.selenium.Keys;
+import org.openqa.selenium.interactions.Actions;
+
+import com.vaadin.testbench.annotations.RunLocally;
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.WindowElement;
+import com.vaadin.testbench.parallel.Browser;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class CloseWindowWithFocusedTextFieldTest extends MultiBrowserTest {
+
+    @Test
+    public void OpenWindow_CloseWithEscapeKey_WindowClosed() {
+        openTestURL();
+        $(ButtonElement.class).first().click();
+        assertTrue("Window should be opened", $(WindowElement.class).exists());
+
+        new Actions(getDriver()).sendKeys(Keys.ESCAPE).build().perform();
+        assertFalse("Window found when there should be none.",
+                $(WindowElement.class).exists());
+    }
+
+}