On Fri, 13 Dec 2024 12:39:02 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> The previous [JDK-8319103](https://bugs.openjdk.org/browse/JDK-8319103) fix >> was not complete. >> >> The case where a menu item with a focusable component was not a direct child >> of a window was missing(failing the `if (window == >> oppositeWindow.getParent() ) {` check), so the ungrab event was posted >> prematurely. >> >> This can be fixed by adding `waylandWindowFocusListener` to all submenus in >> hierarchy. >> >> The manual test updated to use this case, and also added an automated test >> that checks that it didn't close prematurely. > > src/java.desktop/unix/classes/sun/awt/UNIXToolkit.java line 532: > >> 530: } >> 531: } >> 532: return false; > > If you like, you may use Stream API: > Suggestion: > > return Arrays.stream(window.getWindowFocusListeners()) > .anyMatch(fl -> fl == waylandWindowFocusListener); > > I haven't tested the code though. I didn't use the stream API on purpose to be a bit faster, since we're usually working with very small arrays (usually about 0-1 elements). > src/java.desktop/unix/classes/sun/awt/UNIXToolkit.java line 550: > >> 548: >> addWaylandWindowFocusListenerToWindow(oppositeWindow); >> 549: return; >> 550: } > > So it's possible now that several windows in a hierarchy will have the > listener added… Won't the listener be leaked? In other words, do we always > remove the listener? As far as my tests with an instrumented build show - yes, but I discovered another case not covered: for (int i = 0; i <= 1; i++) { JMenu menu = new JMenu("Menu " + i); menu.add(new JTextField("JTextField " + i)); popupMenu.add(menu); } Clicking on gnome text editor does not hide the menu if the second menu is open. I do have a fix for it, but will not rush to post it, as I do not have enough time to test it thoroughly. I will move this PR to draft status as I will be on vacation starting next week. > test/jdk/javax/swing/JPopupMenu/NestedFocusablePopupTest.java line 53: > >> 51: //test is valid only when running on Wayland. >> 52: return; >> 53: } > > I think it should rather throw `jtreg.SkippedException`. > > With [jtreg enhancements](https://github.com/openjdk/jtreg/pull/217) and > further reporting enhancements, we'll be able to see if tests aren't actually > executed. Ok, but this test will lose the ability to run standalone with just `java NestedFocusablePopupTest.java`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22729#discussion_r1884600821 PR Review Comment: https://git.openjdk.org/jdk/pull/22729#discussion_r1884598134 PR Review Comment: https://git.openjdk.org/jdk/pull/22729#discussion_r1884599302
