8327478: Add System test to verify TextSelection issue for webkit-617.1 #1719

Gopalora · 2025-02-21T06:59:52Z

There was no test included with the fix for JDK-8326989,

Hence we are adding a system test now.

Test is written as

Load html content in web view.
pick the color of mouse pointer.
Perform double click.
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 is reverted.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8327478: Add System test to verify TextSelection issue for webkit-617.1 (Bug - P3)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Andy Goryachev (@andy-goryachev-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1719/head:pull/1719
$ git checkout pull/1719

Update a local copy of the PR:
$ git checkout pull/1719
$ git pull https://git.openjdk.org/jfx.git pull/1719/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1719

View PR using the GUI difftool:
$ git pr show -t 1719

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1719.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-21T07:00:10Z

Hi @Gopalora, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user Gopalora" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2025-02-21T07:00:37Z

@Gopalora This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8327478: Add System test to verify TextSelection issue for webkit-617.1

Reviewed-by: kcr, angorya

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 8 new commits pushed to the master branch:

fc770fb: 8349091: Charts: exception initializing in a background thread
1824db5: 8299753: Tree/TableView: Column Resizing With Fractional Scale
f3ba448: 8233179: VetoableListDecorator#sort throws IllegalArgumentException "duplicate children"
7a7854c: 8349750: [TestBug] NodeInitializationStressTest: remaining Nodes
4f0f992: 8347753: VetoableListDecorator doesn't accept its own sublists for bulk operations
f38ab33: 8349924: Additional WebKit 620.1 fixes from WebKitGTK 2.46.6
0555fb2: 8349472: Update copyright header for files modified in 2025
163bf6d: 8350437: [GHA] Update gradle wrapper-validation action to v3

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @andy-goryachev-oracle) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Gopalora · 2025-02-24T08:09:05Z

/covered

bridgekeeper · 2025-02-24T08:09:44Z

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

kevinrushforth · 2025-02-25T13:52:58Z

@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.

kevinrushforth · 2025-02-21T15:38:27Z

tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java

+        int y = (int)(scene.getWindow().getY() + scene.getY() + 15);
+
+        Util.runAndWait(() -> {
+            robot.mouseMove(1, 1);


Minor: use the utility method Util.parkCursor(robot); instead

kevinrushforth · 2025-02-21T15:39:29Z

tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java

+        Util.sleep(500);
+
+        Util.runAndWait(() -> {
+            robot.mouseMove(1, 1);


Minor: use the utility method Util.parkCursor(robot); instead

kevinrushforth · 2025-02-21T15:43:08Z

tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java

+            robot.mousePress(MouseButton.PRIMARY);
+            robot.mouseRelease(MouseButton.PRIMARY);
+            Util.sleep(50);
+            robot.mousePress(MouseButton.PRIMARY);
+            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.

Gopalora · 2025-02-26T09:18:34Z

Thanks for the review.
I have done the code change as per the suggestions in the code file.
Please have a relook of the changes.

kevinrushforth · 2025-02-26T14:54:06Z

tests/system/src/test/java/test/util/Util.java

+    /**
+     * Makes double click of the mouse left button.
+     */
+    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).

kevinrushforth · 2025-02-28T14:48:45Z

The latest changes look good. I'll formally review it once your OCA status is verified.

mlbridge · 2025-02-28T21:07:36Z

Webrevs

andy-goryachev-oracle · 2025-02-28T21:36:09Z

tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java

+
+public class TextSelectionTest {
+
+    private static final String html = "<html>" +


we could use new java text block here

private static final String html = """ <html> <head></head> <body>&nbsp&nbsp&nbsp&nbsp some text</body> </html> """;

andy-goryachev-oracle · 2025-02-28T21:37:38Z

tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java

+import org.junit.jupiter.api.Timeout;
+import test.util.Util;
+
+public class TextSelectionTest {


I wonder if we could extend RobotTestBase base class to simplify the code?

andy-goryachev-oracle · 2025-02-28T21:39:06Z

tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java

+    public void testTextSelection() throws Exception {
+
+        int x = (int)(scene.getWindow().getX() + scene.getX() + 22);
+        int y = (int)(scene.getWindow().getY() + scene.getY() + 15);


can these offsets (22, 15) be measured instead of hard-coded?

We are not getting the text area from web view. This point is used to simulate the mouse click.

so they are arbitrary, and will work for every font size?

(The default font size varies depending on the platform and other circumstances, see PrismFontFactory::getSystemFontSize() - though I am not sure if the default font size is picked up by the WebView.

Tested in font size 1 and 5 separately. In both case it passed. For this test case, set font size 2 for test purpose only.

kevinrushforth

Looks good with one minor code style issue.

kevinrushforth · 2025-03-04T12:14:43Z

tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java

+import test.robot.testharness.RobotTestBase;
+import test.util.Util;
+
+public class TextSelectionTest extends RobotTestBase{


Minor: please add a space before the {

kevinrushforth · 2025-03-04T12:22:11Z

tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java

+    """
+    <html>
+    <head></head>
+    <body>&nbsp&nbsp&nbsp&nbsp some text</body>
+    </html>
+    """;


One more minor code style issue: lines 45-50 should be intended

Done as per one suggestion on "https://docs.oracle.com/en/java/javase/18/text-blocks/index.html"

Removed the line "" as this is empty.

andy-goryachev-oracle · 2025-03-04T15:43:52Z

tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java

+    """;
+
+    private static CountDownLatch webviewLoadLatch = new CountDownLatch(1);
+    private Color colorBefore;


suggestion: make colorBefore/After volatile since they are being accessed from different threads without any synchronization.
I think this should be sufficient (no need for AtomicReference).

This isn't strictly needed, since runAndWait synchronizes the two threads (although there is no harm in adding volatile).

https://en.wikipedia.org/wiki/Cache_coherence

edit: you might be right about runAndWait, we know it does separate the access from multiple threads in time, and it might be even smart enough to figure out that the field is being accessed from two different threads and emit the appropriate bytecode (similarly how we don't need the final keyword for effectively final variables nowadays).

But I think that adding volatile guarantees that all the necessary steps are done by the JVM.

https://en.wikipedia.org/wiki/Cache_coherence

That's interesting, but not particularly relevant. What is relevant is the JVM spec, specifically the Java Memory Model, which guarantees a happens-before ordering of actions in two threads that synchronize-with each other.

edit: you might be right about runAndWait, we know it does separate the access from multiple threads in time, and it might be even smart enough to figure out that the field is being accessed from two different threads and emit the appropriate bytecode (similarly how we don't need the final keyword for effectively final variables nowadays).

It isn't about the byte code that is emitted as much as it is about how the code is executed by the two different threads. And it isn't because it need to be "smart enough", it's because that's how the memory model works.

A write by the thread that executes the runnable passed into runAndWait (the FX app thread in this case), happens-before the read done by the test thread that is done after the runAndWait returns because the runAndWait call causes the actions done in the runAndWait's runnable to synchronize-with the wait done by the calling thread.

But I think that adding volatile guarantees that all the necessary steps are done by the JVM.

Yes, adding volatile guarantees the appropriate happens-before relationship. But so does synchronizing.

I don't really care one way or the other in this case.

andy-goryachev-oracle · 2025-03-04T15:45:16Z

tests/system/src/test/java/test/robot/javafx/web/TextSelectionTest.java

+        Util.parkCursor(robot);
+        Util.runAndWait(() -> colorAfter = robot.getPixelColor(x, y));
+
+        Assertions.assertNotEquals(colorBefore, colorAfter,


should we also test for non-null colorBefore ?

Robot::getPixelColor can't return null (although I checked the docs and we don't specify one way or the other). It seems fine either way.

what if the lambdas never run?

Then we have far bigger problems than this one test. The reason we use runAndWait instead of runLater is so we guarantee that the lambda has run before that method returns. It's guaranteed to do so.

Added volatile to the variables colorBefore and colorAfter.

kevinrushforth

Looks good.

andy-goryachev-oracle

lgtm, thank you for addressing the comments.

Gopalora · 2025-03-06T05:34:45Z

/integrate

openjdk · 2025-03-06T05:35:14Z

@Gopalora
Your change (at version d05cbf0) is now ready to be sponsored by a Committer.

aghaisas · 2025-03-06T05:39:25Z

/sponsor

openjdk · 2025-03-06T05:39:57Z

Going to push as commit 0a20fef.
Since your change was applied there have been 8 commits pushed to the master branch:

fc770fb: 8349091: Charts: exception initializing in a background thread
1824db5: 8299753: Tree/TableView: Column Resizing With Fractional Scale
f3ba448: 8233179: VetoableListDecorator#sort throws IllegalArgumentException "duplicate children"
7a7854c: 8349750: [TestBug] NodeInitializationStressTest: remaining Nodes
4f0f992: 8347753: VetoableListDecorator doesn't accept its own sublists for bulk operations
f38ab33: 8349924: Additional WebKit 620.1 fixes from WebKitGTK 2.46.6
163bf6d: 8350437: [GHA] Update gradle wrapper-validation action to v3

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-06T05:40:05Z

@aghaisas @Gopalora Pushed as commit 0a20fef.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Add System Test TextSelectionTest

d669662

bridgekeeper bot added the oca Needs verification of OCA signatory status label Feb 21, 2025

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Feb 24, 2025

kevinrushforth reviewed Feb 25, 2025

View reviewed changes

Addressed Review comments

012f034

kevinrushforth reviewed Feb 26, 2025

View reviewed changes

Gopalora added 2 commits February 27, 2025 11:57

Empty Commit to test actions.

56d61ae

Addressed Review comments

8f0a0bd

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Feb 28, 2025

openjdk bot added the rfr Ready for review label Feb 28, 2025

andy-goryachev-oracle reviewed Feb 28, 2025

View reviewed changes

Gopalora added 3 commits March 3, 2025 15:54

Addressed Review comments

830c6d2

Addressed Review comments

af570f7

Addressed Review comments

b483624

kevinrushforth self-requested a review March 3, 2025 14:29

kevinrushforth reviewed Mar 4, 2025

View reviewed changes

andy-goryachev-oracle reviewed Mar 4, 2025

View reviewed changes

Addressed Review comments

d05cbf0

kevinrushforth approved these changes Mar 5, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Mar 5, 2025

andy-goryachev-oracle approved these changes Mar 5, 2025

View reviewed changes

openjdk bot added the sponsor Ready to sponsor label Mar 6, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 6, 2025

openjdk bot closed this Mar 6, 2025

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Mar 6, 2025


		public class TextSelectionTest {

		private static final String html = "<html>" +

8327478: Add System test to verify TextSelection issue for webkit-617.1 #1719

8327478: Add System test to verify TextSelection issue for webkit-617.1 #1719

Uh oh!

Conversation

Gopalora commented Feb 21, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 21, 2025

Uh oh!

openjdk bot commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gopalora commented Feb 24, 2025

Uh oh!

bridgekeeper bot commented Feb 24, 2025

Uh oh!

kevinrushforth commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gopalora commented Feb 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrushforth commented Feb 28, 2025

Uh oh!

mlbridge bot commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Gopalora commented Feb 21, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 21, 2025 •

edited

Loading

mlbridge bot commented Feb 28, 2025 •

edited

Loading

andy-goryachev-oracle Mar 4, 2025 •

edited

Loading

andy-goryachev-oracle left a comment •

edited

Loading