On Thu, 20 Aug 2026 18:27:40 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: Disable mnemonic processing on mac I think you are getting close! This is a complicated subject. I've filed https://bugs.openjdk.org/browse/JDK-8390898 to make sure the extended mnemonic behavior is properly specified. There is also an existing ticket [JDK-8336590](https://bugs.openjdk.org/browse/JDK-8336590) Mnemonic Rendering incorrect (different) if Button has a graphic can you check if your fix solves that issue please? modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java line 1124: > 1122: > 1123: private void updateMnemonicRegistration() { > 1124: if (com.sun.javafx.PlatformUtil.isMac()) { please use the import statement instead of the FQCN modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java line 1130: > 1128: Scene skinnableScene = skinnable.getScene(); > 1129: Node mnemonicNode = skinnable; > 1130: if (skinnable instanceof Label l && l.getLabelFor() != null) { minor: might want to use a name other than l as it is similar to 1 or | modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java line 1131: > 1129: Node mnemonicNode = skinnable; > 1130: if (skinnable instanceof Label l && l.getLabelFor() != null) { > 1131: mnemonicNode = l.getLabelFor(); extra space modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java line 2189: > 2187: StringBuilder sb = new StringBuilder(); > 2188: > 2189: provide_testMnemonics_parameters().forEach(a -> { could this be converted to a simple parameterized test? or the sequence is important? modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java line 2252: > 2250: } > 2251: > 2252: private static <T> List<T> listOf(T... a) { isn't this a standard `List.of()`? modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java line 2339: > 2337: tk.firePulse(); > 2338: assertEquals(expectedMnemonics, > mnemonicRegistrationChecker.apply("t"), > mnemonicErrorMessageSupplier.apply(expectedMnemonics)); > 2339: assertEquals(expectedMnemonicsNode, > mnemonicNodePresenceChecker.get()); I think we still might have a problem. "Extended mnemonic" which is not publicly specified anywhere (which is another issue which I just filed https://bugs.openjdk.org/browse/JDK-8390898 ) means that the underline should be removed since the corresponding symbol may not be present. We'll have failures if we add assertFalse(mnemonicNodePresenceChecker.get()); everywhere the extended mnemonic is used. ------------- PR Review: https://git.openjdk.org/jfx/pull/2253#pullrequestreview-4996524959 PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3832933668 PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3832948055 PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3832940111 PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3832968209 PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3832977631 PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3833829464
