On Tue, 9 Jan 2024 07:31:51 GMT, Karthik P K <k...@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:  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
