jdk 8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel

trafficstars

When JRadioButtonMenuItem is called with imageIcon, then only imageIcon is shown without radiobutton in WIndowsLookAndFeel as there was no provision of drawing the radiobutton alongside icon. If icon is not there, the radiobutton is drawn. Added provision of drawing the radiobutton windows Skin even when imageIcon is present.

Progress

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

Issue

JDK-8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel (Bug - P3)

Reviewers

Abhishek Kumar (@kumarabhi006 - Reviewer) 🔄 Re-review required (review applies to ba7ecae3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23324

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

Using diff file

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

Using Webrev

Link to Webrev Comment

Jan 28 '25 03:01 prsadhuk

:wave: 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.

Jan 28 '25 03:01 bridgekeeper[bot]

@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:

8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel

Reviewed-by: prr, kizune, abhiscxk

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 48 new commits pushed to the master branch:

e606278fc8929fe563dd50a1c3f332747e210276: 8358598: PhaseIterGVN::PhaseIterGVN(PhaseGVN* gvn) doesn't use its parameter
83953c458eb65b2af184340dd460325f2b56e5b9: 8364822: Comment cleanup, stale references to closeDescriptors and UNIXProcess.c
bc3d86564042208cee5119abe11905e747a5ef4c: 8364128: Improve gathering of cpu feature names using stringStream
... and 45 more: https://git.openjdk.org/jdk/compare/57553ca1dbc63e329116bc11764816a4c5ccb297...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.

Jan 28 '25 03:01 openjdk[bot]

@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.

Jan 28 '25 03:01 openjdk[bot]

Webrevs

Jan 28 '25 03:01 mlbridge[bot]

@prsadhuk Unable to see the duke image along with RadioButtonMenuItem. Is it correct behaviour?

RadioButtonMenuItem

Jan 30 '25 08:01 kumarabhi006

@prsadhuk Unable to see the duke image along with RadioButtonMenuItem. Is it correct behaviour?

did you download the duke file from the PR?

Jan 30 '25 08:01 prsadhuk

@prsadhuk Unable to see the duke image along with RadioButtonMenuItem. Is it correct behaviour?

did you download the duke file from the PR?

Yeah, it's inside "javax\swing\JMenuItem" folder.

Jan 30 '25 08:01 kumarabhi006

@prsadhuk After image path correction, duke image icon is visible.

Jan 30 '25 08:01 kumarabhi006

This does not look right at all:

The screenshot of the newly added test case TestImageIconWithJRadioButtonMenuItem.java on Windows 10: it renders an unselected radio button control to the left of menu item which has an icon in addition to text

You shouldn't render an unselected radio button in menu.

(Yes, I modified your test case to display the menu items in a popup menu as they're supposed to be.)

The position of the image also seems off. On Windows 10, the selected state has a background which you can see on the screenshot. Without the fix, the duke is centered in this selected background; but with the fix, it's off.

If we add support for painting both an ImageIcon and the bullet / check-mark, the layout of the menu item has to change.

Jan 30 '25 12:01 aivanov-jdk

You shouldn't render an unselected radio button in menu.

Please check updated PR and share your testcase

Jan 30 '25 13:01 prsadhuk

You shouldn't render an unselected radio button in menu.

Please check updated PR…

I saw your update. I wrote this comment before you removed rendering the unselected radio button.

…share your testcase

Here's the latest version of modified TestImageIconWithJRadioButtonMenuItem.java.

The diff between yours and mine.

Jan 30 '25 13:01 aivanov-jdk

Based on the discussion above in relation to JDK-6432667 where the current behaviour/rendering was implemented, this quirk may be a feature of Windows L&F rather than a bug.

Windows 10 - red square item selected

Windows 10 has a highlight around the selected icon. Yet there's no such highlight in Windows 11, therefore it's hard to distinguish whether a menu item has a selected state of not.

Jan 30 '25 14:01 aivanov-jdk

I believe this is likely how layout is calculated.

I just wanted to know "why 3 is chosen as a magic number ?". Actually the spacing with "4 * OFFSET" looks better between radio, icon and text of menu item (on win 11).

Feb 04 '25 12:02 kumarabhi006

I believe this is likely how layout is calculated.

I just wanted to know "why 3 is chosen as a magic number ?". Actually the spacing with "4 * OFFSET" looks better between radio, icon and text of menu item (on win 11).

I guess this is now taken care of in latest PR..

Feb 05 '25 02:02 prsadhuk

@prsadhuk I tested on win11 and I think this should be OK. Although the gap is not equal between RadioButton icon, imageIcon and text but as you said above text location as it is done in WindowsMenuItemUI#paintText which does not have any knowledge of bullet/icon presence, this may not be an issue.

After changing icon.paintIcon(c, g, x - OFFSET + ((skinWidth != -1) ? skinWidth : 16), y + OFFSET); to icon.paintIcon(c, g, x - OFFSET + ((skinWidth != -1) ? skinWidth - 1 : 15), y + OFFSET); looks better (my personal opinion)

Same issue exist for JCheckBoxMenuItem as well, it's better to extend the test for JCheckBoxMenuItem as well. The components can be re-arranged in a grid panel to give better visual alignment (vertically aligned).

Feb 05 '25 14:02 kumarabhi006

@prsadhuk I tested on win11 and I think this should be OK. Although the gap is not equal between RadioButton icon, imageIcon and text but as you said above text location as it is done in WindowsMenuItemUI#paintText which does not have any knowledge of bullet/icon presence, this may not be an issue.

After changing icon.paintIcon(c, g, x - OFFSET + ((skinWidth != -1) ? skinWidth : 16), y + OFFSET); to icon.paintIcon(c, g, x - OFFSET + ((skinWidth != -1) ? skinWidth - 1 : 15), y + OFFSET); looks better (my personal opinion)

Same issue exist for JCheckBoxMenuItem as well, it's better to extend the test for JCheckBoxMenuItem as well. The components can be re-arranged in a grid panel to give better visual alignment (vertically aligned).

It may look better with skinWidth -1 and 16-1 but the icon may overlap with bullet/checkmark skin by 1 pixel in that case, so for better consistency, I have kept it adrift of skinWidth and 16 is because normal icon is of 16x16 size, so in case there is no skin, then also 16 width is maintained

I will work on the testcase to extend for JCheckBoxMenuItem..

Feb 06 '25 05:02 prsadhuk

I tested it on Windows 11 and 10.

Windows 11
Windows-11-menu-items-2025-02-06
It looks reasonably well…

But it doesn't look like the menu in Windows 11 File Explorer, does it?
Windows 11 File Explorer View with icons (100%)
Windows L&F should look like native apps do, and we're not there yet with the menu rendering.

Windows 10
Windows-10-menu-items-2025-02-06
It looks bad because neither icon is where it should be.

Without the fix, it looks good:
Windows-10-menu-items-no-fix-2025-02-06
And one can see if an item is selected or not. Yet they changed the skin in Windows 11, now it's impossible to distinguish them.

The updated test is available at commit 7aa4de1.

Feb 06 '25 17:02 aivanov-jdk

On that note, Swing's menu rendering doesn't blend with the way Windows 11 renders menus:

Menu has rounded corners,
The highlight color is just slightly darked than the menu background,
The highlight rectangle also has rounded.

A highlighted menu item Windows-11-highlighted-menu-item-in-Explorer

This is out of scope of this issue, I just wanted to bring it up. I guess I should submit a bug against this, it would be more relevant as support for Windows 10 is phased out. Swing uses the theming API to paint menus, yet it differs now.

Feb 06 '25 18:02 aivanov-jdk

I tested it on Windows 11 and 10.

Windows 11 It looks reasonably well…

But it doesn't look like the menu in Windows 11 File Explorer, does it? Windows L&F should look like native apps do, and we're not there yet with the menu rendering.

Windows 10 It looks bad because neither icon is where it should be.

Without the fix, it looks good: And one can see if an item is selected or not. Yet they changed the skin in Windows 11, now it's impossible to distinguish them.

I am not sure if we can satisfy both windows 10 and windows 11 simultaneously as both design is different for this logic... Without the fix, it looks good as Windows 10 as it doesn't support separate bullet/checkmark skin whereas windows 11 does...Also, its difficult to test for me as I do not have windows 10 so any change for windows 11 whether it works for windows10 is difficult to make out, also considering the fact mainline jdk is not supporting windows10 so not sure why we should introduce additional logic to satisfy windows10

Feb 07 '25 03:02 prsadhuk

skin =  xp.getSkin(c, backgroundPart);
                            skin.paintSkin(g, x - 2 * OFFSET, y,
                                getIconWidth(), getIconHeight(), backgroundState);
                            skinWidth = getIconWidth();

@aivanov-jdk Can you please check if we use getIconWidth() instead of skin.getWidth, does the icon stay clear of the bullet/checkmark background skin in windows 10?

Feb 07 '25 03:02 prsadhuk

It seems in windows 11, the menuitem text location is dependant on presence of menuitem icon. As per this screenshot, the bullet/checkmark seems to be in its fixed location but text is shifted depending on menuitem icon.

Modified the PR to look the same.

Feb 07 '25 05:02 prsadhuk

I am not sure if we can satisfy both windows 10 and windows 11 simultaneously as both design is different for this logic...

We absolutely can… I'm not proposing using different rendering, instead I'm for following Windows 11 rendering, but it requires updating menu layout code.

Without the fix, it looks good as Windows 10 as it doesn't support separate bullet/checkmark skin whereas windows 11 does...

I'm unsure Windows 11 does support it natively, likely File Explorer implements custom drawing code.

I'm pretty sure the situation is the same with Windows 10, yet none of use was able to find an app which displays both icons and bullets/check-marks.

The commonly available feature in Windows is to display an icon, which you can see in window menu activated by Alt+Space or right-clicking the window title bar: Restore, Minimise, Maximise, and Close items display an icon — this icon is rendered centred exactly at the position where radio bullet or check mark are.

I may be able to play around in a Win32 app with it.

Also, its difficult to test for me as I do not have windows 10 so any change for windows 11 whether it works for windows10 is difficult to make out, also considering the fact mainline jdk is not supporting windows10 so not sure why we should introduce additional logic to satisfy windows10

My point here is that the current approach doesn't look similar to how File Explorer renders menu with bullets and icons. If you do it, this style would work just fine for Windows 10 too: the bullet will have its own place, the icon will have its own place.

Even though Windows 10 is not going to be supported by JDK 25, it doesn't make sense to break anything that works there. After all, we had to restore theming support for Windows 7 support for which ended long-long time ago.

Feb 07 '25 11:02 aivanov-jdk

It seems in windows 11, the menuitem text location is dependant on presence of menuitem icon. As per this screenshot, the bullet/checkmark seems to be in its fixed location but text is shifted depending on menuitem icon.

That's what I said:

If we add support for painting both an ImageIcon and the bullet / check-mark, the layout of the menu item has to change.

Modified the PR to look the same.

Thank you. I'll test it.

However, I think that all text in a popup menu should move to the right… that is all text in menu items should remain aligned if there bullets/check-marks and icons in a particular popup menu. That is the text for the third menu items for radio button and for check mark should also move.

Whether all text is aligned could be controlled by a property:

We may introduce a property that controls this behaviour: whether the text aligns in all the menu items or not.

There's a sample in the linked message.

Feb 07 '25 11:02 aivanov-jdk

This is how it looks on Windows 10:
latest-fix-2025-02-07

It's close, yet the spacing is wrong.

The bullet / check-mark should be where they were before the fix. There was 8-pixel margin to the left of selected bullet background before the fix, now there are only 2 pixels.

The margin between the bullet background and the text was again 8 pixels, I think we should maintain the same margin between the bullet and the icon as well as between the icon and the menu text.

Feb 07 '25 19:02 aivanov-jdk

This is how it looks on Windows 10:

It's close, yet the spacing is wrong.

The bullet / check-mark should be where they were before the fix. There was 8-pixel margin to the left of selected bullet background before the fix, now there are only 2 pixels.

The margin between the bullet background and the text was again 8 pixels, I think we should maintain the same margin between the bullet and the icon as well as between the icon and the menu text.

Does bullet/checkmark use to appear before the fix in windows 10? I thought you told the icon was getting highlighted/dehighlighted depending on item is selected or not

I am not sure if we can make 8 pixel margin as it will cause MenuItem text or bullet to be outgrow popupmenu width ( I faced this issue while experimenting with spacing), which I am not sure if and where we can increase.

Feb 10 '25 05:02 prsadhuk

It's close, yet the spacing is wrong.

The bullet / check-mark should be where they were before the fix. There was 8-pixel margin to the left of selected bullet background before the fix, now there are only 2 pixels.

The margin between the bullet background and the text was again 8 pixels, I think we should maintain the same margin between the bullet and the icon as well as between the icon and the menu text.

Does bullet/checkmark use to appear before the fix in windows 10? I thought you told the icon was getting highlighted/dehighlighted depending on item is selected or not

That's right! The ImageIcon was painted instead of bullet or check-mark.

Now that you moved the ImageIcon to the right, the bullet or check-mark has to be painted where the bullets and check-marks are painted when there's no custom icon.

That's what I meant — the previous location of the bullet / check-mark should be preserved.

I am not sure if we can make 8 pixel margin as it will cause MenuItem text or bullet to be outgrow popupmenu width ( I faced this issue while experimenting with spacing), which I am not sure if and where we can increase.

This is why I say that the layout and therefore the width of the menu item has to be updated.

Previously, the custom icon was painted over, or instead of, the bullet or check-mark.

This PR proposes painting the custom icon separately, which means the width of the menu item has increase to accommodate the margins around the custom icon as well as the icon itself.

Feb 10 '25 16:02 aivanov-jdk

@prrace As discussed, reverted to no-CSR needed fix...please review..

Feb 28 '25 03:02 prsadhuk

There is a lot to absorb here. I don't want to get into line by line code discussion, we need to have the big picture right first.

(1) This affects all releases, right ? The evaluation in the bug report is non-existent. I expect to see a full description of how it came about (history) what the consequences are, what the options are etc. Without that the PR is in a vacuum.

(2) My rough understanding is that this was "observed" because of a change in Windows 11 vs earlier Windows versions with the highlighting of the radio button .. but the fix has some how become focused on the ImageIcon ? If I don't have this right, give me a clear and complete explanation of the issue (ie part of the missing evaluation)

Surely that isn't a Windows 10 vs Windows 11 issue ?? Unless it is an accidental by-product of some layout problem ? I am seeing code in the pre-fix side of the diff that does paint an icon .. so I am not sure I understand. If the same control in native windows doesn't allow such an icon, then we are within our rights to ignore it. There are a bunch of settings, such as b/g color etc that various L&Fs ignore if they aren't appropriate. Search the javax.swing javadoc for the word "ignore" and you'll find hundreds of matches.

I can't actually tell from the PR description what the inentention is "Added provision of drawing the radiobutton windows Skin even when imageIcon is present."

Does that mean it continues to ignore the imageicon - ie not render it ? But as I wrote there is something that looks to be painting an icon. Maybe we are talking about the "icon" that is actually the rendering of the radio button - not an application supplied imageicon ? I'd like 100% clarity here.

(3) Any fix requiring a CSR would be a problem - at least if it is SE spec significant, because it seems like this fix needs to be backported all the way to 8u. If there is a better fix that requires a CSR for the future, we can do that as a follow-on for JDK 25 only, but we should start with what can be backported, and do the better fix as a follow-on.

I'll deviate slightly from my "I won't get in to lines for code here", and say that FWIW the current fix isn't what I'd like to see. Putting a check for Windows L&F in a BasicUI class ?? We had a similar issue with GTK some months ago and it was something that we managed to avoid. Isn't there a Windows L&F sub-class we can do this in ??

(4) We are not any time soon de-supporting Windows 10. Windows 10 is still supported by Microsoft the day JDK 25 ships, even for regular users. We need to make this work properly on both Windows 10 and Windows 11. NB in both cases, there are unsupported older updates and still supported updates .. like 22H2 for Windows 10 is the one to check. If this means the code needs to work out if it is on Windows 11 vs 10, so be it.

Mar 06 '25 18:03 prrace

I can quickly answer to this part:

Surely that isn't a Windows 10 vs Windows 11 issue ?? Unless it is an accidental by-product of some layout problem ?

It is a Windows 10 vs Windows 11 issue.

Currently, an ImageIcon associated with a radio- or check menu item is painted instead of the bullet or check-mark. The selected state of the corresponding menu item is still apparent since the background of the icon is darker.

Windows-10-menu-items-no-fix-2025-02-06

This is a screenshot from the discussion above. Here the purplish background around icon indicates the selected state.

In Windows 11, there's no way to distinguish whether a radio- or check menu item is selected or not because the selection background is gone in Windows 11 style.

Prasanta found a menu in Windows File Explorer, the View button on the toolbar, which displays both the radio bullet and an icon to the left of menu text.

A highlighted menu item Windows-11-highlighted-menu-item-in-Explorer

No one has found a similar usage in Windows 10. Other apps, Oracle VirtualBox and Paint.NET, which draw icons in menu items add a small check-mark to the icon to indicate the menu item is selected.

As far as I'm aware, Win32 menus don't allow displaying both an icon and a bullet or check-mark; apps have to custom-draw such menu items.

Mar 06 '25 19:03 aivanov-jdk

(1) I have added the evaluation in the JBS

(2) Yes, as pointed out already this is a Windows 10 vs Windows 11 issue...WIndows 10 imageicon is drawn on top of bullet/checkmark whereas in Windows 11 it is drawn alongside and I have fixed Windows 11 to look like FileExplorer->View

WIndows 11-native 410771219-f26a71f4-d6e2-4848-9c48-27c3f40f763e

Windows11-withfix 410771901-7d636add-f336-4db3-96cd-d8fe06737b3a

Putting a check for Windows L&F in a BasicUI class ?? Isn't there a Windows L&F sub-class we can do this in ??

(3) The only way we could do that in Windows L&F class would require a CSR and it is done in this commit but I reverted back because of CSR requirement

(4) The current fix ensures Windows 10 will continue to work and also fixes Windows 11

Mar 07 '25 02:03 prsadhuk

jdk jdk copied to clipboard

8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel

Progress

Issue

Reviewers

Reviewing

Webrevs

jdk
jdk copied to clipboard