On Tue, 18 Aug 2026 17:57:27 GMT, Andy Goryachev <[email protected]> wrote:
>> eduardsdv has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8389970: Revert comment
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
> line 59:
>
>> 57: import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
>> 58:
>> 59:
>
> unnecessary blank line, please remove.
Done.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
> line 2131:
>
>> 2129: }
>> 2130:
>> 2131: 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?
Sure, I've added testing for content display TEXT_ONLY.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
> line 2143:
>
>> 2141: assertFalse(containsMnemonicNode.get());
>> 2142:
>> 2143: 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.
Done
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
> line 2159:
>
>> 2157: tk.firePulse();
>> 2158:
>> 2159: label.setText("foo_bar");
>
> I am getting SOOB exception if I do
>
> `label.setText("test_(t)");`
>
> here
Yes. I got the same error.
I've adjusted the code and the tests to catch this error in the future.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
> line 2172:
>
>> 2170: // mnemonic -> empty
>> 2171: label.setText("");
>> 2172: tk.firePulse();
>
> could we also check the mnemonic registration?
> the registration should exist before L2171 and be removed after L2172
Good point. I've added the check for the mnemonic registration to the scene.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3813222833
PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3813220104
PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3813221437
PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3813247700
PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3813234532
