RFR: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly [v5]

[Andy Goryachev]

On Thu, 23 Feb 2023 08:52:19 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

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

this is undocumented, I think, but the caret path can be one of the three things:
1. a single point (MoveTo, I think)
2. a single line - MoveTo followed by a LineTo
3. two separate lines (split caret) - MoveTo, LineTo, MoveTo, LineTo (split caret is explicitly ignored on line 252)

One would think that the caret width can be changed  by CSS, but it is not easy.  The caretPath on TextInputControl:184 has no CSS class name set, so the width cannot be changed trivially via a stylesheet.  I suppose the application code can dig the internals looking for paths and setting widths manually, but this would be a highly unreliable move.

So I think this caretWidth can be other than 1.0 (line 233) for the purposes of this PR.

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

I would also recommend testing the code on Windows with the screen scale set to 225%, as it might show issues related to fractional scale.

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

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