8244418: MenuBar: IOOB exception on requestFocus on empty bar

Reviewed-by: fastegal, kcr
openjdk · Jun 16, 2020 · fb962ac · fb962ac
1 parent bf2e972
commit fb962ac
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 16 deletions.
diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java
@@ -300,17 +300,15 @@ public MenuBarSkin(final MenuBar control) {
  }
  };
  menuBarFocusedPropertyListener = (ov, t, t1) -> {
- if (t1) {
+ unSelectMenus();
+ if (t1 && !container.getChildren().isEmpty()) {
  // RT-23147 when MenuBar's focusTraversable is true the first
  // menu will visually indicate focus
- unSelectMenus();
  menuModeStart(0);
  openMenuButton = ((MenuBarButton)container.getChildren().get(0));
  setFocusedMenuIndex(0);
  openMenuButton.setHover();
- } else {
- unSelectMenus();
- }
+ }
  };
  weakSceneKeyEventHandler = new WeakEventHandler<KeyEvent>(keyEventHandler);
  Utils.executeOnceWhenPropertyIsNonNull(control.sceneProperty(), (Scene scene) -> {
@@ -473,11 +471,14 @@ private void showMenu(Menu menu, boolean selectFirstItem) {
  }
  }
 
- private void setFocusedMenuIndex(int index) {
- this.focusedMenuIndex = index;
- focusedMenu = index == -1 ? null : getSkinnable().getMenus().get(index);
+ /**
+ * This method is package scoped as it is used in this class as well as for testing
+ */
+ void setFocusedMenuIndex(int index) {
+ focusedMenuIndex = (index >= -1 && index < getSkinnable().getMenus().size()) ? index : -1;
+ focusedMenu = (focusedMenuIndex != -1) ? getSkinnable().getMenus().get(index) : null;
 
- if (focusedMenu != null && focusedMenuIndex != -1) {
+ if (focusedMenuIndex != -1) {
  openMenuButton = (MenuBarButton)container.getChildren().get(focusedMenuIndex);
  openMenuButton.setHover();
  }

diff --git a/modules/javafx.controls/src/shims/java/javafx/scene/control/skin/MenuBarSkinShim.java b/modules/javafx.controls/src/shims/java/javafx/scene/control/skin/MenuBarSkinShim.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2020, 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
@@ -53,7 +53,12 @@ public static ContextMenuContent getMenuContent(MenuButton mb) {
  return cmc;
  }
 
- public static int getFocusedIndex(MenuBarSkin skin) {
+ public static int getFocusedMenuIndex(MenuBarSkin skin) {
  return skin.getFocusedMenuIndex();
  }
+
+ public static void setFocusedMenuIndex(MenuBarSkin skin, int index) {
+ skin.setFocusedMenuIndex(index);
+ }
+
 }
diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/MenuBarTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/MenuBarTest.java
@@ -114,10 +114,42 @@ protected void startApp(Parent root) {
  }
 
  @Test public void defaultConstructorButSetTrueFocusTraversable() {
-  menuBar.setFocusTraversable(true);
+ menuBar.setFocusTraversable(true);
  assertTrue(menuBar.isFocusTraversable());
  }
 
+ @Test public void testFocusOnEmptyMenubar() {
+ menuBar.setFocusTraversable(true);
+
+ AnchorPane root = new AnchorPane();
+ root.getChildren().add(menuBar);
+ startApp(root);
+
+ MenuBarSkin skin = (MenuBarSkin)menuBar.getSkin();
+ assertTrue(skin != null);
+
+ menuBar.getScene().getWindow().requestFocus();
+
+ int focusedIndex = MenuBarSkinShim.getFocusedMenuIndex(skin);
+ assertEquals(-1, focusedIndex);
+ }
+
+ @Test public void testSimulateTraverseIntoEmptyMenubar() {
+ menuBar.setFocusTraversable(true);
+
+ AnchorPane root = new AnchorPane();
+ root.getChildren().add(menuBar);
+ startApp(root);
+
+ MenuBarSkin skin = (MenuBarSkin)menuBar.getSkin();
+ assertTrue(skin != null);
+
+ // simulate notification from traversalListener
+ MenuBarSkinShim.setFocusedMenuIndex(skin, 0);
+ int focusedIndex = MenuBarSkinShim.getFocusedMenuIndex(skin);
+ assertEquals(-1, focusedIndex);
+ }
+
  @Test public void getMenusHasSizeZero() {
  assertEquals(0, menuBar.getMenus().size());
  }
@@ -293,8 +325,8 @@ protected void startApp(Parent root) {
  tk.firePulse();
 
  // check if focusedMenuIndex is reset to -1 so navigation happens.
- int focusedIndex = MenuBarSkinShim.getFocusedIndex(skin);
- assertEquals(focusedIndex, -1);
+ int focusedIndex = MenuBarSkinShim.getFocusedMenuIndex(skin);
+ assertEquals(-1, focusedIndex);
  }
 
  @Test public void testMenuOnShownEventFiringWithKeyNavigation() {
@@ -678,7 +710,7 @@ protected void startApp(Parent root) {
  assertFalse(menu1.isShowing());
 
  // check if focusedMenuIndex is 0 (menu1 is still in selected state).
- int focusedIndex = MenuBarSkinShim.getFocusedIndex(skin);
- assertEquals(focusedIndex, 0);
+ int focusedIndex = MenuBarSkinShim.getFocusedMenuIndex(skin);
+ assertEquals(0, focusedIndex);
  }
 }