On Thu, 16 Nov 2023 06:54:04 GMT, Karthik P K <k...@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
