RFR: 8242616: Regression: RTL - TextField Cursor Movement Via Keyboard

[Andy Goryachev]

On Fri, 25 Aug 2023 11:41:22 GMT, Karthik P K <kpk at 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