RFR: 8178368: Right and Center alignment of text field works incorrectly [v5]

[John Hendrikx]

On Thu, 23 Feb 2023 07:36:43 GMT, Karthik P K <kpk at openjdk.org> wrote:

>> When Text width was more than TextField width, the logic to update `textTranslateX` in `updateCaretOff` method was causing the issue of unexpected behavior for Right and Center alignment.
>> 
>> Made changes to update `textTranslateX` in `updateCaretOff` method only when text width is less than text field width i.e `delta` is positive. 
>> For both right and center alignments, the `textTranslateX` value calculated in `updateTextPos` method will be updated without any condition so that expected behavior is achieved for all scenarios of text width relative to text field width. 
>> 
>> Added unit tests to validate LEFT, CENTER and RIGHT alignments. RIGHT and CENTER alignment tests are expected to fail without the fix provided in this PR.
>
> Karthik P K has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix text and prompt alignment issue

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

> 803:                 // appear at the left of the centered prompt.
> 804:                 newX = midPoint - promptNode.getLayoutBounds().getWidth() / 2;
> 805:                 if (newX > 0) {

Let's say `caretWidth / 2` is 5, and that would be position we would show the prompt text if it doesn't fit.

If the `newX` calculation is less than 5, but greater than 0, the prompt text would be further to the left than you'd expect.

So I think this line should be:

Suggestion:

                if (newX > caretWidth / 2) {


Probably best to put that in a local variable.

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

> 816:                 } else if (newX < 0 && oldX > 1) {
> 817:                     textTranslateX.set(caretWidth / 2);
> 818:                 }

I get the impression this code also needs to use `caretWidth / 2` instead of `0` and `1`.

ie:

Suggestion:

                // Update if there is space on the right
                if (newX + textNodeWidth <= textRight.get() - caretWidth / 2) {
                    textTranslateX.set(newX);
                } else if (newX < caretWidth / 2 && oldX > caretWidth / 2) {
                    textTranslateX.set(caretWidth / 2);
                }


It would only work for very narrow caret's otherwise, and for wide caret's it would have some unexpected positioning in the edge case where text almost fits.

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

> 829:                     // Align to left when prompt text length is more than text field width
> 830:                     promptNode.setLayoutX(caretWidth / 2);
> 831:                 }

Similar comment, I don't think its correct. It's just odd that promptNewX  may be smaller than caretWidth / 2 but is accepted, but when the text is too wide the "minimum" value suddently is caretWidth / 2.

Also, I don't see how `promptOldX` has any relevance when it comes to positioning the prompt.  I think this is only relevant for the actual text (because it can scroll depending on where the cursor is), not for the prompt -- unless the prompt can be scrolled as well.  If that's the case then the `CENTER` case may need to be updated to take `promptOldX` into account as well.  As it stands currently, they have behaviors that seem to conflict.

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

> 837:                 } else if (newX < 0 && oldX > 1) {
> 838:                   textTranslateX.set(caretWidth / 2);
> 839:                 }

Again, I don't think its correct.  It's just odd that `newX` may be smaller than `caretWidth / 2` but is accepted, but when the text is too wide the "minimum" value suddenly is `caretWidth / 2`.

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

PR: https://git.openjdk.org/jfx/pull/980