Issue3922disableditem (#10259)

* Accessibility for MenuBar (#3922)
* TabIndex handling fix and an indexing tweak
* Make disabled MenuItems selectable for accessibility (#3922)

- It should be possible to navigate to a disabled MenuItem, even if
triggering the related command is disabled

* Refactor primary style name and aria attribute handling to own method

4 files changed, 230 insertions, 48 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 142f43dd35..29897f7dd6 100644
--- a/client/src/main/java/com/vaadin/client/ui/VMenuBar.java
+++ b/client/src/main/java/com/vaadin/client/ui/VMenuBar.java
@@ -56,7 +56,7 @@ import com.vaadin.client.WidgetUtil;
 import com.vaadin.shared.ui.ContentMode;
 import com.vaadin.shared.ui.menubar.MenuBarConstants;
 
-public class VMenuBar extends SimpleFocusablePanel
+public class VMenuBar extends FocusableFlowPanel
         implements CloseHandler<PopupPanel>, KeyPressHandler, KeyDownHandler,
         FocusHandler, SubPartAware, MouseOutHandler, MouseOverHandler {
 
@@ -105,6 +105,9 @@ public class VMenuBar extends SimpleFocusablePanel
     /** For internal use only. May be removed or replaced in the future. */
     public boolean enabled = true;
 
+    /** For internal use only. May be removed or replaced in the future. */
+    private boolean ignoreFocus = false;
+
     private VLazyExecutor iconLoadedExecutioner = new VLazyExecutor(100,
             () -> iLayout(true));
 
@@ -131,6 +134,7 @@ public class VMenuBar extends SimpleFocusablePanel
         } else {
             addKeyDownHandler(this);
         }
+        getElement().setAttribute("tabindex", "0");
     }
 
     public VMenuBar(boolean subMenu, VMenuBar parentMenu) {
@@ -148,10 +152,13 @@ public class VMenuBar extends SimpleFocusablePanel
         if (parentMenu == null) {
             // Root menu
             setStyleName(CLASSNAME);
+            containerElement.setAttribute("role", "menubar");
         } else {
             // Child menus inherits style name
             setStyleName(parentMenu.getStyleName());
+            containerElement.setAttribute("role", "menu");
         }
+        getElement().setAttribute("tabindex", "-1");
     }
 
     @Override
@@ -173,7 +180,7 @@ public class VMenuBar extends SimpleFocusablePanel
 
         // Reset the style name for all the items
         for (CustomMenuItem item : items) {
-            item.setStyleName(primaryStyleName + "-menuitem");
+            item.refreshPrimaryStyleNameAndAriaAttributes(primaryStyleName);
         }
 
         if (subMenu
@@ -222,7 +229,7 @@ public class VMenuBar extends SimpleFocusablePanel
                 String bgStyle = "";
                 itemHTML.append("<span class=\"" + getStylePrimaryName()
                         + "-submenu-indicator\"" + bgStyle
-                        + ">&#x25BA;</span>");
+                        + " aria-hidden=\"true\">&#x25BA;</span>");
             }
 
             itemHTML.append("<span class=\"" + getStylePrimaryName()
@@ -272,24 +279,13 @@ public class VMenuBar extends SimpleFocusablePanel
      * Remove all the items in this menu.
      */
     public void clearItems() {
-        Element e = getContainerElement();
-        while (DOM.getChildCount(e) > 0) {
-            DOM.removeChild(e, DOM.getChild(e, 0));
+        for (CustomMenuItem child : items) {
+            remove(child);
         }
         items.clear();
     }
 
     /**
-     * Returns the containing element of the menu.
-     *
-     * @return
-     */
-    @Override
-    public com.google.gwt.user.client.Element getContainerElement() {
-        return DOM.asOld(containerElement);
-    }
-
-    /**
      * Add a new item to this menu.
      *
      * @param html
@@ -315,7 +311,7 @@ public class VMenuBar extends SimpleFocusablePanel
         if (items.contains(item)) {
             return;
         }
-        DOM.appendChild(getContainerElement(), item.getElement());
+        add(item);
         item.setParentMenu(this);
         item.setSelected(false);
         items.add(item);
@@ -325,7 +321,7 @@ public class VMenuBar extends SimpleFocusablePanel
         if (items.contains(item)) {
             return;
         }
-        DOM.insertChild(getContainerElement(), item.getElement(), index);
+        insert(item, index);
         item.setParentMenu(this);
         item.setSelected(false);
         items.add(index, item);
@@ -340,8 +336,7 @@ public class VMenuBar extends SimpleFocusablePanel
         if (items.contains(item)) {
             int index = items.indexOf(item);
 
-            DOM.removeChild(getContainerElement(),
-                    DOM.getChild(getContainerElement(), index));
+            remove(item);
             items.remove(index);
         }
     }
@@ -386,14 +381,6 @@ public class VMenuBar extends SimpleFocusablePanel
                 if (isEnabled() && targetItem.isEnabled()) {
                     itemClick(targetItem);
                 }
-                if (subMenu) {
-                    // Prevent moving keyboard focus to child menus
-                    VMenuBar parent = parentMenu;
-                    while (parent.getParentMenu() != null) {
-                        parent = parent.getParentMenu();
-                    }
-                    parent.setFocus(true);
-                }
 
                 break;
 
@@ -410,13 +397,6 @@ public class VMenuBar extends SimpleFocusablePanel
                 LazyCloser.schedule();
                 break;
             }
-        } else if (subMenu && DOM.eventGetType(e) == Event.ONCLICK && subMenu) {
-            // Prevent moving keyboard focus to child menus
-            VMenuBar parent = parentMenu;
-            while (parent.getParentMenu() != null) {
-                parent = parent.getParentMenu();
-            }
-            parent.setFocus(true);
         }
     }
 
@@ -812,7 +792,8 @@ public class VMenuBar extends SimpleFocusablePanel
      * A class to hold information on menu items.
      *
      */
-    public static class CustomMenuItem extends Widget implements HasHTML {
+    public static class CustomMenuItem extends Widget
+            implements HasHTML, SubPartAware {
 
         protected String html = null;
         protected Command command = null;
@@ -856,6 +837,40 @@ public class VMenuBar extends SimpleFocusablePanel
         }
 
         @Override
+        public void onBrowserEvent(Event event) {
+            VMenuBar p = getParentMenu();
+            if (event.getTypeInt() == Event.ONKEYDOWN
+                    || event.getTypeInt() == Event.ONKEYPRESS) {
+                if (p.getParentMenu() != null
+                        && p.getParentMenu().visibleChildMenu != null) {
+                    int keyCode = event.getKeyCode();
+                    if (keyCode == 0) {
+                        keyCode = event.getCharCode();
+                    }
+                    if (getParentMenu().handleNavigation(keyCode,
+                            event.getCtrlKey() || event.getMetaKey(),
+                            event.getShiftKey())) {
+                        event.preventDefault();
+                    }
+                }
+            }
+        }
+
+        @Override
+        protected void onLoad() {
+            super.onLoad();
+            if (getParentMenu() != null
+                    && getParentMenu().getParentMenu() == null
+                    && getParentMenu().getItems().get(0).equals(this)) {
+                getElement().setAttribute("tabindex", "0");
+            } else {
+                getElement().setAttribute("tabindex", "-1");
+            }
+
+            sinkEvents(Event.KEYEVENTS);
+        }
+
+        @Override
         public void setStyleName(String style) {
             super.setStyleName(style);
             updateStyleNames();
@@ -869,6 +884,9 @@ public class VMenuBar extends SimpleFocusablePanel
         public void setSelected(boolean selected) {
             this.selected = selected;
             updateStyleNames();
+            if (selected) {
+                getElement().focus();
+            }
         }
 
         public void setChecked(boolean checked) {
@@ -903,6 +921,11 @@ public class VMenuBar extends SimpleFocusablePanel
 
         public void setSubMenu(VMenuBar subMenu) {
             this.subMenu = subMenu;
+            if (subMenu != null) {
+                getElement().setAttribute("aria-haspopup", "true");
+            } else {
+                getElement().setAttribute("aria-haspopup", "false");
+            }
         }
 
         public VMenuBar getSubMenu() {
@@ -935,11 +958,7 @@ public class VMenuBar extends SimpleFocusablePanel
                 }
             }
 
-            if (isSeparator) {
-                super.setStyleName(primaryStyleName + "-separator");
-            } else {
-                super.setStyleName(primaryStyleName + "-menuitem");
-            }
+            refreshPrimaryStyleNameAndAriaAttributes(primaryStyleName);
 
             for (String customStyle : customStyles) {
                 super.addStyleName(customStyle);
@@ -988,6 +1007,28 @@ public class VMenuBar extends SimpleFocusablePanel
             }
         }
 
+        private void refreshPrimaryStyleNameAndAriaAttributes(
+                String primaryStyleName) {
+            if (isSeparator) {
+                super.setStyleName(primaryStyleName + "-separator");
+                getElement().setAttribute("role", "separator");
+            } else {
+                super.setStyleName(primaryStyleName + "-menuitem");
+                if (isCheckable()) {
+                    getElement().setAttribute("role", "menuitemcheckbox");
+                    getElement().setAttribute("aria-checked",
+                            String.valueOf(isChecked()));
+                } else {
+                    getElement().setAttribute("role", "menuitem");
+                }
+                if (isEnabled()) {
+                    getElement().removeAttribute("aria-disabled");
+                } else {
+                    getElement().setAttribute("aria-disabled", "true");
+                }
+            }
+        }
+
         public VMenuBar getParentMenu() {
             return parentMenu;
         }
@@ -1091,7 +1132,27 @@ public class VMenuBar extends SimpleFocusablePanel
          * @return true if it is possible to select this item, false otherwise
          */
         public boolean isSelectable() {
-            return !isSeparator() && isEnabled();
+            return !isSeparator();
+        }
+
+        @SuppressWarnings("deprecation")
+        @Override
+        public com.google.gwt.user.client.Element getSubPartElement(
+                String subPart) {
+            if (getSubMenu() != null && getSubMenu().menuVisible) {
+                return getSubMenu().getSubPartElement(subPart);
+            }
+            return null;
+        }
+
+        @SuppressWarnings("deprecation")
+        @Override
+        public String getSubPartName(
+                com.google.gwt.user.client.Element subElement) {
+            if (getSubMenu() != null && getSubMenu().menuVisible) {
+                return getSubMenu().getSubPartName(subElement);
+            }
+            return null;
         }
 
     }
@@ -1338,8 +1399,26 @@ public class VMenuBar extends SimpleFocusablePanel
         // If tab or shift+tab close menus
         if (keycode == KeyCodes.KEY_TAB) {
             setSelected(null);
-            hideChildren();
+            hideParents(false);
             menuVisible = false;
+            VMenuBar root = getParentMenu();
+            while (root != null && root.getParentMenu() != null) {
+                root = root.getParentMenu();
+            }
+            if (root != null) {
+                if (shift) {
+                    root.ignoreFocus = true;
+                    root.getElement().focus();
+                    root.ignoreFocus = false;
+                } else {
+                    root.getElement().focus();
+                    root.setSelected(null);
+                }
+            } else if (shift) {
+                ignoreFocus = true;
+                getElement().focus();
+                ignoreFocus = false;
+            }
             return false;
         }
 
@@ -1394,6 +1473,8 @@ public class VMenuBar extends SimpleFocusablePanel
             } else {
                 getParentMenu().getSelected().getSubMenu().setSelected(null);
                 getParentMenu().hideChildren();
+                getParentMenu().getSelected().getElement().focus();
+                getParentMenu().menuVisible = false;
             }
 
             return true;
@@ -1469,10 +1550,22 @@ public class VMenuBar extends SimpleFocusablePanel
                 // Redirect all navigation to the submenu
                 visibleChildMenu.handleNavigation(keycode, ctrl, shift);
             } else {
-                // Select the previous item if possible or loop to the last item
+                // Select the previous item if possible or loop to the last
+                // item. If menu is in the first popup (opens down), closes the
+                // popup. If menu is the root menu, opens the popup.
                 int idx = items.indexOf(getSelected());
-                if (idx > 0) {
+                if (getParentMenu() == null && visibleChildMenu == null) {
+                    openMenuAndFocusLastIfPossible(selected);
+                } else if (idx > 0) {
                     setSelected(items.get(idx - 1));
+                } else if (getParentMenu() != null
+                        && getParentMenu().getParentMenu() == null) {
+                    getParentMenu().getSelected().getSubMenu()
+                            .setSelected(null);
+                    getParentMenu().hideChildren();
+                    getParentMenu().getSelected().getElement().focus();
+                    getParentMenu().menuVisible = false;
+                    return true;
                 } else {
                     setSelected(items.get(items.size() - 1));
                 }
@@ -1514,12 +1607,19 @@ public class VMenuBar extends SimpleFocusablePanel
         } else if (keycode == getCloseMenuKey()) {
             setSelected(null);
             hideChildren();
+            if (getParentMenu() != null) {
+                getParentMenu().hideChildren();
+                getParentMenu().getSelected().getElement().focus();
+            }
             menuVisible = false;
+            return true;
 
         } else if (isNavigationSelectKey(keycode)) {
             if (getSelected() == null) {
                 // If nothing is selected then select the first item
                 selectFirstItem();
+            } else if (!getSelected().isEnabled()) {
+                // NOP
             } else if (visibleChildMenu != null) {
                 // Redirect all navigation to the submenu
                 visibleChildMenu.handleNavigation(keycode, ctrl, shift);
@@ -1542,8 +1642,17 @@ public class VMenuBar extends SimpleFocusablePanel
                     // #17076 keyboard selected menuitem without children: do
                     // not leave menu to visible ("hover open") mode
                     menuVisible = false;
+
+                    VMenuBar root = this;
+                    while (root.getParentMenu() != null) {
+                        root = root.getParentMenu();
+                    }
+                    root.ignoreFocus = true;
+                    root.getElement().focus();
+                    root.ignoreFocus = false;
                 }
             }
+            return true;
         }
 
         return false;
@@ -1559,10 +1668,20 @@ public class VMenuBar extends SimpleFocusablePanel
         }
     }
 
+    private void selectLastItem() {
+        for (int i = items.size() - 1; i >= 0; i--) {
+            CustomMenuItem item = items.get(i);
+            if (item.isSelectable()) {
+                setSelected(item);
+                break;
+            }
+        }
+    }
+
     private void openMenuAndFocusFirstIfPossible(CustomMenuItem menuItem) {
         VMenuBar subMenu = menuItem.getSubMenu();
-        if (subMenu == null) {
-            // No child menu? Nothing to do
+        if (!menuItem.isEnabled() || subMenu == null) {
+            // No child menu or disabled? Nothing to do
             return;
         }
 
@@ -1572,7 +1691,21 @@ public class VMenuBar extends SimpleFocusablePanel
         menuVisible = true;
         // Select the first item in the newly open submenu
         subMenu.selectFirstItem();
+    }
+
+    private void openMenuAndFocusLastIfPossible(CustomMenuItem menuItem) {
+        VMenuBar subMenu = menuItem.getSubMenu();
+        if (!menuItem.isEnabled() || subMenu == null) {
+            // No child menu or disabled? Nothing to do
+            return;
+        }
 
+        VMenuBar parentMenu = menuItem.getParentMenu();
+        parentMenu.showChildMenu(menuItem);
+
+        menuVisible = true;
+        // Select the last item in the newly open submenu
+        subMenu.selectLastItem();
     }
 
     /*
@@ -1584,7 +1717,9 @@ public class VMenuBar extends SimpleFocusablePanel
      */
     @Override
     public void onFocus(FocusEvent event) {
-
+        if (!ignoreFocus && getSelected() == null) {
+            selectFirstItem();
+        }
     }
 
     private static final String SUBPART_PREFIX = "item";
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 802d4a7396..e1e5dd0297 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
@@ -25,6 +25,7 @@ import com.vaadin.client.ApplicationConnection;
 import com.vaadin.client.Paintable;
 import com.vaadin.client.TooltipInfo;
 import com.vaadin.client.UIDL;
+import com.vaadin.client.annotations.OnStateChange;
 import com.vaadin.client.ui.AbstractComponentConnector;
 import com.vaadin.client.ui.Icon;
 import com.vaadin.client.ui.SimpleManagedLayout;
@@ -214,4 +215,19 @@ public class MenuBarConnector extends AbstractComponentConnector
          */
         return true;
     }
+
+    @OnStateChange("enabled")
+    void updateEnabled() {
+        if (getState().enabled) {
+            getWidget().getElement().removeAttribute("aria-disabled");
+        } else {
+            getWidget().getElement().setAttribute("aria-disabled", "true");
+        }
+    }
+
+    @OnStateChange("tabIndex")
+    void updateTabIndex() {
+        getWidget().getElement().setAttribute("tabindex",
+                String.valueOf(getState().tabIndex));
+    }
 }
diff --git a/uitest/src/main/java/com/vaadin/tests/components/menubar/MenuBarNavigation.java b/uitest/src/main/java/com/vaadin/tests/components/menubar/MenuBarNavigation.java
index b7f2206979..134ccc998a 100644
--- a/uitest/src/main/java/com/vaadin/tests/components/menubar/MenuBarNavigation.java
+++ b/uitest/src/main/java/com/vaadin/tests/components/menubar/MenuBarNavigation.java
@@ -40,6 +40,9 @@ public class MenuBarNavigation extends AbstractTestUIWithLog
         edit.addItem("Cut", this);
         edit.addItem("Paste", this);
         mb.addItem("Help", this);
+        MenuItem disabled = mb.addItem("Disabled", this);
+        disabled.setEnabled(false);
+        disabled.addItem("Can't reach", this);
 
         addComponent(mb);
     }
diff --git a/uitest/src/test/java/com/vaadin/tests/components/menubar/MenuBarNavigationKeyboardTest.java b/uitest/src/test/java/com/vaadin/tests/components/menubar/MenuBarNavigationKeyboardTest.java
index 2f8c4f2e46..6bcb1a259a 100644
--- a/uitest/src/test/java/com/vaadin/tests/components/menubar/MenuBarNavigationKeyboardTest.java
+++ b/uitest/src/test/java/com/vaadin/tests/components/menubar/MenuBarNavigationKeyboardTest.java
@@ -9,6 +9,7 @@ import org.openqa.selenium.By;
 import org.openqa.selenium.Keys;
 import org.openqa.selenium.interactions.Actions;
 
+import com.vaadin.testbench.elements.LabelElement;
 import com.vaadin.testbench.elements.MenuBarElement;
 import com.vaadin.tests.tb3.MultiBrowserTest;
 
@@ -85,11 +86,38 @@ public class MenuBarNavigationKeyboardTest extends MultiBrowserTest {
                 isElementPresent(By.className("v-menubar-popup")));
     }
 
+    @Test
+    public void testNavigatingToDisabled() throws InterruptedException {
+        openTestURL();
+
+        openMenu("File");
+
+        getMenuBar().sendKeys(Keys.ARROW_RIGHT, Keys.ARROW_RIGHT,
+                Keys.ARROW_RIGHT, Keys.ARROW_RIGHT, Keys.ENTER);
+
+        assertTrue("Disabled menu not selected",
+                getFocusedElement().getText().contains("Disabled"));
+
+        assertFalse("Disabled menu was triggered",
+                logContainsText("MenuItem Disabled selected"));
+
+        getMenuBar().sendKeys(Keys.ARROW_DOWN, Keys.ENTER);
+
+        assertFalse("Disabled submenu was opened",
+                logContainsText("MenuItem Disabled/Can't reach selected"));
+
+        assertTrue("Disabled menu not selected",
+                getFocusedElement().getText().contains("Disabled"));
+    }
+
     public MenuBarElement getMenuBar() {
         return $(MenuBarElement.class).first();
     }
 
     public void openMenu(String name) {
+        // move hover focus outside the MenuBar to keep the behaviour stable
+        new Actions(driver).moveToElement($(LabelElement.class).first(), 10, 10)
+                .perform();
         getMenuBar().clickItem(name);
     }
 }