Ensure wrong Action is not executed after detaching a component (#9806)

Due to action key generation on ActionManager a wrong action
may be executed if the component that fired the wanted action
is already detached.

This patch makes action keys globally unique, simplifying the
approach of #8495 but reusing its tests.

Fixes #5864

5 files changed, 274 insertions, 13 deletions

diff --git a/server/src/main/java/com/vaadin/event/ActionManager.java b/server/src/main/java/com/vaadin/event/ActionManager.java
index 1056106b16..fbc5c8411f 100644
--- a/server/src/main/java/com/vaadin/event/ActionManager.java
+++ b/server/src/main/java/com/vaadin/event/ActionManager.java
@@ -19,6 +19,7 @@ import java.util.Arrays;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import com.vaadin.event.Action.Container;
 import com.vaadin.event.Action.Handler;
@@ -166,14 +167,16 @@ public class ActionManager
          * removed but still exist on client side
          */
         if (!actions.isEmpty() || clientHasActions) {
-            actionMapper = new KeyMapper<>();
+            actionMapper = new ActionKeyMapper();
 
             paintTarget.addVariable((VariableOwner) viewer, "action", "");
             paintTarget.startTag("actions");
 
             for (final Action a : actions) {
+
                 paintTarget.startTag("action");
                 final String akey = actionMapper.key(a);
+
                 paintTarget.addAttribute("key", akey);
                 if (a.getCaption() != null) {
                     paintTarget.addAttribute("caption", a.getCaption());
@@ -244,7 +247,6 @@ public class ActionManager
         LinkedHashSet<Action> actions = new LinkedHashSet<>();
         if (ownActions != null) {
             actions.addAll(ownActions);
-
         }
         if (actionHandlers != null) {
             for (Action.Handler h : actionHandlers) {
@@ -256,4 +258,20 @@ public class ActionManager
         }
         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 f7b878dd27..d20eb7c605 100644
--- a/server/src/main/java/com/vaadin/server/KeyMapper.java
+++ b/server/src/main/java/com/vaadin/server/KeyMapper.java
@@ -43,9 +43,11 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
     /**
      * Constructs a new mapper
      *
-     * @param identifierGetter has to return a unique key for every bean, and the returned key has to
-     *                         follow general {@code hashCode()} and {@code equals()} contract,
-     *                         see {@link Object#hashCode()} for details.
+     * @param identifierGetter
+     *            has to return a unique key for every bean, and the returned
+     *            key has to follow general {@code hashCode()} and
+     *            {@code equals()} contract, see {@link Object#hashCode()} for
+     *            details.
      * @since 8.1
      */
     public KeyMapper(ValueProvider<V, Object> identifierGetter) {
@@ -62,7 +64,8 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
     /**
      * Gets key for an object.
      *
-     * @param o the object.
+     * @param o
+     *            the object.
      */
     @Override
     public String key(V o) {
@@ -79,13 +82,25 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
         }
 
         // If the object is not yet mapped, map it
-        key = String.valueOf(++lastKey);
+        key = createKey();
         objectIdKeyMap.put(id, key);
         keyObjectMap.put(key, o);
 
         return key;
     }
 
+    /**
+     * Creates a key for a new item.
+     *
+     * This method can be overridden to customize the keys used.
+     *
+     * @return new key
+     * @since 8.1.2
+     */
+    protected String createKey() {
+        return String.valueOf(++lastKey);
+    }
+
     @Override
     public boolean has(V o) {
         return objectIdKeyMap.containsKey(identifierGetter.apply(o));
@@ -94,7 +109,8 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
     /**
      * Retrieves object with the key.
      *
-     * @param key the name with the desired value.
+     * @param key
+     *            the name with the desired value.
      * @return the object with the key.
      */
     @Override
@@ -105,11 +121,13 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
     /**
      * Removes object from the mapper.
      *
-     * @param removeobj the object to be removed.
+     * @param removeobj
+     *            the object to be removed.
      */
     @Override
     public void remove(V removeobj) {
-        final String key = objectIdKeyMap.remove(identifierGetter.apply(removeobj));
+        final String key = objectIdKeyMap
+                .remove(identifierGetter.apply(removeobj));
         if (key != null) {
             keyObjectMap.remove(key);
         }
@@ -127,9 +145,10 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
     /**
      * Checks if the given key is mapped to an object.
      *
-     * @param key the key to check
+     * @param key
+     *            the key to check
      * @return <code>true</code> if the key is currently mapped,
-     * <code>false</code> otherwise
+     *         <code>false</code> otherwise
      * @since 7.7
      */
     public boolean containsKey(String key) {
@@ -151,7 +170,8 @@ public class KeyMapper<V> implements DataKeyMapper<V>, Serializable {
             this.identifierGetter = identifierGetter;
             objectIdKeyMap.clear();
             for (Map.Entry<String, V> entry : keyObjectMap.entrySet()) {
-                objectIdKeyMap.put(identifierGetter.apply(entry.getValue()),entry.getKey());
+                objectIdKeyMap.put(identifierGetter.apply(entry.getValue()),
+                        entry.getKey());
             }
         }
     }
diff --git a/uitest/src/main/java/com/vaadin/tests/actions/ActionsOnDetachedComponents.java b/uitest/src/main/java/com/vaadin/tests/actions/ActionsOnDetachedComponents.java
new file mode 100644
index 0000000000..085f5f9efe
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/actions/ActionsOnDetachedComponents.java
@@ -0,0 +1,131 @@
+/*
+ * 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.tests.actions;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import com.vaadin.event.Action;
+import com.vaadin.event.ShortcutAction;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUIWithLog;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Label;
+import com.vaadin.ui.Panel;
+import com.vaadin.ui.VerticalLayout;
+import com.vaadin.v7.ui.Table;
+
+/**
+ * @author Vaadin Ltd
+ */
+public class ActionsOnDetachedComponents extends AbstractTestUIWithLog {
+
+    private final AtomicInteger clickCounter = new AtomicInteger();
+    private Panel mainLayout;
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        clickCounter.set(0);
+        mainLayout = new Panel();
+        mainLayout.setSizeFull();
+        mainLayout.setContent(new ShortCutALayer());
+        addComponent(mainLayout);
+
+    }
+
+    private Table tableWithActions(final LayerSwitcher switcher) {
+        Table table = new Table();
+        table.addContainerProperty("id", Integer.class, 0);
+        table.addItems(1, 2, 3, 4);
+        table.addActionHandler(new Action.Handler() {
+
+            Action action = new Action("Table action");
+
+            @Override
+            public Action[] getActions(Object target, Object sender) {
+                return new Action[] { action };
+            }
+
+            @Override
+            public void handleAction(Action action, Object sender,
+                    Object target) {
+                if (action == this.action) {
+                    log("Fired action for tableAction");
+                    switcher.switchLayers();
+                }
+            }
+        });
+        return table;
+    }
+
+    private interface LayerSwitcher {
+        void switchLayers();
+    }
+
+    private class ShortCutALayer extends VerticalLayout
+            implements LayerSwitcher {
+        public ShortCutALayer() {
+            setId("layer-A");
+            Label l = new Label(getClass().getSimpleName());
+            Button b = new Button("click here or press 'a'");
+            b.setId("btn-A");
+            b.setClickShortcut(ShortcutAction.KeyCode.A);
+            b.addClickListener(event -> {
+                log("Fired action for btn-A");
+                switchLayers();
+            });
+            addComponents(l, b);
+            Table table = tableWithActions(this);
+            addComponent(table);
+        }
+
+        @Override
+        public void switchLayers() {
+            try {
+                Thread.sleep(1000); // do something important
+
+            } catch (InterruptedException e) {
+            }
+            mainLayout.setContent(new ShortCutBLayer());
+        }
+    }
+
+    private class ShortCutBLayer extends VerticalLayout
+            implements LayerSwitcher {
+        public ShortCutBLayer() {
+            setId("layer-B");
+            Label l = new Label(getClass().getSimpleName());
+            Button b = new Button("click here or press 'b'");
+            b.setId("btn-B");
+            b.setClickShortcut(ShortcutAction.KeyCode.B);
+            b.addClickListener(event -> {
+                log("Fired action for btn-B");
+                switchLayers();
+            });
+            addComponents(l, b);
+            Table table = tableWithActions(this);
+            addComponent(table);
+        }
+
+        @Override
+        public void switchLayers() {
+            try {
+                Thread.sleep(1000); // do something important
+            } catch (InterruptedException e) {
+            }
+            mainLayout.setContent(new ShortCutALayer());
+        }
+    }
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/actions/ActionsOnDetachedComponentsTest.java b/uitest/src/test/java/com/vaadin/tests/actions/ActionsOnDetachedComponentsTest.java
new file mode 100644
index 0000000000..9410dfecba
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/actions/ActionsOnDetachedComponentsTest.java
@@ -0,0 +1,90 @@
+/*
+ * 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.tests.actions;
+
+import static org.hamcrest.CoreMatchers.endsWith;
+import static org.hamcrest.CoreMatchers.not;
+import static org.junit.Assert.assertThat;
+
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.openqa.selenium.WebElement;
+import org.openqa.selenium.interactions.Actions;
+import org.openqa.selenium.remote.DesiredCapabilities;
+
+import com.vaadin.testbench.By;
+import com.vaadin.testbench.elements.TableElement;
+import com.vaadin.testbench.parallel.BrowserUtil;
+import com.vaadin.tests.tb3.MultiBrowserTest;
+
+/**
+ * @author Vaadin Ltd
+ */
+public class ActionsOnDetachedComponentsTest extends MultiBrowserTest {
+
+    @Before
+    public void init() {
+        openTestURL();
+        if (BrowserUtil.isFirefox(getDesiredCapabilities())) {
+            // focus the page to make shortcuts go to the right place
+            getDriver().findElement(By.className("v-app")).click();
+        }
+    }
+
+    @Override
+    public List<DesiredCapabilities> getBrowsersToTest() {
+        return getBrowsersSupportingContextMenu();
+    }
+
+    @Test
+    public void shortcutActionOnDetachedComponentShouldNotBeHandled()
+            throws InterruptedException {
+
+        Actions k = new Actions(driver);
+        k.sendKeys("a").perform();
+        k.sendKeys("a").perform();
+        sleep(500);
+
+        assertElementNotPresent(By.id("layer-A"));
+        assertElementPresent(By.id("layer-B"));
+        assertThat(getLogRow(0), endsWith("btn-A"));
+        assertThat(getLogRow(1), not(endsWith("btn-B")));
+
+    }
+
+    @Test
+    public void actionOnDetachedComponentShouldNotBeHandled()
+            throws InterruptedException {
+        TableElement table = $(TableElement.class).first();
+        table.getRow(0).contextClick();
+        // Find the opened menu
+        WebElement menu = findElement(By.className("v-contextmenu"));
+        WebElement menuitem = menu
+                .findElement(By.xpath("//*[text() = 'Table action']"));
+
+        Actions doubleClick = new Actions(getDriver());
+        doubleClick.doubleClick(menuitem).build().perform();
+
+        assertElementNotPresent(By.id("layer-A"));
+        assertElementPresent(By.id("layer-B"));
+        assertThat(getLogRow(0), endsWith("tableAction"));
+        assertThat(getLogRow(1), not(endsWith("tableAction")));
+
+    }
+
+}
diff --git a/uitest/src/test/java/com/vaadin/tests/components/ui/WindowAndUIShortcutsTest.java b/uitest/src/test/java/com/vaadin/tests/components/ui/WindowAndUIShortcutsTest.java
index 7c14ae0864..715025bf5e 100644
--- a/uitest/src/test/java/com/vaadin/tests/components/ui/WindowAndUIShortcutsTest.java
+++ b/uitest/src/test/java/com/vaadin/tests/components/ui/WindowAndUIShortcutsTest.java
@@ -35,6 +35,8 @@ public class WindowAndUIShortcutsTest extends SingleBrowserTest {
         $(ButtonElement.class).caption("Open dialog window").first().click();
 
         WindowElement window = $(WindowElement.class).first();
+        // for PhantomJS to have the focus in the right place
+        window.click();
         window.$(TextFieldElement.class).first().sendKeys(Keys.ESCAPE);
 
         // Window should have been closed