RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation

[Andy Goryachev]

On Tue, 9 Jan 2024 07:31:51 GMT, Karthik P K <kpk at openjdk.org> wrote:

> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation conditions were not considered, hence hit test values such as character index and insertion index values were incorrect.
> 
> Added checks for RTL orientation of nodes and  fixed the issue in `getHitInfo()` to calculate correct hit test values.
> 
> Added system tests to validate the changes.

Preliminary testing looks ok, though it does suffer from effects of JDK-8318095 (and I did remove `this.getScene().` in Text:1042)

I wonder if we should address JDK-8318095 first to be able to test this fix fully.  Right now there are scenarios when things fail after resizing the split:

![Screenshot 2024-01-10 at 11 48 39](https://github.com/openjdk/jfx/assets/107069028/3852b9dc-1d19-406f-bf39-b81790ec61ca)

modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1042:

> 1040:         int runIndex = 0;
> 1041:         if (runs.length != 0) {
> 1042:             if (this.getScene().getNodeOrientation() == NodeOrientation.RIGHT_TO_LEFT) {

I think this should not refer to scene:


if (getNodeOrientation() == NodeOrientation.RIGHT_TO_LEFT) {

modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java line 202:

> 200:             double x = point.getX();
> 201:             double y = point.getY();
> 202:             TextLayout.Hit h = layout.getHitInfo((float)x, (float)y, null, -1, -1);

-1 looks like magic value, could you please describe it in the `com.sun.javafx.scene.tex.TextLayout` javadoc in both cases (textRunStart and curRunStart)?

tests/system/src/test/java/test/robot/javafx/scene/RTLTextCharacterIndexTest.java line 48:

> 46: import javafx.stage.StageStyle;
> 47: import javafx.stage.Window;
> 48: import org.junit.After;

should we be using junit5 for all new tests?

tests/system/src/test/java/test/robot/javafx/scene/RTLTextCharacterIndexTest.java line 133:

> 131:         Util.runAndWait(() -> {
> 132:             Window w = scene.getWindow();
> 133:             robot.mouseMove((int) (w.getX() + scene.getX() + x),

is there a reason to cast to int?
fx robot does accept double arguments, and we might be dealing with fractional scale on some platforms

tests/system/src/test/java/test/robot/javafx/scene/RTLTextCharacterIndexTest.java line 149:

> 147:             textOne.setFont(new Font(48));
> 148:             vBox.getChildren().clear();
> 149:             vBox.getChildren().add(textOne);

setAll(...) does clear() and add(...)

tests/system/src/test/java/test/robot/javafx/scene/RTLTextCharacterIndexTest.java line 209:

> 207:             moveMouseOverText(x, 0);
> 208:             if (isLeading) {
> 209:                 Assert.assertEquals(charIndex, insertionIndex);

junit5: Assertions.assertEquals

tests/system/src/test/java/test/robot/javafx/scene/RTLTextFlowCharacterIndexTest.java line 135:

> 133:         Util.runAndWait(() -> {
> 134:             Window w = scene.getWindow();
> 135:             robot.mouseMove((int) (w.getX() + scene.getX() + x),

same comments as for RTLTextCharacterIndexTest...

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

PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1885746224
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1447887330
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1447893349
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1447919208
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1447917225
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1447918283
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1447919870
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1447925706