8283509: Invisible menus can lead to IndexOutOfBoundsException

Reviewed-by: aghaisas

Author: Robert Lichtenberger

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

@@ -90,6 +90,8 @@
 import com.sun.javafx.scene.control.GlobalMenuAdapter;
 import com.sun.javafx.tk.Toolkit;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
+
 import javafx.stage.Window;
 import javafx.util.Pair;
 
@@ -1110,11 +1112,13 @@ private Optional<Pair<Menu,Integer>> findSibling(Direction dir, int startIndex)
             return Optional.empty();
         }
 
-        final int totalMenus = getSkinnable().getMenus().size();
+        List<Menu> visibleMenus = getSkinnable().getMenus().stream().filter(Menu::isVisible)
+                .collect(Collectors.toList());
+        final int totalMenus = visibleMenus.size();
         int i = 0;
         int nextIndex = 0;
 
-        // Traverse all menus in menubar to find nextIndex
+        // Traverse all visible menus in menubar to find nextIndex
         while (i < totalMenus) {
             i++;
 
@@ -1126,7 +1130,7 @@ private Optional<Pair<Menu,Integer>> findSibling(Direction dir, int startIndex)
             }
 
             // if menu at nextIndex is disabled, skip it
-            if (getSkinnable().getMenus().get(nextIndex).isDisable()) {
+            if (visibleMenus.get(nextIndex).isDisable()) {
                 // Calculate new nextIndex by continuing loop
                 startIndex = nextIndex;
             } else {
@@ -1136,7 +1140,7 @@ private Optional<Pair<Menu,Integer>> findSibling(Direction dir, int startIndex)
         }
 
         clearMenuButtonHover();
-        return Optional.of(new Pair<>(getSkinnable().getMenus().get(nextIndex), nextIndex));
+        return Optional.of(new Pair<>(visibleMenus.get(nextIndex), nextIndex));
     }
 
     private void updateFocusedIndex() {

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/MenuBarSkinTest.java

@@ -28,25 +28,29 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
 import com.sun.javafx.menu.MenuBase;
 import com.sun.javafx.stage.WindowHelper;
-import test.com.sun.javafx.pgstub.StubToolkit;
 import com.sun.javafx.tk.Toolkit;
+
 import javafx.beans.value.ObservableValue;
 import javafx.geometry.Insets;
 import javafx.scene.Group;
 import javafx.scene.Scene;
 import javafx.scene.control.Menu;
 import javafx.scene.control.MenuBar;
 import javafx.scene.control.MenuItem;
-import javafx.stage.Stage;
-
-import java.util.List;
 import javafx.scene.control.skin.MenuBarSkin;
-
-import org.junit.Before;
-import org.junit.BeforeClass;
-import org.junit.Test;
+import javafx.scene.control.skin.MenuBarSkinShim;
+import javafx.scene.input.KeyCode;
+import javafx.stage.Stage;
+import test.com.sun.javafx.pgstub.StubToolkit;
+import test.com.sun.javafx.scene.control.infrastructure.KeyEventFirer;
 
 /**
  * This fails with IllegalStateException because of the toolkit's check for the FX application thread
@@ -194,6 +198,16 @@ public class MenuBarSkinTest {
         }
     }
 
+    @Test
+    public void testInvisibleMenuNavigation() {
+        menubar.getMenus().get(0).setVisible(false);
+        MenuBarSkinShim.setFocusedMenuIndex(skin, 0);
+
+        KeyEventFirer keyboard = new KeyEventFirer(menubar);
+        keyboard.doKeyPress(KeyCode.LEFT);
+        tk.firePulse();
+    }
+
     public static final class MenuBarSkinMock extends MenuBarSkin {
         boolean propertyChanged = false;
         int propertyChangeCount = 0;