8373650: Test "javax/swing/JMenuItem/6458123/ManualBug6458123.java" fails because the check icons are not aligned properly as expected #28889

prsadhuk · 2025-12-18T07:34:36Z

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

After fix

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8373650: Test "javax/swing/JMenuItem/6458123/ManualBug6458123.java" fails because the check icons are not aligned properly as expected (Bug - P4)

Reviewers

Damon Nguyen (@DamonGuy - Reviewer) Review applies to c4bda416
Tejesh R (@TejeshR13 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28889

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28889.diff

Using Webrev

Link to Webrev Comment

…ls because the check icons are not aligned properly as expected

bridgekeeper · 2025-12-18T07:36:14Z

👋 Welcome back psadhukhan! 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 · 2025-12-18T07:36:55Z

@prsadhuk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: tr, dnguyen

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 320 new commits pushed to the master branch:

303de9a: 8370666: VectorAPI: Add clear comments for vector relative code in c2
496af3c: 8375093: Convert GlobalCounter to use Atomic
f2d5290: 8367319: Add os interfaces to get machine and container values separately
... and 317 more: https://git.openjdk.org/jdk/compare/78c2d57259ad829a2cfc1370efbb2a5913df4661...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-12-18T07:37:47Z

@prsadhuk The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

prsadhuk · 2025-12-18T07:44:41Z

/touch

openjdk · 2025-12-18T07:46:41Z

@prsadhuk The pull request is being re-evaluated and the inactivity timeout has been reset.

mlbridge · 2025-12-18T07:50:29Z

Webrevs

mrserb · 2025-12-22T18:04:00Z

Check/radiobutton icon are not aligned properly in RTL.

What is the proper alignment in this context? The “After fix” image shows that the icons still jump left and right.

DamonGuy

Any reason why the "After fix" image also has the "Radio button" dot misaligned still?

prsadhuk · 2026-01-02T09:54:11Z

Any reason why the "After fix" image also has the "Radio button" dot misaligned still?

Fixed

aivanov-jdk · 2026-01-02T19:34:26Z

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

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


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

jdk/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java

Line 881 in 2daf12e

assert menuItem == null || c == menuItem;

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.

aivanov-jdk · 2026-01-02T19:48:55Z

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

+                                    if (icon.getIconWidth() <= 8) {
+                                        skin.paintSkin(g,
+                                                x + OFFSET,
+                                                (icon.getIconHeight() <= 16) ? y + OFFSET : (y + icon.getIconHeight() / 2), state);
+                                    } 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?

As I see, MenuItemLayout handler's x position calculation for RTL depends on icon rect width

jdk/src/java.desktop/share/classes/sun/swing/MenuItemLayoutHelper.java

Lines 737 to 741 in 6eaabed

private void calcXPositionsRTL(int startXPos, int leadingGap,

int gap, Rectangle... rects) {

int curXPos = startXPos - leadingGap;

for (Rectangle rect : rects) {

rect.x = curXPos - rect.width;

but LTR doesn't depend on rect width

jdk/src/java.desktop/share/classes/sun/swing/MenuItemLayoutHelper.java

Lines 726 to 730 in 6eaabed

private void calcXPositionsLTR(int startXPos, int leadingGap,

int gap, Rectangle... rects) {

int curXPos = startXPos + leadingGap;

for (Rectangle rect : rects) {

rect.x = curXPos;

Okay, but why do you have tweak the layout produced by MenuItemLayoutHelper?

Do Metal and Nimbus have to tweak the layout too?

Is it because Windows icons used to be special and combined the icon and check mark / bullet mark, or rather replaced the check mark / bullet mark with the icon? Should we get rid of that special treatment?

I'd like to have answers to these questions.

To be clear, I'm not opposing this approach, it seems to work, yet it seems superfluous, the code kind of repeats calculations. Both Metal and Nimbus L&F have similar column layouts where a separate column is allocated for marks and icons, Windows L&F should re-use that code.

Is it because Windows icons used to be special and combined the icon and check mark / bullet mark, or rather replaced the check mark / bullet mark with the icon? Should we get rid of that special treatment?

I am reusing the VistaMenuItemCheckIcon to render the RadioButtonMenuItem and CheckBoxMenuItem icons and getting rid of the working/treatment made in that class (and rewriting) might result in more regressions which I am avoiding..
There are many factors apart from normal that is affecting the icon position like menuItem.setHorizontalTextPosition(LEADING/TRAILING/LEFT/RIGHT etc), menuItem.setHorizontalAlignment(RIGHT/CENTER etc) which can cause issues like what we are seeing in #28210 which atleast is working now for Windows L&F without any rework.
Metal L&F seems to hardcode the icon position to (0,0) or (2,2)

jdk/src/java.desktop/share/classes/javax/swing/plaf/metal/MetalIconFactory.java

Lines 2268 to 2302 in e4474ad

private static class RadioButtonMenuItemIcon implements Icon, UIResource, Serializable

{

public void paintOceanIcon(Component c, Graphics g, int x, int y) {

ButtonModel model = ((JMenuItem)c).getModel();

boolean isSelected = model.isSelected();

boolean isEnabled = model.isEnabled();

boolean isPressed = model.isPressed();

boolean isArmed = model.isArmed();

g.translate( x, y );

if (isEnabled) {

MetalUtils.drawGradient(c, g, "RadioButtonMenuItem.gradient",

1, 1, 7, 7, true);

if (isPressed || isArmed) {

g.setColor(MetalLookAndFeel.getPrimaryControl());

}

else {

g.setColor(MetalLookAndFeel.getControlHighlight());

}

g.drawArc(-1, -1, 10, 10, 245, 140);

if (isPressed || isArmed) {

g.setColor(MetalLookAndFeel.getControlInfo());

}

else {

g.setColor(MetalLookAndFeel.getControlDarkShadow());

}

}

else {

g.setColor( MetalLookAndFeel.getMenuDisabledForeground() );

}

g.drawOval(0, 0, 8, 8);

whereas I am reusing the methodology in VistaMenuItemIcon using OFFSET and it seems to work for cases seen till now..

aivanov-jdk · 2026-01-02T19:50:31Z

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

+                                            skin.paintSkin(g,
+                                                (type == JRadioButtonMenuItem.class) ? (x + 4 * OFFSET) : (x + 3 * OFFSET),
+                                                (icon.getIconHeight() <= 16) ? y + OFFSET : (y + icon.getIconHeight() / 2), state);
+                                        }
+                                    } else {
+                                        if ((c instanceof JMenuItem mi) && (mi.getText().isEmpty() || mi.getAccelerator() != null)) {
+                                            skin.paintSkin(g,
+                                                (type == JRadioButtonMenuItem.class) ? (x + 3 * OFFSET) : (x + 4 * OFFSET),
+                                                (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.

it is taken care via ternary operator, width/spread of check mark is usually more than radio bullet so starting point of checkmark is considered a little ahead compared to radio bullet

I see that it's taken care by the ternary operator. Why is it needed, though?

The skin for both check mark and radio bullet have the same size, and the mark is positioned accordingly inside the rectangle. This works well for the LTR layout, both marks are rendered at the same location and appear aligned in the end. Why does RTL layout treat these marks differently?

As told and code shown earlier, the RTL oversees overall rectangle width using all text rect, image icon rect, icon rect etc to calculate icon x position unlike LTR and it seemed a bit off for checkmark as compared to bullet by 1 or 2 pixel or so I catered to it by this logic

DamonGuy

Although the if/else branches look strange, it works. I don't see a clear way to clean up and condense the conditionals so lgtm unless someone else has an idea.

DamonGuy · 2026-01-15T00:40:06Z

Also I guess this Copyright year needs to be updated to 2026 now as well.

aivanov-jdk · 2026-01-15T12:39:21Z

Although the if/else branches look strange, it works. I don't see a clear way to clean up and condense the conditionals so lgtm unless someone else has an idea.

I haven't looked at the updated code yet.

I wonder why the logic in MenuItemLayoutHelper.java isn't enough, why adjustments are needed.

As I posted in my previous comment, I'm not against this approach as long as it works and fixes the layout, yet I'd like to clean up the code if possible… in a follow-up issue.

TejeshR13

Should we update the bugid in test too ?

TejeshR13 · 2026-01-19T08:52:33Z

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

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


Can we optimize the code here, since mostly x is changing here based on conditions. It'll help in readability and review too.

No, y is also changing depending on icon height...
As of now, any more optimization is not possible, it can be done as followup if it is needed..

Line 926, 930, 936 and 940 looks same to me. I'm referring to these lines.

ok.. optimized..

TejeshR13 · 2026-01-20T04:31:10Z

Should we update the bugid in test too ?

@prsadhuk ?

prsadhuk · 2026-01-20T04:43:29Z

Should we update the bugid in test too ?

@prsadhuk ?

Its in closed...it shouldn't affect this open review but anyway raised a MR

TejeshR13

LGTM

prsadhuk · 2026-01-20T07:10:01Z

/integrate

openjdk · 2026-01-20T07:10:47Z

Going to push as commit e45f565.
Since your change was applied there have been 321 commits pushed to the master branch:

ca6925e: 8370112: Remove VM_Version::supports_fast_class_init_checks() in platform-specific code
303de9a: 8370666: VectorAPI: Add clear comments for vector relative code in c2
496af3c: 8375093: Convert GlobalCounter to use Atomic
... and 318 more: https://git.openjdk.org/jdk/compare/78c2d57259ad829a2cfc1370efbb2a5913df4661...master

Your commit was automatically rebased without conflicts.

openjdk · 2026-01-20T07:10:56Z

@prsadhuk Pushed as commit e45f565.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8373650: Test javax/swing/JMenuItem/6458123/ManualBug6458123.java fai…

f0b9927

…ls because the check icons are not aligned properly as expected

openjdk bot added the client [email protected] label Dec 18, 2025

openjdk bot added the rfr Pull request is ready for review label Dec 18, 2025

DamonGuy reviewed Dec 24, 2025

View reviewed changes

Alignment fix

bb174e9

aivanov-jdk reviewed Jan 2, 2026

View reviewed changes

Use menuItem var

c4bda41

DamonGuy approved these changes Jan 15, 2026

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 16, 2026

copyright year updated

7a0683a

openjdk bot removed the ready Pull request is ready to be integrated label Jan 16, 2026

TejeshR13 reviewed Jan 19, 2026

View reviewed changes

Review comment

d0d8fdc

TejeshR13 approved these changes Jan 20, 2026

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 20, 2026

openjdk bot added the integrated Pull request has been integrated label Jan 20, 2026

openjdk bot closed this Jan 20, 2026

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 20, 2026

prsadhuk deleted the JDK-8373650 branch January 20, 2026 07:11

	private void calcXPositionsRTL(int startXPos, int leadingGap,
	int gap, Rectangle... rects) {
	int curXPos = startXPos - leadingGap;
	for (Rectangle rect : rects) {
	rect.x = curXPos - rect.width;

	private void calcXPositionsLTR(int startXPos, int leadingGap,
	int gap, Rectangle... rects) {
	int curXPos = startXPos + leadingGap;
	for (Rectangle rect : rects) {
	rect.x = curXPos;

	private static class RadioButtonMenuItemIcon implements Icon, UIResource, Serializable
	{
	public void paintOceanIcon(Component c, Graphics g, int x, int y) {
	ButtonModel model = ((JMenuItem)c).getModel();
	boolean isSelected = model.isSelected();
	boolean isEnabled = model.isEnabled();
	boolean isPressed = model.isPressed();
	boolean isArmed = model.isArmed();

	g.translate( x, y );

	if (isEnabled) {
	MetalUtils.drawGradient(c, g, "RadioButtonMenuItem.gradient",
	1, 1, 7, 7, true);
	if (isPressed \|\| isArmed) {
	g.setColor(MetalLookAndFeel.getPrimaryControl());
	}
	else {
	g.setColor(MetalLookAndFeel.getControlHighlight());
	}

	g.drawArc(-1, -1, 10, 10, 245, 140);

	if (isPressed \|\| isArmed) {
	g.setColor(MetalLookAndFeel.getControlInfo());
	}
	else {
	g.setColor(MetalLookAndFeel.getControlDarkShadow());
	}
	}
	else {
	g.setColor( MetalLookAndFeel.getMenuDisabledForeground() );
	}

	g.drawOval(0, 0, 8, 8);

8373650: Test "javax/swing/JMenuItem/6458123/ManualBug6458123.java" fails because the check icons are not aligned properly as expected #28889

8373650: Test "javax/swing/JMenuItem/6458123/ManualBug6458123.java" fails because the check icons are not aligned properly as expected #28889

Uh oh!

Conversation

prsadhuk commented Dec 18, 2025 • 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 Dec 18, 2025

Uh oh!

openjdk bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prsadhuk commented Dec 18, 2025

Uh oh!

openjdk bot commented Dec 18, 2025

Uh oh!

mlbridge bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

mrserb commented Dec 22, 2025

Uh oh!

DamonGuy left a comment

Choose a reason for hiding this comment

Uh oh!

prsadhuk commented Jan 2, 2026

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!

prsadhuk Jan 16, 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!

DamonGuy left a comment

Choose a reason for hiding this comment

Uh oh!

DamonGuy commented Jan 15, 2026

Uh oh!

aivanov-jdk commented Jan 15, 2026

Uh oh!

TejeshR13 left a comment

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!

TejeshR13 commented Jan 20, 2026

Uh oh!

prsadhuk commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TejeshR13 left a comment

prsadhuk commented Dec 18, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Dec 18, 2025 •

edited

Loading

openjdk bot commented Dec 18, 2025 •

edited

Loading

mlbridge bot commented Dec 18, 2025 •

edited

Loading

prsadhuk Jan 16, 2026 •

edited

Loading

prsadhuk commented Jan 20, 2026 •

edited

Loading