On Thu, 23 Feb 2023 08:52:19 GMT, John Hendrikx <jhendr...@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