RFR: 8345538: Robot.mouseMove doesn't clamp bounds on macOS when trying to move mouse off screen [v6]

Fri Jan 24 19:01:56 UTC 2025

On Fri, 24 Jan 2025 00:25:14 GMT, Alisen Chung <achung at openjdk.org> wrote:

>> Currently on macOS when mouseMove is given an offscreen coordinate to move the mouse to, mouseMove will physically clamp to the edge of the screen, but if you try to grab the mouse location immediately after by using MouseInfo.getPointerInfo().getLocation() you will get the value of the offscreen point.
>> 
>> Windows and linux do this clamping and coordinate handling for us, but new distributions may not necessarily handle clamping the same way, so Robot should be checking for clamping rather than delegating it to native.
>> 
>> This fix updates shared code to cache the screen bounds and adds a check to not exceed the bounds in mouseMove. The caching is done in the Robot constructor, so if the screen bounds changes the constructor must be called again to update to the new bounds.
>
> Alisen Chung has updated the pull request incrementally with one additional commit since the last revision:
> 
>   init finX, finY

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/java/awt/Robot.java line 1:

> 1: /*

Revert all the unrelated formatting changes.

src/java.desktop/share/classes/java/awt/Robot.java line 109:

> 107:  *
> 108:  * @author Robi Khan
> 109:  * @since 1.3

Do we have to modify these lines?

src/java.desktop/share/classes/java/awt/Robot.java line 133:

> 131:         init(GraphicsEnvironment.getLocalGraphicsEnvironment()
> 132:                 .getDefaultScreenDevice());
> 133:     }

Nothing's changed here… These look as unrelated to the problem that's being addressed.

src/java.desktop/share/classes/java/awt/Robot.java line 157:

> 155:      *                                  GraphicsDevice.
> 156:      * @see java.awt.GraphicsEnvironment#isHeadless
> 157:      * @see GraphicsDevice

It's exactly the same here: the formatting is changed, yet nothing's touched in this method.

You should avoid changing or reformatting lines of code which you don't touch… unless the purpose of a PR is to update formatting and perform some clean-ups.

src/java.desktop/share/classes/java/awt/Robot.java line 175:

> 173:         for (int i = 0; i < gs.length; i++) {
> 174:             allScreenBounds[i] = gs[i].getDefaultConfiguration().getBounds();
> 175:         }

This doesn't look right to me… `init` is an instance method, yet it initialises `allScreenBounds` which is shared among all instances.

In a way it makes sense to share the graphics environment / configuration. On the other hand how much would it affect the operation of robot if the bounds are fetched from `GraphicsEnvironment` only when needed?

What happens if the graphics configuration changes? Does the user need to create a new instance of the `Robot` class?

src/java.desktop/share/classes/java/awt/Robot.java line 234:

> 232:         for (Rectangle screenBounds : allScreenBounds) {
> 233:             int closestX = Math.min(Math.max(x, screenBounds.x), screenBounds.x + screenBounds.width);
> 234:             int closestY = Math.min(Math.max(y, screenBounds.y), screenBounds.y + screenBounds.height);

Shouldn't the width and height have `- 1`? I mean if the screen resolution is 1920×1080, the highest valid coordinates are 1919 and 1079 for <var>x</var> and <var>y</var> correspondingly.

src/java.desktop/share/classes/java/awt/Robot.java line 253:

> 251:                 leastXDiff = currXDiff;
> 252:                 leastYDiff = currYDiff;
> 253:             }

Both branches must be executed at the same time — you have adjust both <var>x</var> and <var>y</var> if the diff isn't 0.

Probably, min/max would work better here too.

test/jdk/java/awt/Robot/MouseMoveOffScreen.java line 2:

> 1: /*
> 2:  * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.

Suggestion:

It hasn't been integrated in 2024, so the year needs bumping up to 2025.

test/jdk/java/awt/Robot/MouseMoveOffScreen.java line 44:

> 42:         robot.mouseMove(STARTING_LOC.x, STARTING_LOC.y);
> 43:         robot.delay(500);
> 44:         robot.mouseMove(OFF_SCREEN_LOC.x, OFF_SCREEN_LOC.y);

You should validate to ensure your `OFF_SCREEN_LOC` is actually off screen.

test/jdk/java/awt/Robot/MouseMoveOffScreen.java line 50:

> 48:             throw new RuntimeException("Test Failed, getLocation returned null.");
> 49:         }
> 50:         Point currLoc = MouseInfo.getPointerInfo().getLocation();

Suggestion:

        Point currLoc = MouseInfo.getPointerInfo().getLocation();
        if (currLoc == null) {
            throw new RuntimeException("Test Failed, getLocation returned null.");
        }

You could save the the mouse location right away.

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

PR Review: https://git.openjdk.org/jdk/pull/22781#pullrequestreview-2573308137
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929109081
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929070058
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929071748
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929073747
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929100875
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929103712
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929107458
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929110307
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929111847
PR Review Comment: https://git.openjdk.org/jdk/pull/22781#discussion_r1929113298