From c9dc0d64ecade71574be9f494e7c5eec2d190ff5 Mon Sep 17 00:00:00 2001 From: Anastasia Smirnova Date: Fri, 14 Dec 2018 11:13:01 +0200 Subject: [PATCH] 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) --- .../java/com/vaadin/client/ui/VMenuBar.java | 11 + .../client/ui/menubar/MenuBarConnector.java | 197 +++++++++--------- .../src/main/java/com/vaadin/ui/MenuBar.java | 23 ++ .../shared/ui/menubar/MenuBarState.java | 1 + .../MenuBarChangeFromEventListener.java | 55 +++++ .../MenuBarChangeFromEventListenerTest.java | 30 +++ 6 files changed, 222 insertions(+), 95 deletions(-) create mode 100644 uitest/src/main/java/com/vaadin/tests/components/menubar/MenuBarChangeFromEventListener.java create mode 100644 uitest/src/test/java/com/vaadin/tests/components/menubar/MenuBarChangeFromEventListenerTest.java 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 71460c1212..28592ad34b 100644 --- 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 bd8cb2758d..e9b568da4f 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; @@ -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 itr = uidlItems.getChildIterator(); - Stack> iteratorStack = new Stack>(); - Stack menuStack = new Stack(); - 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 itr = uidlItems.getChildIterator(); + Stack> iteratorStack = new Stack>(); + Stack menuStack = new Stack(); + 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 654ca7726c..4614cbf8f1 100644 --- 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 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..5efc13f830 --- /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 { + public final static String MENU_CLICKED = "Menu Selected"; + public final static String MENU_CLICKED_BLUR = "Menu Selected after TF Blur event"; + + + @Override + protected Class 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 index 0000000000..6e8e292a62 --- /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()); + } +} -- 2.39.5