8389970: StringIndexOutOfBoundsException when a text containing a mnemonic is removed by eduardsdv · Pull Request #2253 · openjdk/jfx

eduardsdv · 2026-08-07T16:50:41Z

This PR fixes the StringIndexOutOfBoundsException, that occurs when an empty text is set to a Labeled, that previously contained a mnemonic.

The reason for the error is that the updateDisplayedText(double, double), which also updates containsMnemonic flag, was not invoked if the text was empty. The value of this flag was still true but the index of the mnemonic character inside MnemonicInfo had already been updated to -1. This led to the StringIndexOutOfBoundsException in the line 611.

I fixed it by moving the call to updateDisplayedText(double, double) outside the if-clause, so that it is always called when the text is being laid out. This should not affect performance because the method already checks whether recalculation is required. If not, it exits quickly.

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

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-8389970: StringIndexOutOfBoundsException when a text containing a mnemonic is removed (Bug - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer) 🔄 Re-review required (review applies to 7fe6711d)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2253

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-08-07T16:51:51Z

👋 Welcome back eduardsdv! 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-08-07T16:54:14Z

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

openjdk · 2026-08-07T16:58:32Z

The total number of required reviews for this PR has been set to 2 based on the presence of this label: rfr. This can be overridden with the /reviewers command.

mlbridge · 2026-08-07T17:02:47Z

Webrevs

andy-goryachev-oracle · 2026-08-07T17:25:45Z

Thank you for logging the bug and offering to fix it!
The reproducer works fine with macOS; I'll test it on windows soon.

andy-goryachev-oracle

Looks like a good solution.

andy-goryachev-oracle · 2026-08-07T17:48:37Z

as I commented in the ticket, the mnemonic decorations are not visible on macOS so this issue does not occur there. It still needs to be tested on linux.

@arapte could you be the second reviewer please?

eduardsdv · 2026-08-17T07:23:17Z

It would be nice to have this reviewed. Could you be the second reviewer, @arapte?

arapte · 2026-08-17T09:24:01Z

+        // Have to do this just in case it needs to be recomputed
+        // This is especially important because it also recomputes the 'containsMnemonic' value,
+        // which is used by this method later on.
+        updateDisplayedText(w, h);
+
        if (ignoreText) {
            textWidth  = textHeight = 0;
            text.setText("");
        } else {
-            updateDisplayedText(w, h); // Have to do this just in case it needs to be recomputed
            textWidth  = snapSizeX(Math.min(text.getLayoutBounds().getWidth(),  wrapWidth));


ignoreText would be true in two cases, either

when the text is empty / null or

ContentDisplay.GRAPHIC_ONLY is true. refer LabeledSkinBase.isIgnoreText()

With this change, updateDisplayedText() gets invoked unconditionally even for the scenario when the text is non-empty, but needs to be ignored because ContentDisplay.GRAPHIC_ONLY is true.

Please check if this can cause any issue.

You are right.
This is my approach to fix the issue by calling the updateDisplayedText() regardless of the value of ignoreText. Otherwise, the containsMnemonic value may be stale.

I added additional tests to check combinations ContentDisplay.GRAPHIC_ONLY and ContentDisplay.TEXT_ONLY with empty and null text.

When ContentDisplay.GRAPHIC_ONLY is true, and text is non-empty: Calling updateDisplayedText() may result in invoking addMnemonic(); and getChildren().add(mnemonic_underscore);. [ refer the method updateDisplayedText() , unable to share links due to Github outage ]
This can result in addition of an un-required node to scenegraph.

The proposed change fixes the exception, but it seems to be introducing a regression.

Perhaps another option would be to reset the containsMnemonic flag in the if (ignoreText) block rather than calling updateDisplayedText?

I also tested a new condition (!ignoreText) in the layoutLabelInArea() method in the following if-clause.
The test are running green and also no additional children are added in case of ContentDisplay.GRAPHIC_ONLY.

Would this be a preferable solution?

Point2D mnemonicPos = null; double mnemonicWidth = 0.0; double mnemonicHeight = 0.0; if (!ignoreText && containsMnemonic) { final Font font = text.getFont(); String preSt = mnemonicInfo.getText(); boolean isRTL = (labeledNode.getEffectiveNodeOrientation() == NodeOrientation.RIGHT_TO_LEFT); mnemonicPos = Utils.computeMnemonicPosition(font, preSt, mnemonicInfo.getMnemonicIndex(), this.wrapWidth, labeled.getLineSpacing(), isRTL); mnemonicWidth = Utils.computeTextWidth(font, preSt.substring(mnemonicInfo.getMnemonicIndex(), mnemonicInfo.getMnemonicIndex() + 1), 0); mnemonicHeight = Utils.computeTextHeight(font, "_", 0, text.getBoundsType()); }

That might be OK, but I'll defer to @arapte and @andy-goryachev-oracle

alternatively, to keep the changes to a minimum, we only need to clear containsMnemonic in updateDisplayedText() in case of ContentDisplay.GRAPHIC_ONLY ?

to keep the changes to a minimum

This is the key point.

The StringIndexOutOfBoundsException occurs because the mnemonicIndex is -1.

Since the mnemonicIndex is checked for being >= 0 in other places in javafx.scene.control.skin.LabeledSkinBase and com.sun.javafx.scene.control.skin.Util classes, I decided to move the updateDisplayedText() back behind the ignoreText condition and to add a new mnemonicIndex >=0 condition to the if-clause instead.

This avoids the error and minimizes the risk of regressions.

Point2D mnemonicPos = null; double mnemonicWidth = 0.0; double mnemonicHeight = 0.0; int mnemonicIndex = mnemonicInfo != null ? mnemonicInfo.getMnemonicIndex() : -1; if (containsMnemonic && mnemonicIndex >= 0) { final Font font = text.getFont(); String preSt = mnemonicInfo.getText(); boolean isRTL = (labeledNode.getEffectiveNodeOrientation() == NodeOrientation.RIGHT_TO_LEFT); mnemonicPos = Utils.computeMnemonicPosition(font, preSt, mnemonicIndex, this.wrapWidth, labeled.getLineSpacing(), isRTL); mnemonicWidth = Utils.computeTextWidth(font, preSt.substring(mnemonicIndex, mnemonicIndex + 1), 0); mnemonicHeight = Utils.computeTextHeight(font, "_", 0, text.getBoundsType()); }

andy-goryachev-oracle · 2026-08-18T17:46:54Z

+            assertTrue(containsMnemonicNode.get());
+        }
+
+        label.setContentDisplay(ContentDisplay.GRAPHIC_ONLY);


here we tested mnemonic behavior when content display was set to GRAPHIC_ONLY.

would it make sense to set the content display to TEXT_ONLY and re-check as a part of this test?

andy-goryachev-oracle · 2026-08-18T17:49:03Z

+        tk.firePulse();
+        assertFalse(containsMnemonicNode.get());
+
+        sl.dispose();


suggestion: to make sure the test cleans the environment even when it fails, you could create a private StageLoader stageLoader field instead of a local variable, and dispose of it if it's not null in @AfterEach.

alternatively, you'll need to dispose of it in a finally block.

andy-goryachev-oracle · 2026-08-18T17:57:27Z

 import com.sun.javafx.tk.Toolkit;
 import test.com.sun.javafx.scene.control.infrastructure.StageLoader;

+


unnecessary blank line, please remove.

andy-goryachev-oracle · 2026-08-18T17:59:05Z

+
+        // mnemonic -> empty
+        label.setText("");
+        tk.firePulse();


could we also check the mnemonic registration?
the registration should exist before L2171 and be removed after L2172

andy-goryachev-oracle · 2026-08-18T18:43:36Z

+        StageLoader sl = new StageLoader(label);
+        tk.firePulse();
+
+        label.setText("foo_bar");


I am getting SOOB exception if I do

label.setText("test_(t)");

here

Fix StringIndexOutOfBoundsException

7fe6711

eduardsdv changed the title ~~Fix StringIndexOutOfBoundsException~~ 8389970: Fix StringIndexOutOfBoundsException Aug 7, 2026

openjdk Bot added the rfr Ready for review label Aug 7, 2026

eduardsdv changed the title ~~8389970: Fix StringIndexOutOfBoundsException~~ 8389970: StringIndexOutOfBoundsException when a text containing a mnemonic is removed Aug 7, 2026

andy-goryachev-oracle approved these changes Aug 7, 2026

View reviewed changes

arapte suggested changes Aug 17, 2026

View reviewed changes

8389970: Add additional mnemonic tests

efd7370

eduardsdv requested a review from arapte August 17, 2026 14:30

eduardsdv added 4 commits August 18, 2026 15:25

8389970: Add mnemonicIndex check

e153414

8389970: Add import java.util.function.Supplier

8c7286c

8389970: Skip a test on Mac

e9f437b

8389970: Revert comment

b4fb814

andy-goryachev-oracle reviewed Aug 18, 2026

View reviewed changes

		import com.sun.javafx.tk.Toolkit;
		import test.com.sun.javafx.scene.control.infrastructure.StageLoader;

Uh oh!

Conversation

eduardsdv commented Aug 7, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Aug 7, 2026

Uh oh!

openjdk Bot commented Aug 7, 2026

Uh oh!

openjdk Bot commented Aug 7, 2026

Uh oh!

mlbridge Bot commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

andy-goryachev-oracle commented Aug 7, 2026

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Aug 7, 2026

Uh oh!

eduardsdv commented Aug 17, 2026

Uh oh!

arapte Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

eduardsdv Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

eduardsdv commented Aug 7, 2026 •

edited by openjdk Bot

Loading

mlbridge Bot commented Aug 7, 2026 •

edited

Loading

arapte Aug 17, 2026 •

edited

Loading

eduardsdv Aug 17, 2026 •

edited

Loading