On Fri, 21 Feb 2025 06:59:52 GMT, Gopal Pattnaik <d...@openjdk.org> wrote:
> There was no test included with the fix for > [JDK-8326989](https://bugs.openjdk.org/browse/JDK-8326989), > > Hence we are adding a system test now. > > Test is written as > 1. Load html content in web view. > 2. pick the color of mouse pointer. > 3. Perform double click. > 4. pick the color again. >> expected bahaviour: colour picked in step 2 and 4 should not match. > > Verification: > The test passes with latest webkit source > Also verified that test fails when the fix for > [JDK-8326989](https://bugs.openjdk.org/browse/JDK-8326989) is reverted. @Gopalora Please enable GHA tests on your repo. I'll provide a few comments now, and formally review it once your OCA status is verified. The latest changes look good. I'll formally review it once your OCA status is verified. tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java line 101: > 99: > 100: Util.runAndWait(() -> { > 101: robot.mouseMove(1, 1); Minor: use the utility method `Util.parkCursor(robot);` instead tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java line 110: > 108: Util.sleep(50); > 109: robot.mousePress(MouseButton.PRIMARY); > 110: robot.mouseRelease(MouseButton.PRIMARY); Minor: You can call `robot.mouseClick(MouseButton)` instead of separate calls to press/release. Not so minor: You shouldn't' sleep in the FX app thread. Instead, break this block into two separate lambdas passed into to separate calls to `runAndWait` with the sleep in between them running in the test thread. Like this: Util.runAndWait(() -> { ... robot.mouseClick(MouseButton.PRIMARY); }); Util.sleep(50); Util.runAndWait(() -> { robot.mouseClick(MouseButton.PRIMARY); }); Suggestion: consider creating a `doubleClick` utility method in the test `Util` class. tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java line 116: > 114: > 115: Util.runAndWait(() -> { > 116: robot.mouseMove(1, 1); Minor: use the utility method `Util.parkCursor(robot);` instead tests/system/src/test/java/test/util/Util.java line 340: > 338: * Makes double click of the mouse left button. > 339: */ > 340: public static void doubleClick(Robot robot, int x, int y) { Since this is now a general-purpose utility, I'd prefer to separate out the mouse move from the double-click (with the mouse move being done in the test itself) and just have this be `void doubleClick(Robot robot)`. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1719#issuecomment-2682052978 PR Comment: https://git.openjdk.org/jfx/pull/1719#issuecomment-2690829082 PR Review Comment: https://git.openjdk.org/jfx/pull/1719#discussion_r1965697976 PR Review Comment: https://git.openjdk.org/jfx/pull/1719#discussion_r1965712264 PR Review Comment: https://git.openjdk.org/jfx/pull/1719#discussion_r1965699563 PR Review Comment: https://git.openjdk.org/jfx/pull/1719#discussion_r1971745158
