Improve VMenuBar click handling logic (#11356)

* Improve VMenuBar click handling logic

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

* Remove accidentally committed comment

* Don't update the state on the getDelayMs call

6 files changed, 218 insertions, 93 deletions

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 2d4f35a2ad..2f905bd4b9 100644
--- a/client/src/main/java/com/vaadin/client/ui/VMenuBar.java
+++ b/client/src/main/java/com/vaadin/client/ui/VMenuBar.java
@@ -121,6 +121,8 @@ public class VMenuBar extends FocusableFlowPanel implements
     /** For internal use only. May be removed or replaced in the future. */
     public boolean htmlContentAllowed;
 
+    public boolean mouseDownPressed;
+
     private Map<String, List<Command>> triggers = new HashMap<>();
 
     public VMenuBar() {
@@ -377,7 +379,6 @@ public class VMenuBar extends FocusableFlowPanel implements
     @Override
     public void onBrowserEvent(Event e) {
         super.onBrowserEvent(e);
-
         // Handle onload events (icon loaded, size changes)
         if (DOM.eventGetType(e) == Event.ONLOAD) {
             VMenuBar parent = getParentMenu();
@@ -401,20 +402,26 @@ public class VMenuBar extends FocusableFlowPanel implements
                 targetItem = item;
             }
         }
-
         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);
                 }
-
                 break;
 
             case Event.ONMOUSEOVER:
                 LazyCloser.cancelClosing();
-
                 if (isEnabled() && targetItem.isEnabled()) {
                     itemOver(targetItem);
                 }
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 8697f07d49..a66cd59851 100644
--- 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;
@@ -64,110 +65,126 @@ public class MenuBarConnector extends AbstractComponentConnector
         // For future connections
         widget.client = client;
         widget.uidlId = uidl.getId();
+        Timer timer = new Timer() {
 
-        // Empty the menu every time it receives new information
-        if (!widget.getItems().isEmpty()) {
-            widget.clearItems();
-        }
-
-        UIDL options = uidl.getChildUIDL(0);
-
-        if (null != getState()
-                && !ComponentStateUtil.isUndefinedWidth(getState())) {
-            UIDL moreItemUIDL = options.getChildUIDL(0);
-            StringBuilder itemHTML = new StringBuilder();
-
-            if (moreItemUIDL.hasAttribute("icon")) {
-                Icon icon = client
-                        .getIcon(moreItemUIDL.getStringAttribute("icon"));
-                if (icon != null) {
-                    itemHTML.append(icon.getElement().getString());
+            @Override
+            public void run() {
+                // Empty the menu every time it receives new information
+                if (!widget.getItems().isEmpty()) {
+                    widget.clearItems();
                 }
-            }
 
-            String moreItemText = moreItemUIDL.getStringAttribute("text");
-            if ("".equals(moreItemText)) {
-                moreItemText = "&#x25BA;";
-            }
-            itemHTML.append(moreItemText);
+                UIDL options = uidl.getChildUIDL(0);
 
-            widget.moreItem = GWT.create(VMenuBar.CustomMenuItem.class);
-            widget.moreItem.setHTML(itemHTML.toString());
-            widget.moreItem.setCommand(VMenuBar.emptyCommand);
+                if (null != getState()
+                        && !ComponentStateUtil.isUndefinedWidth(getState())) {
+                    UIDL moreItemUIDL = options.getChildUIDL(0);
+                    StringBuilder itemHTML = new StringBuilder();
 
-            widget.collapsedRootItems = new VMenuBar(true, widget);
-            widget.moreItem.setSubMenu(widget.collapsedRootItems);
-            widget.moreItem.addStyleName(
-                    widget.getStylePrimaryName() + "-more-menuitem");
-        }
-
-        UIDL uidlItems = uidl.getChildUIDL(1);
-        Iterator<Object> itr = uidlItems.iterator();
-        Stack<Iterator<Object>> iteratorStack = new Stack<>();
-        Stack<VMenuBar> menuStack = new Stack<>();
-        VMenuBar currentMenu = widget;
-
-        while (itr.hasNext()) {
-            UIDL item = (UIDL) itr.next();
-            VMenuBar.CustomMenuItem currentItem = null;
-
-            final int itemId = item.getIntAttribute("id");
+                    if (moreItemUIDL.hasAttribute("icon")) {
+                        Icon icon = client.getIcon(
+                                moreItemUIDL.getStringAttribute("icon"));
+                        if (icon != null) {
+                            itemHTML.append(icon.getElement().getString());
+                        }
+                    }
 
-            boolean itemHasCommand = item.hasAttribute("command");
-            boolean itemIsCheckable = item
-                    .hasAttribute(MenuBarConstants.ATTRIBUTE_CHECKED);
+                    String moreItemText = moreItemUIDL
+                            .getStringAttribute("text");
+                    if ("".equals(moreItemText)) {
+                        moreItemText = "&#x25BA;";
+                    }
+                    itemHTML.append(moreItemText);
 
-            String itemHTML = widget.buildItemHTML(item);
+                    widget.moreItem = GWT.create(VMenuBar.CustomMenuItem.class);
+                    widget.moreItem.setHTML(itemHTML.toString());
+                    widget.moreItem.setCommand(VMenuBar.emptyCommand);
 
-            Command cmd = null;
-            if (!item.hasAttribute("separator")) {
-                if (itemHasCommand || itemIsCheckable) {
-                    // Construct a command that fires onMenuClick(int) with the
-                    // item's id-number
-                    cmd = () -> widget.hostReference.onMenuClick(itemId);
+                    widget.collapsedRootItems = new VMenuBar(true, widget);
+                    widget.moreItem.setSubMenu(widget.collapsedRootItems);
+                    widget.moreItem.addStyleName(
+                            widget.getStylePrimaryName() + "-more-menuitem");
                 }
-            }
 
-            currentItem = currentMenu.addItem(itemHTML, cmd);
-            currentItem.setId("" + itemId);
-            currentItem.updateFromUIDL(item, client);
-
-            if (item.getChildCount() > 0) {
-                menuStack.push(currentMenu);
-                iteratorStack.push(itr);
-                itr = item.iterator();
-                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.iterator();
+                Stack<Iterator<Object>> iteratorStack = new Stack<>();
+                Stack<VMenuBar> menuStack = new Stack<>();
+                VMenuBar currentMenu = widget;
+
+                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 = widget.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 = () -> widget.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, cmd);
+                    currentItem.setId("" + itemId);
+                    currentItem.updateFromUIDL(item, client);
+
+                    if (item.getChildCount() > 0) {
+                        menuStack.push(currentMenu);
+                        iteratorStack.push(itr);
+                        itr = item.iterator();
+                        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 (!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();
+                    }
+                }
             }
+        };
+        getLayoutManager().setNeedsHorizontalLayout(MenuBarConnector.this);
+        if (widget.mouseDownPressed) {
+            timer.schedule(getState().delayMs);
+            widget.mouseDownPressed = false;
+        } else {
+            timer.run();
         }
-
-        getLayoutManager().setNeedsHorizontalLayout(this);
     }
 
     @Override
diff --git a/server/src/main/java/com/vaadin/ui/MenuBar.java b/server/src/main/java/com/vaadin/ui/MenuBar.java
index 7b2437e772..ce43802cc5 100644
--- a/server/src/main/java/com/vaadin/ui/MenuBar.java
+++ b/server/src/main/java/com/vaadin/ui/MenuBar.java
@@ -450,6 +450,28 @@ public class MenuBar extends AbstractComponent
         getState().tabIndex = tabIndex;
     }
 
+    /**
+     * 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;
+    }
+
     @Override
     public void focus() {
         // Overridden only to make public
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 a2854ad6be..b79b676f89 100644
--- 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
index 0000000000..add98ea4e8
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/menubar/MenuBarChangeFromEventListener.java
@@ -0,0 +1,47 @@
+package com.vaadin.tests.components.menubar;
+
+import com.vaadin.annotations.Widgetset;
+import com.vaadin.event.FieldEvents;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUIWithLog;
+import com.vaadin.ui.MenuBar;
+import com.vaadin.ui.TextField;
+
+@Widgetset("com.vaadin.DefaultWidgetSet")
+public class MenuBarChangeFromEventListener extends AbstractTestUIWithLog {
+    public final static String MENU_CLICKED = "Menu Selected";
+    public final static String MENU_CLICKED_BLUR = "Menu Selected after TF Blur event";
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        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) {
+                        log(MENU_CLICKED);
+                    }
+                });
+        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("");
+                }
+                log(MENU_CLICKED_BLUR);
+            }
+
+        });
+
+        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
index 0000000000..72db8b5fc5
--- /dev/null
+++ b/uitest/src/test/java/com/vaadin/tests/components/menubar/MenuBarChangeFromEventListenerTest.java
@@ -0,0 +1,31 @@
+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;
+
+public class MenuBarChangeFromEventListenerTest extends MultiBrowserTest {
+
+    @Test
+    public void eventFired() {
+        openTestURL();
+
+        findElement(By.className("v-menubar-menuitem")).click();
+        assertLogRow(0, 1, MenuBarChangeFromEventListener.MENU_CLICKED);
+        findElement(By.id("textField")).click();
+
+        findElement(By.className("v-menubar-menuitem")).click();
+        sleep(300);
+        assertLogRow(1, 2, MenuBarChangeFromEventListener.MENU_CLICKED_BLUR);
+        assertLogRow(0, 3, MenuBarChangeFromEventListener.MENU_CLICKED);
+    }
+
+    private void assertLogRow(int index, int expentedRowNo,
+            String expectedValueWithoutRowNo) {
+        assertEquals(expentedRowNo + ". " + expectedValueWithoutRowNo,
+                getLogRow(index));
+    }
+
+}