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

3 files changed, 223 insertions, 0 deletions

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