8380935: TextFieldSkin caret width calculation is broken by crschnick · Pull Request #2128 · openjdk/jfx

crschnick · 2026-03-25T17:53:26Z

This fixes a regression caused by the fix for https://bugs.openjdk.org/browse/JDK-8282290. The old if else block was broken by the other PR, which caused the calculation for the caret width to be never run.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8380935: TextFieldSkin caret width calculation is broken (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2128/head:pull/2128
$ git checkout pull/2128

Update a local copy of the PR:
$ git checkout pull/2128
$ git pull https://git.openjdk.org/jfx.git pull/2128/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2128

View PR using the GUI difftool:
$ git pr show -t 2128

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2128.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-03-25T17:55:04Z

👋 Welcome back crschnick! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-03-25T17:56:13Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

mlbridge · 2026-03-25T18:00:12Z

Webrevs

andy-goryachev-oracle · 2026-03-25T18:22:01Z

/reviewers 2

openjdk · 2026-03-25T18:22:27Z

@andy-goryachev-oracle
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

andy-goryachev-oracle · 2026-03-25T18:24:01Z

Looks like a number of tests is currently failing. Also, could you add a unit test specifically for this fix?

Maran23 · 2026-03-25T18:40:26Z

            } else if (caretPath.getElements().size() == 4) {
                // The caret is split. Ignore and keep the previous width value.
-            } else {
-                caretWidth = Math.round(caretPath.getLayoutBounds().getWidth());


I'm confused, isn't this effectively dead code?

if (!4) { .. } elseif (4) { .. } else { when do we come here? }

Yes, it was. But it shouldn't have been

Thanks for confirming. Yeah agree, that looks wrong.

Looking at the code review for the commit that caused this, that is a perfect example of how diffs can mislead. The fact that it introduced dead code was not visible from the diffs, only if you looked at the whole context.

And I even made a (IMO good) suggestion to improve the if condition 🙂
-> I made this comment because I felt like the if-else condition is confusing. So turns out, it is.

I would therefore recommend to improve it now and remove the else block. It is empty anyway.

I think it's out of scope for this PR, for several reasons.

This code relies on current, undocumented behavior of the caret shape being either a single line or two lines encoded as MoveTo-LineTo pairs. There are plans to change the caret shape for certain BIDI/RTL scenarios to indicate the direction, so the old assumptions might not hold anymore.

Also, in jfx25 we've added LayoutInfo/CaretInfo APIs in the Text and TextFlow, which should provide the caret information without relying on implementation details.

related umbrellas:
https://bugs.openjdk.org/browse/JDK-8343557
https://bugs.openjdk.org/browse/JDK-8300569

There are plans to change the caret shape for certain BIDI/RTL scenarios to indicate the direction, so the old assumptions might not hold anymore.

When that happens, this code of course need to be adjusted. But they will probably require more code changes.
In any case, the empty else if block does not make sense

This is an area of constant disagreement: I think it's ok to have minor cleanup like this one here, while @kevinrushforth much prefers to keep the PR focused on the fix and create a separate follow-up tickets when required, the rationale being backporting and merge conflicts. Piling up unrelated changes also delays the fix.

I think the newest version looks very good and should still be easy to e.g. backport. And is now more clear, with variables that express the purpose (which makes comments obsolete, good!).

crschnick · 2026-03-25T18:41:50Z

I think the tests are wrong. I looked at the logic for the caret at

jfx/modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java

Line 868 in d695b31

private void updateCaretOff() {

and if I understand it correctly, if the caret is fully to the left, the text should be shifted by caretWidth / 2. The tests assume that the text is not shifted in this case, which I don't think is accurate. If that would be the case, then the caret would be invisible

Maran23 · 2026-03-25T19:02:35Z

                 * The caret pos is invalid in this case,
                 * hence it should be updated when caret path size is not 4 */
+
+                if (caretPath.getElements().size() != 0) {


!caretPath.getElements().isEmpty()

I refactored it to make it easier to understand what the condition is checking now

andy-goryachev-oracle · 2026-03-26T14:55:16Z

Reviewers: @andy-goryachev-oracle

andy-goryachev-oracle

Looks like the problem is fixed on macOS, but it should be tested on Windows at fractional scales (I suspect the fix is not exactly right for fractional scales).

Also, it would help to provide a bit more information about the fix in the PR description (explain the change).

andy-goryachev-oracle · 2026-03-26T15:31:30Z

                 * hence it should be updated when caret path size is not 4 */
+
+                if (caretPath.getElements().size() != 0) {
+                    caretWidth = Math.round(caretPath.getLayoutBounds().getWidth());


I wonder if this should be Region.snapSizeX() instead of Math.round():
not only Math.round() may round downward, but also produce mid-pixel results on systems with fractional scale.

Can you double check it on Windows at fractional scales (125%, 150%, 175%, 225%)?

Since caretWidth is not directly used here, but rather in other code to calculate the translation or related, it is very likely fine here, but the target values in the other calculations may need to be snapped.

The caretWidth value is included in various calculations, and the results of those are snapped. So I think this works correctly. This PR just restores the old behaviour before it was broken.

I will check on Windows for various scales

Ok so Andy was right, the caret is sometimes still hidden on the right on different display scales. But even snapping the caret width does not always fix it. It requires a bigger fix for the skin in general.

However, I would argue that this is a different issue unrelated to this. I could open a follow-up to make the TextFieldSkin handle the display scale properly

+1 for a follow up. I think not all final values that are later set in the translate etc. are snapped as of now. So we probably need to catch all locations at once.

Do I understand you correctly - the issue is still seen with the fix at fractional scales? If so, then it would make sense to fix it in this PR.

Yes, the issue of the missing caret still exists at some fractional scales. However, I would say that this is a separate issue, more general to the whole skin implementation.

So there are two issues here, which cause the same problem in some scenarios, but the causes and potential fixes don't really have a lot to do with each other. This one just repairs a small part that was broken during and earlier PR. To fix the caret issue on fractional scales, that would require a more investigation.

The JBS issue focuses on the broken caret width calculation, not the missing caret behavior so I think splitting this up into a separate issue makes more sense

andy-goryachev-oracle · 2026-03-26T16:16:17Z

            } else if (caretPath.getElements().size() == 4) {
                // The caret is split. Ignore and keep the previous width value.
-            } else {
-                caretWidth = Math.round(caretPath.getLayoutBounds().getWidth());


I think it's out of scope for this PR, for several reasons.

This code relies on current, undocumented behavior of the caret shape being either a single line or two lines encoded as MoveTo-LineTo pairs. There are plans to change the caret shape for certain BIDI/RTL scenarios to indicate the direction, so the old assumptions might not hold anymore.

Also, in jfx25 we've added LayoutInfo/CaretInfo APIs in the Text and TextFlow, which should provide the caret information without relying on implementation details.

related umbrellas:
https://bugs.openjdk.org/browse/JDK-8343557
https://bugs.openjdk.org/browse/JDK-8300569

andy-goryachev-oracle · 2026-03-26T22:15:55Z

How would you propose to test this, from the user perspective?
Yes, it works, but only partially?
What is the goal here?

crschnick · 2026-03-26T22:21:36Z

The caret shows on non-fractional display scales, which is already better than before. The user can test this.

In the other cases where it still doesn't show, that is not due to this fix being wrong, but due to a completely separate issue of how the skin is implemented. That can be implemented in another, more large-scale issue. There are probably more issues caused by the skin not properly snapping values than just the caret not showing if you go looking for them at fractional scales.

Fix caret width calculation

d695b31

openjdk bot added the rfr Ready for review label Mar 25, 2026

Maran23 reviewed Mar 25, 2026

View reviewed changes

crschnick added 2 commits March 25, 2026 19:44

Adjust tests

373c715

Adjust tests

3ef5366

Maran23 reviewed Mar 25, 2026

View reviewed changes

andy-goryachev-oracle reviewed Mar 26, 2026

View reviewed changes

Clarify if else flow

4e420ea

Conversation

crschnick commented Mar 25, 2026 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Mar 25, 2026

Uh oh!

openjdk bot commented Mar 25, 2026

Uh oh!

mlbridge bot commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

andy-goryachev-oracle commented Mar 25, 2026

Uh oh!

openjdk bot commented Mar 25, 2026

Uh oh!

andy-goryachev-oracle commented Mar 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crschnick commented Mar 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Mar 26, 2026

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crschnick commented Mar 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

crschnick commented Mar 25, 2026 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 25, 2026 •

edited

Loading

andy-goryachev-oracle commented Mar 26, 2026 •

edited

Loading