RFR: 8242616: Regression: RTL - TextField Cursor Movement Via Keyboard [v2]

[Karthik P K]

On Fri, 25 Aug 2023 14:56:24 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Karthik P K has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address review coments
>
> 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.

Sure. I'll wait till [PR#1220](https://github.com/openjdk/jfx/pull/1220) to get integrated and use `TextInputControlHelper` method.

> 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?

junit5 would be better. Updated code to use the same

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

Added description for tests.

> tests/system/src/test/java/test/robot/javafx/scene/TextFieldCursorMovementTest.java line 50:
> 
>> 48: public class TextFieldCursorMovementTest {
>> 49:     static CountDownLatch startupLatch = new CountDownLatch(1);
>> 50:     static CountDownLatch caretPositionLatch;
> 
> caretPositionLatch is unused

Removed unused caretPositionLatch

> 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?

Added test cases for mixed text, rtl and ltr text as well.

> 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"

Corrected typo

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1307292693
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1307293802
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1307292890
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1307294016
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1307293241
PR Review Comment: https://git.openjdk.org/jfx/pull/1222#discussion_r1307293413