RFR: 8342096: Popup menus that request focus are not shown on Linux with Wayland [v4]

[Alexey Ivanov]

On Wed, 22 Jan 2025 03:42:56 GMT, Alexander Zvegintsev <azvegint at 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