On Wed, 22 Jan 2025 03:42:56 GMT, Alexander Zvegintsev <azveg...@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. > > Alexander Zvegintsev has updated the pull request incrementally with one > additional commit since the last revision: > > getLocationOnScreen on EDT Looks good to me. I've run the test; both tests fail without the fix and *pass with the fix*. test/jdk/javax/swing/JPopupMenu/FocusablePopupDismissTest.java line 101: > 99: JFrame frame = new JFrame("FocusablePopupDismissTest"); > 100: JButton button = new JButton("Click me"); > 101: frame.add(button); May I suggest making the test window larger? You can make the button larger by setting its size or the size of the frame. Alternatively, you can add a wrapper panel with border around it: @@ -33,10 +33,12 @@ * @run main/manual FocusablePopupDismissTest */ +import javax.swing.BorderFactory; import javax.swing.JButton; import javax.swing.JFrame; import javax.swing.JMenu; import javax.swing.JMenuItem; +import javax.swing.JPanel; import javax.swing.JPopupMenu; import javax.swing.JTextField; import java.awt.Window; @@ -97,8 +99,12 @@ static JMenu getMenuWithMenuItem(boolean isSubmenuItemFocusable, String text) { static List<Window> createTestUI() { JFrame frame = new JFrame("FocusablePopupDismissTest"); + JPanel wrapper = new JPanel(); + wrapper.setBorder(BorderFactory.createEmptyBorder(16, 16, 16, 16)); + JButton button = new JButton("Click me"); - frame.add(button); + wrapper.add(button); + frame.add(wrapper); button.addActionListener(e -> { JPopupMenu popupMenu = new JPopupMenu(); The test becomes easier to use… Otherwise the small test UI frame gets somewhat lost near the large instructions. test/jdk/javax/swing/JPopupMenu/NestedFocusablePopupTest.java line 108: > 106: Point frameLocation = waitAndGetLocationOnEDT(frame); > 107: robot.mouseMove(frameLocation.x + frame.getWidth() / 2, > 108: frameLocation.y + frame.getHeight() / 2); Then `frame.getWidth()` and `frame.getHeight()` should also be called on EDT, shouldn't they? test/jdk/javax/swing/JPopupMenu/NestedFocusablePopupTest.java line 116: > 114: robot.mouseMove(menuLocation.x + 5, menuLocation.y + 5); > 115: > 116: // give popup some time to disappear (in case of failure) Suggestion: // Give popup some time to disappear (in case of failure) test/jdk/javax/swing/JPopupMenu/NestedFocusablePopupTest.java line 121: > 119: > 120: try { > 121: waitTillShown(popupMenu, 500); Huh, I see you're using `waitTillShown` for other purposes too… I wonder if a popup listener would do better. But then, it's over-thinking and over-engineering… ------------- Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22729#pullrequestreview-2573182808 PR Review Comment: https://git.openjdk.org/jdk/pull/22729#discussion_r1928998212 PR Review Comment: https://git.openjdk.org/jdk/pull/22729#discussion_r1929022755 PR Review Comment: https://git.openjdk.org/jdk/pull/22729#discussion_r1929024068 PR Review Comment: https://git.openjdk.org/jdk/pull/22729#discussion_r1929026835
