8330304: MenuBar: Invisible Menu works incorrectly with keyboard arrows

Reviewed-by: mstrauss, kcr

Author: Andy Goryachev

modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java

@@ -121,17 +121,15 @@ public class MenuBarSkin extends SkinBase<MenuBar> {
      *                                                                         *
      **************************************************************************/
 
+    /** contains MenuBarButton's children only */
     private final HBox container;
+    /** index of *focused* MenuBarButton */
+    private int focusedMenuIndex = -1;
 
     // represents the currently _open_ menu
     private Menu openMenu;
     private MenuBarButton openMenuButton;
 
-    // represents the currently _focused_ menu. If openMenu is non-null, this should equal
-    // openMenu. If openMenu is null, this can be any menu in the menu bar.
-    private Menu focusedMenu;
-    private int focusedMenuIndex = -1;
-
     private static WeakHashMap<Stage, Reference<MenuBarSkin>> systemMenuMap;
     private static List<MenuBase> wrappedDefaultMenus = new ArrayList<>();
     private static Stage currentMenuBarStage;
@@ -295,7 +293,7 @@ public MenuBarSkin(final MenuBar control) {
                 // Key navigation
                 sceneListenerHelper.addEventFilter(scene, KeyEvent.KEY_PRESSED, (ev) -> {
                     // process right left and may be tab key events
-                    if (focusedMenu != null) {
+                    if (focusedMenuIndex >= 0) {
                         switch (ev.getCode()) {
                         case LEFT: {
                             boolean isRTL = control.getEffectiveNodeOrientation() == NodeOrientation.RIGHT_TO_LEFT;
@@ -429,7 +427,7 @@ private void showMenu(Menu menu, boolean selectFirstItem) {
         if (!menu.isShowing() && !isMenuEmpty(menu)) {
             if (selectFirstItem) {
                 // put selection / focus on first item in menu
-                MenuButton menuButton = getNodeForMenu(focusedMenuIndex);
+                MenuButton menuButton = menuBarButtonAt(focusedMenuIndex);
                 Skin<?> skin = menuButton.getSkin();
                 if (skin instanceof MenuButtonSkinBase) {
                     ((MenuButtonSkinBase)skin).requestFocusOnFirstMenuItem();
@@ -445,7 +443,6 @@ private void showMenu(Menu menu, boolean selectFirstItem) {
      */
     void setFocusedMenuIndex(int index) {
         focusedMenuIndex = (index >= -1 && index < getSkinnable().getMenus().size()) ? index : -1;
-        focusedMenu = (focusedMenuIndex != -1) ? getSkinnable().getMenus().get(index) : null;
 
         if (focusedMenuIndex != -1) {
             openMenuButton = (MenuBarButton)container.getChildren().get(focusedMenuIndex);
@@ -733,12 +730,9 @@ public void dispose() {
      *                                                                         *
      **************************************************************************/
 
-    // For testing purpose only.
-    MenuButton getNodeForMenu(int i) {
-        if (i < container.getChildren().size()) {
-            return (MenuBarButton)container.getChildren().get(i);
-        }
-        return null;
+    // package protected for testing purposes
+    MenuBarButton menuBarButtonAt(int i) {
+        return (MenuBarButton)container.getChildren().get(i);
     }
 
     int getFocusedMenuIndex() {
@@ -1083,6 +1077,7 @@ private void menuModeEnd() {
     }
 
     private void moveToMenu(Direction dir, boolean doShow) {
+        Menu focusedMenu = menuBarButtonAt(focusedMenuIndex).menu;
         boolean showNextMenu = doShow && focusedMenu.isShowing();
         findSibling(dir, focusedMenuIndex).ifPresent(p -> {
             setFocusedMenuIndex(p.getValue());

modules/javafx.controls/src/shims/java/javafx/scene/control/skin/MenuBarSkinShim.java

@@ -37,7 +37,7 @@ public class MenuBarSkinShim {
 
     // can only access the getNodeForMenu method in MenuBarSkin from this package.
     public static MenuButton getNodeForMenu(MenuBarSkin skin, int i) {
-        return skin.getNodeForMenu(i);
+        return skin.menuBarButtonAt(i);
     }
 
     public static Skin getPopupSkin(MenuButton mb) {

modules/javafx.controls/src/test/java/test/javafx/scene/control/MenuBarTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -716,4 +716,68 @@ protected void startApp(Parent root) {
         int focusedIndex = MenuBarSkinShim.getFocusedMenuIndex(skin);
         assertEquals(0, focusedIndex);
     }
+
+    /** Tests keyboard navigation with invisible menu JDK-8330304 */
+    @Test
+    public void testKeyNavigationWithInvisibleMenuItem() {
+        Menu menu1 = mkMenu("1", 3);
+        Menu menu2 = mkMenu("2", 3);
+        Menu menu3 = mkMenu("3", 3);
+        Menu menu4 = mkMenu("4", 3);
+        menuBar.getMenus().setAll(menu1, menu2, menu3, menu4);
+
+        menu2.setVisible(false);
+
+        VBox root = new VBox(menuBar);
+        startApp(root);
+        tk.firePulse();
+
+        MenuBarSkin skin = (MenuBarSkin)menuBar.getSkin();
+        assertTrue(skin != null);
+
+        double x = (menuBar.localToScene(menuBar.getLayoutBounds())).getMinX();
+        double y = (menuBar.localToScene(menuBar.getLayoutBounds())).getMinY();
+
+        // show menu1 using mouse
+        MenuButton mb = MenuBarSkinShim.getNodeForMenu(skin, 0);
+        mb.getScene().getWindow().requestFocus();
+        SceneHelper.processMouseEvent(scene,
+            MouseEventGenerator.generateMouseEvent(MouseEvent.MOUSE_PRESSED, x+20, y+20));
+        SceneHelper.processMouseEvent(scene,
+            MouseEventGenerator.generateMouseEvent(MouseEvent.MOUSE_RELEASED, x+20, y+20));
+        assertTrue(menu1.isShowing());
+
+        // show menu3 with RIGHT key press, menu2 is hidden and should be skipped
+        KeyEventFirer keyboard = new KeyEventFirer(mb.getScene());
+        keyboard.doKeyPress(KeyCode.RIGHT);
+        tk.firePulse();
+
+        assertTrue(menu3.isShowing());
+
+        // show menu4
+        keyboard.doKeyPress(KeyCode.RIGHT);
+        tk.firePulse();
+
+        assertTrue(menu4.isShowing());
+
+        // show menu3
+        keyboard.doKeyPress(KeyCode.LEFT);
+        tk.firePulse();
+
+        assertTrue(menu3.isShowing());
+
+        // show menu1 (menu2 is hidden)
+        keyboard.doKeyPress(KeyCode.LEFT);
+        tk.firePulse();
+
+        assertTrue(menu1.isShowing());
+    }
+
+    private Menu mkMenu(String name, int itemCount) {
+        Menu m = new Menu("Menu" + name);
+        for (int i = 0; i < itemCount; i++) {
+            m.getItems().add(new MenuItem("Menu" + name + "Item" + i));
+        }
+        return m;
+    }
 }