Validate that the connector is enabled before triggering actions for it (#12743)

Automated test enabled only for IE9-IE11 because of #12785

Change-Id: I265e5d1ead3fa56469861c5a98dcc9d0106d1051

6 files changed, 238 insertions, 12 deletions

diff --git a/server/src/com/vaadin/event/ConnectorActionManager.java b/server/src/com/vaadin/event/ConnectorActionManager.java
new file mode 100644
index 0000000000..297f78f179
--- /dev/null
+++ b/server/src/com/vaadin/event/ConnectorActionManager.java
@@ -0,0 +1,88 @@
+/*
+ * Copyright 2000-2013 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.event;
+
+import java.util.logging.Logger;
+
+import com.vaadin.event.Action.Container;
+import com.vaadin.server.ClientConnector;
+import com.vaadin.server.VariableOwner;
+import com.vaadin.server.communication.ServerRpcHandler;
+import com.vaadin.ui.Component;
+
+/**
+ * An ActionManager connected to a connector. Takes care of verifying that the
+ * connector can receive events before triggering an action.
+ * <p>
+ * This is mostly a workaround until shortcut actions are re-implemented in a
+ * more sensible way.
+ * 
+ * @since 7.1.8
+ * @author Vaadin Ltd
+ */
+public class ConnectorActionManager extends ActionManager {
+
+    private ClientConnector connector;
+
+    /**
+     * Initialize an action manager for the given connector.
+     * 
+     * @param connector
+     *            the owner of this action manager
+     */
+    public ConnectorActionManager(ClientConnector connector) {
+        super();
+        this.connector = connector;
+    }
+
+    /**
+     * Initialize an action manager for the given connector using the given
+     * viewer.
+     * 
+     * @param connector
+     *            the owner of this action manager
+     * @param viewer
+     *            the viewer connected
+     */
+    public <T extends Component & Container & VariableOwner> ConnectorActionManager(
+            ClientConnector connector, T viewer) {
+        super(viewer);
+        this.connector = connector;
+    }
+
+    /*
+     * (non-Javadoc)
+     * 
+     * @see com.vaadin.event.ActionManager#handleAction(com.vaadin.event.Action,
+     * java.lang.Object, java.lang.Object)
+     */
+    @Override
+    public void handleAction(Action action, Object sender, Object target) {
+        if (!connector.isConnectorEnabled()) {
+            getLogger().warning(
+                    ServerRpcHandler.getIgnoredDisabledError("action",
+                            connector));
+            return;
+        }
+
+        super.handleAction(action, sender, target);
+    }
+
+    private static final Logger getLogger() {
+        return Logger.getLogger(ConnectorActionManager.class.getName());
+    }
+
+}
diff --git a/server/src/com/vaadin/server/communication/ServerRpcHandler.java b/server/src/com/vaadin/server/communication/ServerRpcHandler.java
index 3cc85909ee..f14d703454 100644
--- a/server/src/com/vaadin/server/communication/ServerRpcHandler.java
+++ b/server/src/com/vaadin/server/communication/ServerRpcHandler.java
@@ -182,15 +182,8 @@ public class ServerRpcHandler implements Serializable {
                     }
 
                     // Connector is disabled, log a warning and move to the next
-                    String msg = "Ignoring RPC call for disabled connector "
-                            + connector.getClass().getName();
-                    if (connector instanceof Component) {
-                        String caption = ((Component) connector).getCaption();
-                        if (caption != null) {
-                            msg += ", caption=" + caption;
-                        }
-                    }
-                    getLogger().warning(msg);
+                    getLogger().warning(
+                            getIgnoredDisabledError("RPC call", connector));
                     continue;
                 }
                 // DragAndDropService has null UI
@@ -466,4 +459,26 @@ public class ServerRpcHandler implements Serializable {
     private static final Logger getLogger() {
         return Logger.getLogger(ServerRpcHandler.class.getName());
     }
+
+    /**
+     * Generates an error message when the client is trying to to something
+     * ('what') with a connector which is disabled or invisible.
+     * 
+     * @since 7.1.8
+     * @param connector
+     *            the connector which is disabled (or invisible)
+     * @return an error message
+     */
+    public static String getIgnoredDisabledError(String what,
+            ClientConnector connector) {
+        String msg = "Ignoring " + what + " for disabled connector "
+                + connector.getClass().getName();
+        if (connector instanceof Component) {
+            String caption = ((Component) connector).getCaption();
+            if (caption != null) {
+                msg += ", caption=" + caption;
+            }
+        }
+        return msg;
+    }
 }
diff --git a/server/src/com/vaadin/ui/AbstractComponent.java b/server/src/com/vaadin/ui/AbstractComponent.java
index 262d47af18..61bcf00ad8 100644
--- a/server/src/com/vaadin/ui/AbstractComponent.java
+++ b/server/src/com/vaadin/ui/AbstractComponent.java
@@ -27,6 +27,7 @@ import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import com.vaadin.event.ActionManager;
+import com.vaadin.event.ConnectorActionManager;
 import com.vaadin.event.ShortcutListener;
 import com.vaadin.server.AbstractClientConnector;
 import com.vaadin.server.ComponentSizeValidator;
@@ -90,7 +91,7 @@ public abstract class AbstractComponent extends AbstractClientConnector
      * Keeps track of the Actions added to this component; the actual
      * handling/notifying is delegated, usually to the containing window.
      */
-    private ActionManager actionManager;
+    private ConnectorActionManager actionManager;
 
     private boolean visible = true;
 
@@ -929,7 +930,7 @@ public abstract class AbstractComponent extends AbstractClientConnector
      */
     protected ActionManager getActionManager() {
         if (actionManager == null) {
-            actionManager = new ActionManager();
+            actionManager = new ConnectorActionManager(this);
             setActionManagerViewer();
         }
         return actionManager;
diff --git a/server/src/com/vaadin/ui/Button.java b/server/src/com/vaadin/ui/Button.java
index 1bcf802f12..589f30d631 100644
--- a/server/src/com/vaadin/ui/Button.java
+++ b/server/src/com/vaadin/ui/Button.java
@@ -340,7 +340,7 @@ public class Button extends AbstractComponent implements
      * No action is taken is the button is disabled.
      */
     public void click() {
-        if (isEnabled() && !isReadOnly()) {
+        if (isConnectorEnabled() && !isReadOnly()) {
             fireClick();
         }
     }
diff --git a/uitest/src/com/vaadin/tests/actions/ActionsOnInvisibleComponents.java b/uitest/src/com/vaadin/tests/actions/ActionsOnInvisibleComponents.java
new file mode 100644
index 0000000000..0f4c179cee
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/actions/ActionsOnInvisibleComponents.java
@@ -0,0 +1,76 @@
+package com.vaadin.tests.actions;
+
+import com.vaadin.event.ShortcutAction.KeyCode;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUIWithLog;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Button.ClickEvent;
+
+public class ActionsOnInvisibleComponents extends AbstractTestUIWithLog {
+
+    private static final long serialVersionUID = -5993467736906948993L;
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        getContent().setId("test-root");
+        log("'A' triggers a click on an invisible button");
+        log("'B' triggers a click on a disabled button");
+        log("'C' triggers a click on a visible and enabled button");
+
+        Button invisibleButton = new Button("Invisible button with shortcut");
+        invisibleButton.setClickShortcut(KeyCode.A);
+        invisibleButton.addClickListener(new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                log("Click event for invisible button");
+            }
+        });
+
+        invisibleButton.setVisible(false);
+        addComponent(invisibleButton);
+
+        Button disabledButton = new Button("Disabled button with shortcut");
+        disabledButton.setClickShortcut(KeyCode.B);
+        disabledButton.addClickListener(new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                log("Click event for disabled button");
+            }
+        });
+
+        disabledButton.setEnabled(false);
+        addComponent(disabledButton);
+
+        Button enabledButton = new Button("Enabled button with shortcut");
+        enabledButton.setClickShortcut(KeyCode.C);
+        enabledButton.addClickListener(new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                log("Click event for enabled button");
+            }
+        });
+
+        addComponent(enabledButton);
+    }
+
+    /*
+     * (non-Javadoc)
+     * 
+     * @see com.vaadin.tests.components.AbstractTestUI#getTestDescription()
+     */
+    @Override
+    protected String getTestDescription() {
+        return "Test to ensure actions are not performed on disabled/invisible components";
+    }
+
+    /*
+     * (non-Javadoc)
+     * 
+     * @see com.vaadin.tests.components.AbstractTestUI#getTicketNumber()
+     */
+    @Override
+    protected Integer getTicketNumber() {
+        return 12743;
+    }
+
+}
diff --git a/uitest/src/com/vaadin/tests/actions/ActionsOnInvisibleComponentsTest.java b/uitest/src/com/vaadin/tests/actions/ActionsOnInvisibleComponentsTest.java
new file mode 100644
index 0000000000..bb2a7d0995
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/actions/ActionsOnInvisibleComponentsTest.java
@@ -0,0 +1,46 @@
+package com.vaadin.tests.actions;
+
+import java.util.Collection;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.openqa.selenium.WebElement;
+import org.openqa.selenium.remote.DesiredCapabilities;
+
+import com.vaadin.tests.tb3.MultiBrowserTest;
+
+public class ActionsOnInvisibleComponentsTest extends MultiBrowserTest {
+
+    private static final String LAST_INIT_LOG = "3. 'C' triggers a click on a visible and enabled button";
+
+    // This method should be removed once #12785 is fixed
+    @Override
+    public Collection<DesiredCapabilities> getBrowsersToTest() {
+        Collection<DesiredCapabilities> browsers = super.getBrowsersToTest();
+        // sendKeys does nothing on these browsers
+        browsers.remove(BrowserUtil.firefox(24));
+        browsers.remove(BrowserUtil.ie(8));
+        browsers.remove(BrowserUtil.opera(12));
+
+        // Causes 'cannot focus element'
+        browsers.remove(BrowserUtil.chrome(29));
+        return browsers;
+    }
+
+    @Test
+    public void testShortcutsOnInvisibleDisabledButtons() {
+        openTestURL();
+        Assert.assertEquals(LAST_INIT_LOG, getLogRow(0));
+        invokeShortcut("A");
+        Assert.assertEquals(LAST_INIT_LOG, getLogRow(0));
+        invokeShortcut("B");
+        Assert.assertEquals(LAST_INIT_LOG, getLogRow(0));
+        invokeShortcut("C");
+        Assert.assertEquals("4. Click event for enabled button", getLogRow(0));
+    }
+
+    private void invokeShortcut(CharSequence key) {
+        WebElement shortcutTarget = vaadinElementById("test-root");
+        shortcutTarget.sendKeys(key);
+    }
+}