On Wed, 19 Aug 2026 14:42:12 GMT, eduardsdv <[email protected]> wrote:
>> 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. >> >> --------- >> - [X] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > eduardsdv has updated the pull request incrementally with one additional > commit since the last revision: > > 8389970: Add mnemonic registration even if ContentDisplay==GRAPHIC_ONLY What looked like a simple change proved to be exceedingly tricky... I wonder if we are hitting pre-existing design issues. For example, what I might have done is to eagerly discard mnemonicInfo when any properties that affect it change, and re-create it each time it's needed (making sure not to lose sync with the scene mnemonics). The fact that we have SOOBEs and NPEs indicates that we are using stale values, so perhaps we need to make sure we re-compute them correctly. What do you think? modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java line 97: > 95: public void teardown() { > 96: if (stageLoader != null) { > 97: stageLoader.dispose(); `stageLoader = null;` to avoid using stale stageLoader left over from an earlier run? modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java line 2127: > 2125: checkMnemonics(); > 2126: } > 2127: I was trying to check whether mnemonic is registered with the scene, but this test fails with NPE before that: @Test public void withGraphic() { assumeFalse(PlatformUtil.isMac()); label.setText("_test"); label.setMnemonicParsing(true); label.setGraphic(new Label()); label.setContentDisplay(ContentDisplay.GRAPHIC_ONLY); stageLoader = new StageLoader(label); Toolkit.getToolkit().firePulse(); // NPE modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java line 2166: > 2164: > 2165: // Mac does not support mnemonics > 2166: boolean expectedMnemonics = PlatformUtil.isMac() ? false : > label.isMnemonicParsing(); could we also check whether mnemonic is actually registered with the scene, using `label.getScene().getMnemonic()`? modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java line 2261: > 2259: > 2260: private StageLoader createStageLoader(Node node) { > 2261: if (stageLoader == null) { this works only if `stageLoader` is null'ed in `@AfterEach` ------------- Changes requested by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/2253#pullrequestreview-4975919991 PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3816036622 PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3816781363 PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3816760533 PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3816767347
