Test case and fix for #4528 - PaintException if the last menuItem of a sub menu is invisible

svn changeset:12765/svn branch:6.3

2 files changed, 69 insertions, 42 deletions

diff --git a/src/com/vaadin/ui/MenuBar.java b/src/com/vaadin/ui/MenuBar.java
index 03057c6207..036aa1b718 100644
--- a/src/com/vaadin/ui/MenuBar.java
+++ b/src/com/vaadin/ui/MenuBar.java
@@ -76,63 +76,56 @@ public class MenuBar extends AbstractComponent {
         target.endTag("options");
         target.startTag("items");
 
-        Iterator<MenuItem> itr = menuItems.iterator();
-
         // This generates the tree from the contents of the menu
-        while (itr.hasNext()) {
-
-            MenuItem item = itr.next();
-            if (!item.isVisible()) {
-                continue;
-            }
+        for (MenuItem item : menuItems) {
+            paintItem(target, item);
+        }
 
-            target.startTag("item");
-            target.addAttribute("id", item.getId());
+        target.endTag("items");
+    }
 
-            if (item.getStyleName() != null) {
-                target.addAttribute("style", item.getStyleName());
-            }
+    private void paintItem(PaintTarget target, MenuItem item)
+            throws PaintException {
+        if (!item.isVisible()) {
+            return;
+        }
 
-            if (item.isSeparator()) {
-                target.addAttribute("separator", true);
-                target.endTag("item");
-            } else {
-                target.addAttribute("text", item.getText());
+        target.startTag("item");
 
-                Command command = item.getCommand();
-                if (command != null) {
-                    target.addAttribute("command", true);
-                }
+        target.addAttribute("id", item.getId());
 
-                Resource icon = item.getIcon();
-                if (icon != null) {
-                    target.addAttribute("icon", icon);
-                }
+        if (item.getStyleName() != null) {
+            target.addAttribute("style", item.getStyleName());
+        }
 
-                if (!item.isEnabled()) {
-                    target.addAttribute("disabled", true);
-                }
+        if (item.isSeparator()) {
+            target.addAttribute("separator", true);
+        } else {
+            target.addAttribute("text", item.getText());
 
-                if (item.hasChildren()) {
-                    iteratorStack.push(itr); // For later use
+            Command command = item.getCommand();
+            if (command != null) {
+                target.addAttribute("command", true);
+            }
 
-                    // Go through the children
-                    itr = item.getChildren().iterator();
-                } else {
-                    target.endTag("item"); // Item had no children, end
-                    // description
-                }
+            Resource icon = item.getIcon();
+            if (icon != null) {
+                target.addAttribute("icon", icon);
+            }
 
+            if (!item.isEnabled()) {
+                target.addAttribute("disabled", true);
             }
 
-            // The end submenu. More than one submenu may end at once.
-            while (!itr.hasNext() && !iteratorStack.empty()) {
-                itr = iteratorStack.pop();
-                target.endTag("item");
+            if (item.hasChildren()) {
+                for (MenuItem child : item.getChildren()) {
+                    paintItem(target, child);
+                }
             }
+
         }
 
-        target.endTag("items");
+        target.endTag("item");
     }
 
     /** Deserialize changes received from client. */
diff --git a/tests/src/com/vaadin/tests/components/menubar/HiddenAndDisabledMenus.java b/tests/src/com/vaadin/tests/components/menubar/HiddenAndDisabledMenus.java
new file mode 100644
index 0000000000..ef5d5ad395
--- /dev/null
+++ b/tests/src/com/vaadin/tests/components/menubar/HiddenAndDisabledMenus.java
@@ -0,0 +1,34 @@
+package com.vaadin.tests.components.menubar;

+

+import com.vaadin.tests.components.TestBase;

+import com.vaadin.ui.MenuBar;

+import com.vaadin.ui.MenuBar.MenuItem;

+

+public class HiddenAndDisabledMenus extends TestBase {

+

+    @Override

+    protected void setup() {

+        MenuBar mb = new MenuBar();

+        mb.addItem("Item 1", null);

+        mb.addItem("Item 2 - hidden", null).setVisible(false);

+        MenuItem submenu = mb.addItem("Item 3 - sub menu", null);

+        mb.addItem("Item 4 - hidden", null).setVisible(false);

+        submenu.addItem("Sub item 1 - disabled", null).setEnabled(false);

+        submenu.addItem("Sub item 2 - enabled", null);

+        submenu.addItem("Sub item 3 - visible", null);

+        submenu.addItem("Sub item 4 - hidden", null).setVisible(false);

+

+        addComponent(mb);

+    }

+

+    @Override

+    protected String getDescription() {

+        return "The menu contains 4 items, 2 of which are hidden. The sub menu contains 4 items, the last one is hidden";

+    }

+

+    @Override

+    protected Integer getTicketNumber() {

+        return 4528;

+    }

+

+}