On Thu, 23 Feb 2023 07:36:43 GMT, Karthik P K <k...@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
