RFR: 8282290: TextField Cursor Position one off [v2]

[Andy Goryachev]

On Thu, 16 Nov 2023 06:54:04 GMT, Karthik P K <kpk at openjdk.org> wrote:

>> The change listener on caretPositionProperty() was not getting invoked on replacing the content with same text as there was no change in caret position. Since the textProperty invalidation sets the forward bias to true by default, incorrect caret position was  calculated when the same text was replaced after clicking on the trailing edge of last character or empty space in the TextField.
>> 
>> Since caretShapeProperty invalidation listener gets invoked without changing the caret position, updating the caretBiasProperty on this listener solves the issue.
>> 
>> Since the caret position value will be same when the caret is present after the last character or before the last character, it can not be validated using unit test.
>> The fix can be validated using MonkeyTester.
>> Steps to select TextField option in Monkey Tester.
>> 
>> - Open the MonkeyTester app and select TextField from the left option pane.
>> - Select Short from Text selection option and click on the TextField to bring it into focus.
>> - Select all using cmd(ctrl) + a and copy using cmd(ctrl) + c
>> - Click on empty space in the TextField after the present content.
>> - Select all again using the keyboard shortcut and paste using cmd(ctrl) + v
>> - The caret should be displayed after the last character. Without the fix caret gets displayed before the last character.
>
> Karthik P K has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Code review

looks good, with a minor comment.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java line 248:

> 246:         textNode.caretShapeProperty().addListener(observable -> {
> 247:             caretPath.getElements().setAll(textNode.caretShapeProperty().get());
> 248:             if (caretPath.getElements().size() != 4) {

it seems to work correctly in the presence of RTL text.

The full test is of course blocked by https://bugs.openjdk.org/browse/JDK-8296266 , but this scenario requires only mouse clicks and the DELETE key.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java line 248:

> 246:         textNode.caretShapeProperty().addListener(observable -> {
> 247:             caretPath.getElements().setAll(textNode.caretShapeProperty().get());
> 248:             if (caretPath.getElements().size() != 4) {

could you add a comment explaining why != 4 is here for the future reference.

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

Marked as reviewed by angorya (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1287#pullrequestreview-1758106745
PR Review Comment: https://git.openjdk.org/jfx/pull/1287#discussion_r1411058514
PR Review Comment: https://git.openjdk.org/jfx/pull/1287#discussion_r1411064703