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 22cc85c76f)

* Improve VMenuBar click handling logic

Add missing files from the first commit

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

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) {

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

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

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;
 }

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);
+    }
+}

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());
+    }
+}