RFR: 8373650: Test javax/swing/JMenuItem/6458123/ManualBug6458123.java fails because the check icons are not aligned properly as expected [v2]

[Alexey Ivanov]

On Fri, 2 Jan 2026 09:58:56 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> Check/radiobutton icon are not aligned properly in RTL. `WindowsMenuItemUI `uses `MenuItemLayoutHelper.layoutMenuItem` to do the layout which calls `doRTLColumnLayout `which calculates x positions in `calcXPositionsRTL `and then again aligns in `alignRects`. However, since in Windows historically radiobutton/check icon was not drawn or drawn below the menuitem image icon (since image icon and check icon was drawn in the same layout space and not separately) the aligned x position of check icons returned from `MenuItemLayoutHelper` was not correct but since `MenuItemLayoutHelper` alignment is used in other L&Fs also so we need to realign it in windows specific class i.e in WindowsIconFactory in paintIcon
>> 
>> Before fix
>> 
>> <img width="425" height="646" alt="image" src="https://github.com/user-attachments/assets/6aac649d-b099-4e11-ba9a-83c623034287" />
>> 
>> After fix
>> 
>> <img width="430" height="641" alt="image" src="https://github.com/user-attachments/assets/e0ea7e3e-d6cb-44a6-aa4f-78435f85d6fb" />
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Alignment fix

src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java line 922:

> 920:                                                 x + OFFSET,
> 921:                                                 (icon.getIconHeight() <= 16) ? y + OFFSET : (y + icon.getIconHeight() / 2), state);
> 922:                                     } else if (icon.getIconWidth() <= 16) {

I don't understand why painting an icon and check marks / radio bullets in RTL case depends on the width of the icon but there's no such dependency in LTR case.

Shouldn't the performed menu layout handle these cases?

src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java line 923:

> 921:                                                 (icon.getIconHeight() <= 16) ? y + OFFSET : (y + icon.getIconHeight() / 2), state);
> 922:                                     } else if (icon.getIconWidth() <= 16) {
> 923:                                         if ((c instanceof JMenuItem mi) && mi.getText().isEmpty()) {

The condition `(c instanceof JMenuItem mi)` is always `true` according to the assert at line 881:

https://github.com/openjdk/jdk/blob/2daf12edd24e641d4d7706d582994c2b3fe95e87/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java#L881

where `menuItem` is a field in `VistaMenuItemCheckIcon`, the type of the field is `JMenuItem`.

This means, you can use `menuItem` and ignore the passed in parameter `c`.

src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java line 936:

> 934:                                             skin.paintSkin(g,
> 935:                                                 (type == JRadioButtonMenuItem.class) ? (x + 3 * OFFSET) : (x + 4 * OFFSET),
> 936:                                                 (icon.getIconHeight() <= 16) ? y + OFFSET : (y + icon.getIconHeight() / 2), state);

I wonder why painting the skin for radio button menu requires such a fix up whereas it's not required for `JCheckBoxMenuItem`.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/28889#discussion_r2658309916
PR Review Comment: https://git.openjdk.org/jdk/pull/28889#discussion_r2658290233
PR Review Comment: https://git.openjdk.org/jdk/pull/28889#discussion_r2658311962