V7: Improve VMenuBar click handling logic (#11362)

* Improve VMenuBar click handling logic

Backport to V7:
During `updateFromUIDL` inside MenuBarConnector we empty and re-instantiate the components of MenuBar. When we are modifying the Menubar from the BlurEventListener of another component, we ,by this, remove widgets, therefore clickEvent is not fired and the action of the MenuItem is not proceed as a result. (The BlurEvent is fired before the click event in the chain of events. )

To improve the situation, we catch onMouseDown event , which is fired before BlurEvent,by assigning mouseDown flag to true. Then if no click event has yet happened, we delay the execution of update inside `updateFromUIDL` by default 500 ms. Then if click event occurs, it proceeds normally. The time can be increased/decreased using setter.

There is no delay, if we are clicking on the MenuBar as usual or no Blur listener is set.

This change allows setting descriptions preserving the action from the MenuItem

(cherry picked from commit 22cc85c76f28a762685204911ad66f95bda2d296)

* Improve VMenuBar click handling logic

Add missing files from the first commit

Backported to V7:
(cherry picked from commit 22cc85c)

diff --git a/client/src/main/java/com/vaadin/client/ui/VMenuBar.java b/client/src/main/java/com/vaadin/client/ui/VMenuBar.java
index 71460c1212173795a78606293b1362520492a5ce..28592ad34ba6cd4a1c57a798c2a30f2ff75d7e1f 100644 (file)
--- a/client/src/main/java/com/vaadin/client/ui/VMenuBar.java
+++ b/client/src/main/java/com/vaadin/client/ui/VMenuBar.java
@@ -114,6 +114,8 @@ public class VMenuBar extends SimpleFocusablePanel
     /** For internal use only. May be removed or replaced in the future. */
     public boolean openRootOnHover;
 
+    public boolean mouseDownPressed;
+
     /** For internal use only. May be removed or replaced in the future. */
     public boolean htmlContentAllowed;
 
@@ -384,8 +386,17 @@ public class VMenuBar extends SimpleFocusablePanel
         if (targetItem != null) {
             switch (DOM.eventGetType(e)) {
 
+            case Event.ONMOUSEDOWN:
+                if (e.getButton() == Event.BUTTON_LEFT) {
+                    if (isEnabled() && targetItem.isEnabled()) {
+                        // Button is clicked, but not yet released
+                        mouseDownPressed = true;
+                    }
+                }
+                break;
             case Event.ONCLICK:
                 if (isEnabled() && targetItem.isEnabled()) {
+                    mouseDownPressed = false;
                     itemClick(targetItem);
                 }
                 if (subMenu) {

diff --git a/client/src/main/java/com/vaadin/client/ui/menubar/MenuBarConnector.java b/client/src/main/java/com/vaadin/client/ui/menubar/MenuBarConnector.java
index bd8cb2758d5c8d16ff7d727ea87b458fcad99f3d..e9b568da4f31f869e9fe0ed2c07480cd34ad03ca 100644 (file)
--- a/client/src/main/java/com/vaadin/client/ui/menubar/MenuBarConnector.java
+++ b/client/src/main/java/com/vaadin/client/ui/menubar/MenuBarConnector.java
@@ -21,6 +21,7 @@ import java.util.Stack;
 import com.google.gwt.core.client.GWT;
 import com.google.gwt.dom.client.Element;
 import com.google.gwt.user.client.Command;
+import com.google.gwt.user.client.Timer;
 import com.vaadin.client.ApplicationConnection;
 import com.vaadin.client.Paintable;
 import com.vaadin.client.TooltipInfo;
@@ -46,7 +47,7 @@ public class MenuBarConnector extends AbstractComponentConnector
      * every time UI changes in the component are received from the server.
      */
     @Override
-    public void updateFromUIDL(UIDL uidl, ApplicationConnection client) {
+    public void updateFromUIDL(final UIDL uidl, final ApplicationConnection client) {
         if (!isRealUpdate(uidl)) {
             return;
         }
@@ -63,114 +64,120 @@ public class MenuBarConnector extends AbstractComponentConnector
         getWidget().client = client;
         getWidget().uidlId = uidl.getId();
 
-        // Empty the menu every time it receives new information
-        if (!getWidget().getItems().isEmpty()) {
-            getWidget().clearItems();
-        }
+        Timer timer = new Timer() {
 
-        UIDL options = uidl.getChildUIDL(0);
+            @Override
+            public void run() {
+                // Empty the menu every time it receives new information
+                if (!getWidget().getItems().isEmpty()) {
+                    getWidget().clearItems();
+                }
 
-        if (null != getState()
-                && !ComponentStateUtil.isUndefinedWidth(getState())) {
-            UIDL moreItemUIDL = options.getChildUIDL(0);
-            StringBuffer itemHTML = new StringBuffer();
+                UIDL options = uidl.getChildUIDL(0);
 
-            if (moreItemUIDL.hasAttribute("icon")) {
-                Icon icon = client
-                        .getIcon(moreItemUIDL.getStringAttribute("icon"));
-                if (icon != null) {
-                    itemHTML.append(icon.getElement().getString());
-                }
-            }
+                if (null != getState() && !ComponentStateUtil.isUndefinedWidth(getState())) {
+                    UIDL moreItemUIDL = options.getChildUIDL(0);
+                    StringBuffer itemHTML = new StringBuffer();
 
-            String moreItemText = moreItemUIDL.getStringAttribute("text");
-            if ("".equals(moreItemText)) {
-                moreItemText = "►";
-            }
-            itemHTML.append(moreItemText);
+                    if (moreItemUIDL.hasAttribute("icon")) {
+                        Icon icon = client.getIcon(moreItemUIDL.getStringAttribute("icon"));
+                        if (icon != null) {
+                            itemHTML.append(icon.getElement().getString());
+                        }
+                    }
 
-            getWidget().moreItem = GWT.create(VMenuBar.CustomMenuItem.class);
-            getWidget().moreItem.setHTML(itemHTML.toString());
-            getWidget().moreItem.setCommand(VMenuBar.emptyCommand);
+                    String moreItemText = moreItemUIDL.getStringAttribute("text");
+                    if ("".equals(moreItemText)) {
+                        moreItemText = "►";
+                    }
+                    itemHTML.append(moreItemText);
 
-            getWidget().collapsedRootItems = new VMenuBar(true, getWidget());
-            getWidget().moreItem.setSubMenu(getWidget().collapsedRootItems);
-            getWidget().moreItem.addStyleName(
-                    getWidget().getStylePrimaryName() + "-more-menuitem");
-        }
+                    getWidget().moreItem = GWT.create(VMenuBar.CustomMenuItem.class);
+                    getWidget().moreItem.setHTML(itemHTML.toString());
+                    getWidget().moreItem.setCommand(VMenuBar.emptyCommand);
 
-        UIDL uidlItems = uidl.getChildUIDL(1);
-        Iterator<Object> itr = uidlItems.getChildIterator();
-        Stack<Iterator<Object>> iteratorStack = new Stack<Iterator<Object>>();
-        Stack<VMenuBar> menuStack = new Stack<VMenuBar>();
-        VMenuBar currentMenu = getWidget();
-
-        while (itr.hasNext()) {
-            UIDL item = (UIDL) itr.next();
-            VMenuBar.CustomMenuItem currentItem = null;
-
-            final int itemId = item.getIntAttribute("id");
-
-            boolean itemHasCommand = item.hasAttribute("command");
-            boolean itemIsCheckable = item
-                    .hasAttribute(MenuBarConstants.ATTRIBUTE_CHECKED);
-
-            String itemHTML = getWidget().buildItemHTML(item);
-
-            Command cmd = null;
-            if (!item.hasAttribute("separator")) {
-                if (itemHasCommand || itemIsCheckable) {
-                    // Construct a command that fires onMenuClick(int) with the
-                    // item's id-number
-                    cmd = new Command() {
-                        @Override
-                        public void execute() {
-                            getWidget().hostReference.onMenuClick(itemId);
-                        }
-                    };
+                    getWidget().collapsedRootItems = new VMenuBar(true,
+                            getWidget());
+                    getWidget().moreItem.setSubMenu(getWidget().collapsedRootItems);
+                    getWidget().moreItem.addStyleName(
+                            getWidget().getStylePrimaryName() + "-more-menuitem");
                 }
-            }
 
-            currentItem = currentMenu.addItem(itemHTML.toString(), cmd);
-            currentItem.updateFromUIDL(item, client);
-
-            if (item.getChildCount() > 0) {
-                menuStack.push(currentMenu);
-                iteratorStack.push(itr);
-                itr = item.getChildIterator();
-                currentMenu = new VMenuBar(true, currentMenu);
-                client.getVTooltip().connectHandlersToWidget(currentMenu);
-                // this is the top-level style that also propagates to items -
-                // any item specific styles are set above in
-                // currentItem.updateFromUIDL(item, client)
-                if (ComponentStateUtil.hasStyles(getState())) {
-                    for (String style : getState().styles) {
-                        currentMenu.addStyleDependentName(style);
+                UIDL uidlItems = uidl.getChildUIDL(1);
+                Iterator<Object> itr = uidlItems.getChildIterator();
+                Stack<Iterator<Object>> iteratorStack = new Stack<Iterator<Object>>();
+                Stack<VMenuBar> menuStack = new Stack<VMenuBar>();
+                VMenuBar currentMenu = getWidget();
+
+                while (itr.hasNext()) {
+                    UIDL item = (UIDL) itr.next();
+                    VMenuBar.CustomMenuItem currentItem = null;
+
+                    final int itemId = item.getIntAttribute("id");
+
+                    boolean itemHasCommand = item.hasAttribute("command");
+                    boolean itemIsCheckable = item.hasAttribute(MenuBarConstants.ATTRIBUTE_CHECKED);
+
+                    String itemHTML = getWidget().buildItemHTML(item);
+
+                    Command cmd = null;
+                    if (!item.hasAttribute("separator")) {
+                        if (itemHasCommand || itemIsCheckable) {
+                            // Construct a command that fires onMenuClick(int) with the
+                            // item's id-number
+                            cmd = new Command() {
+                                @Override
+                                public void execute() {
+                                    getWidget().hostReference.onMenuClick(itemId);
+                                }
+                            };
+                        }
                     }
-                }
-                currentItem.setSubMenu(currentMenu);
-            }
 
-            while (!itr.hasNext() && !iteratorStack.empty()) {
-                boolean hasCheckableItem = false;
-                for (VMenuBar.CustomMenuItem menuItem : currentMenu
-                        .getItems()) {
-                    hasCheckableItem = hasCheckableItem
-                            || menuItem.isCheckable();
-                }
-                if (hasCheckableItem) {
-                    currentMenu.addStyleDependentName("check-column");
-                } else {
-                    currentMenu.removeStyleDependentName("check-column");
-                }
+                    currentItem = currentMenu.addItem(itemHTML.toString(), cmd);
+                    currentItem.updateFromUIDL(item, client);
+
+                    if (item.getChildCount() > 0) {
+                        menuStack.push(currentMenu);
+                        iteratorStack.push(itr);
+                        itr = item.getChildIterator();
+                        currentMenu = new VMenuBar(true, currentMenu);
+                        client.getVTooltip().connectHandlersToWidget(currentMenu);
+                        // this is the top-level style that also propagates to items -
+                        // any item specific styles are set above in
+                        // currentItem.updateFromUIDL(item, client)
+                        if (ComponentStateUtil.hasStyles(getState())) {
+                            for (String style : getState().styles) {
+                                currentMenu.addStyleDependentName(style);
+                            }
+                        }
+                        currentItem.setSubMenu(currentMenu);
+                    }
 
-                itr = iteratorStack.pop();
-                currentMenu = menuStack.pop();
-            }
-        } // while
+                    while (!itr.hasNext() && !iteratorStack.empty()) {
+                        boolean hasCheckableItem = false;
+                        for (VMenuBar.CustomMenuItem menuItem : currentMenu.getItems()) {
+                            hasCheckableItem = hasCheckableItem || menuItem.isCheckable();
+                        }
+                        if (hasCheckableItem) {
+                            currentMenu.addStyleDependentName("check-column");
+                        } else {
+                            currentMenu.removeStyleDependentName("check-column");
+                        }
 
+                        itr = iteratorStack.pop();
+                        currentMenu = menuStack.pop();
+                    }
+                } // while
+            }
+        };
         getLayoutManager().setNeedsHorizontalLayout(this);
-
+        if (getWidget().mouseDownPressed) {
+            timer.schedule(getState().delayMs);
+            getWidget().mouseDownPressed = false;
+        } else {
+            timer.run();
+        }
     }// updateFromUIDL
 
     @Override

diff --git a/server/src/main/java/com/vaadin/ui/MenuBar.java b/server/src/main/java/com/vaadin/ui/MenuBar.java
index 654ca7726c5fc88e2a293b6d24766fa244d30ad8..4614cbf8f1b6c6dd38a90202f0b2dfb7bf7d189b 100644 (file)
--- a/server/src/main/java/com/vaadin/ui/MenuBar.java
+++ b/server/src/main/java/com/vaadin/ui/MenuBar.java
@@ -1089,4 +1089,27 @@ public class MenuBar extends AbstractComponent
         result.add("html-content-allowed");
         return result;
     }
+
+    /**
+     * Returns the delay before executing update logic inside
+     * {@link com.vaadin.client.ui.menubar.MenuBarConnector#updateFromUIDL(UIDL, ApplicationConnection)}
+     * after mouseDownEvent
+     *
+     * @since
+     */
+    public int getDelayMs() {
+        return getState(false).delayMs;
+    }
+
+    /**
+     * Set the delay before executing update logic inside
+     * {@link com.vaadin.client.ui.menubar.MenuBarConnector#updateFromUIDL(UIDL, ApplicationConnection)}
+     * after mouseDownEvent
+     *
+     * @since
+     */
+    public void setDelayMs(int delayMs) {
+        getState().delayMs = delayMs;
+    }
+
 }// class MenuBar

diff --git a/shared/src/main/java/com/vaadin/shared/ui/menubar/MenuBarState.java b/shared/src/main/java/com/vaadin/shared/ui/menubar/MenuBarState.java
index a2854ad6be0ab37bff81f55175a6312fcb62acfa..b79b676f899d2e0244040702ed5e6eda0f3d6219 100644 (file)
--- a/shared/src/main/java/com/vaadin/shared/ui/menubar/MenuBarState.java
+++ b/shared/src/main/java/com/vaadin/shared/ui/menubar/MenuBarState.java
@@ -21,4 +21,5 @@ public class MenuBarState extends TabIndexState {
     {
         primaryStyleName = "v-menubar";
     }
+    public int delayMs = 500;
 }

diff --git a/uitest/src/main/java/com/vaadin/tests/components/menubar/MenuBarChangeFromEventListener.java b/uitest/src/main/java/com/vaadin/tests/components/menubar/MenuBarChangeFromEventListener.java
new file mode 100644 (file)
index 0000000..5efc13f
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/menubar/MenuBarChangeFromEventListener.java
@@ -0,0 +1,55 @@
+package com.vaadin.tests.components.menubar;
+
+import com.vaadin.event.FieldEvents;
+import com.vaadin.tests.components.ComponentTestCase;
+import com.vaadin.ui.Label;
+import com.vaadin.ui.MenuBar;
+import com.vaadin.ui.TextField;
+
+public class MenuBarChangeFromEventListener extends ComponentTestCase<MenuBar> {
+    public final static String MENU_CLICKED = "Menu Selected";
+    public final static String MENU_CLICKED_BLUR = "Menu Selected after TF Blur event";
+
+
+    @Override
+    protected Class<MenuBar> getTestClass() {
+        return MenuBar.class;
+    }
+
+    @Override
+    protected void initializeComponents() {
+       final MenuBar mb = new MenuBar();
+        mb.setCaption("");
+
+        MenuBar.MenuItem mi = mb.addItem("Item to click", null,
+                new MenuBar.Command() {
+                    @Override
+                    public void menuSelected(MenuBar.MenuItem selectedItem) {
+                            Label label=new Label(MENU_CLICKED);
+                            label.addStyleName("menuClickedLabel");
+                             addComponent(label);
+                    }
+                });
+        mb.setId("menuBar");
+        TextField tf = new TextField(
+                "2. Focus this TextField and then click the menu");
+        tf.setId("textField");
+        tf.addBlurListener(new FieldEvents.BlurListener() {
+            @Override
+            public void blur(FieldEvents.BlurEvent event) {
+                if (mb.getDescription().isEmpty()) {
+                    mb.setDescription("Some Text here");
+                } else {
+                    mb.setDescription("");
+                }
+                Label label=new Label(MENU_CLICKED_BLUR);
+                label.addStyleName("blurListenerLabel");
+                addComponent(label);
+            }
+
+        });
+
+        addComponent(mb);
+        addComponent(tf);
+    }
+}

diff --git a/uitest/src/test/java/com/vaadin/tests/components/menubar/MenuBarChangeFromEventListenerTest.java b/uitest/src/test/java/com/vaadin/tests/components/menubar/MenuBarChangeFromEventListenerTest.java
new file mode 100644 (file)
index 0000000..6e8e292
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/menubar/MenuBarChangeFromEventListenerTest.java
@@ -0,0 +1,30 @@
+package com.vaadin.tests.components.menubar;
+
+import com.vaadin.tests.tb3.MultiBrowserTest;
+import org.junit.Test;
+import org.openqa.selenium.By;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class MenuBarChangeFromEventListenerTest extends MultiBrowserTest {
+
+    @Test
+    public void eventFired() {
+        openTestURL();
+
+        findElement(By.className("v-menubar-menuitem")).click();
+        assertEquals(1,findElements(By.className("menuClickedLabel")).size());
+        findElement(By.id("textField")).click();
+
+        findElement(By.className("v-menubar-menuitem")).click();
+        try {
+            sleep(300);
+        } catch (InterruptedException e) {
+            e.printStackTrace();
+        }
+
+        assertEquals(1,findElements(By.className("blurListenerLabel")).size());
+        assertEquals(2,findElements(By.className("menuClickedLabel")).size());
+    }
+}