On Fri, 8 May 2020 12:21:48 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote:
> Issue : > https://bugs.openjdk.java.net/browse/JDK-8244418 > > Root Cause : > Incorrect assumption about menu list size. > > Fix : > Added check for empty menu list before trying to access it. > > Test : > Added a unit test that fails before fix and passes after it. looks good - just two inline questions :) modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 307: > 306: unSelectMenus(); > 307: if (!container.getChildren().isEmpty()) { > 308: menuModeStart(0); wondering whether this would be an appropriate time to simplify the logic a bit: as unSelectMenus is called in both if and else block, it could be condensed to unSelectMenus(); if (t1 && !container.getChildren().isEmpty()) { ... } might overlook something, though modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 485: > 484: } > 485: > 486: if (focusedMenu != null && focusedMenuIndex != -1) { don't quite understand why this change is necessary? The guard in the listener seems to be enough, at least the test passes without this. If it's needed to fix another potentially breaking path to this, would it be possible to add a test method that fails/passes before/after? ------------- PR: https://git.openjdk.java.net/jfx/pull/216
