RFR: 8330304: MenuBar: Invisible Menu works incorrectly with keyboard arrows [v2]

[Andy Goryachev]

On Fri, 31 May 2024 12:35:53 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   review comments
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java line 735:
> 
>> 733:     // package protected for testing purposes
>> 734:     MenuBarButton menuBarButtonAt(int i) {
>> 735:         if (i < container.getChildren().size()) {
> 
> This method throws `IndexOutOfBoundsException` if the index is negative, but returns `null` if the index is larger than the collection size. None of the callers account for this, so we'd just get `NullPointerException` at the call sites.
> 
> Since you touched this method, I suggest replacing the implementation with
> 
> MenuBarButton menuBarButtonAt(int i) {
>     return (MenuBarButton)container.getChildren().get(i);
> }

yes, the suggested change is safe: the product code always guards against invalid indexes, while the test code always expects a non-null return value.

the only time where it might backfire is if a (new) test sets up an empty menu bar.  but we don't have such a test.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1467#discussion_r1622575628