On Fri, 25 Aug 2023 11:41:22 GMT, Karthik P K <k...@openjdk.org> wrote:
> The old logic for cursor movement was buggy when both RTL and LTR text was
> present in the TextField. Used character BreakIterator instead of finding the
> character index using hitTest.
>
> Added system test to validate the fix.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
line 590:
> 588: int next = moveRight ? bi.following(pos) : bi.preceding(pos);
> 589: if (next != BreakIterator.DONE) {
> 590: textField.selectRange(next, next);
I might suggest making this PR depend on
https://github.com/openjdk/jfx/pull/1220 and make use of
`TextInputControlHelper.charIterator()` to get the instance cached in
TextInputControl.
tests/system/src/test/java/test/robot/javafx/scene/TextFieldCursorMovementTest.java
line 26:
> 24: */
> 25:
> 26: package test.robot.javafx.scene;
thank you for adding a test case!
tests/system/src/test/java/test/robot/javafx/scene/TextFieldCursorMovementTest.java
line 44:
> 42: import org.junit.Assert;
> 43: import org.junit.BeforeClass;
> 44: import org.junit.Test;
should we use junit5 instead?
tests/system/src/test/java/test/robot/javafx/scene/TextFieldCursorMovementTest.java
line 48:
> 46: import test.util.Util;
> 47:
> 48: public class TextFieldCursorMovementTest {
I'd recommend to add a description of the test in a javadoc.
tests/system/src/test/java/test/robot/javafx/scene/TextFieldCursorMovementTest.java
line 58:
> 56:
> 57: static int curIndex = 0;
> 58: static int prevIndex = -1;
using static fields and @BeforeClass and @AfterClass prevents us from adding a
second (third, ...) test case.
for example, we might want to check isRTL=false, and/or add a newline in the
text, and/or add modify test such that it wraps (should we?)
tests/system/src/test/java/test/robot/javafx/scene/TextFieldCursorMovementTest.java
line 69:
> 67: }
> 68:
> 69: private void addTextFieldContent(String text, boolean isRtl) {
do you think we should also test the case when isRtl = false?
tests/system/src/test/java/test/robot/javafx/scene/TextFieldCursorMovementTest.java
line 81:
> 79: @Test
> 80: public void testCursorMovementInRTLText() throws Exception {
> 81: String str = "Aracbic يشترشسيرشي";
spelling "Arabic"
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1305773633
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1305784402
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1305790292
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1305785328
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1305789385
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1305786270
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1305789702
