-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8348760: RadioButton is not shown if JRadioButtonMenuItem is rendered with ImageIcon in WindowsLookAndFeel #23324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
… with ImageIcon in WindowsLookAndFeel
👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into |
@prsadhuk This change is no longer ready for integration - check the PR body for details. |
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
Outdated
Show resolved
Hide resolved
test/jdk/javax/swing/JMenuItem/TestImageIconWithJRadioButtonMenuItem.java
Outdated
Show resolved
Hide resolved
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
Outdated
Show resolved
Hide resolved
test/jdk/javax/swing/JMenuItem/TestImageIconWithJRadioButtonMenuItem.java
Outdated
Show resolved
Hide resolved
@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. |
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
Outdated
Show resolved
Hide resolved
test/jdk/javax/swing/JMenuItem/TestImageIconWithJRadioButtonMenuItem.java
Outdated
Show resolved
Hide resolved
@prsadhuk After image path correction, duke image icon is visible. |
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also for generating an ImageIcon
on the fly to avoid adding a binary file.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
Outdated
Show resolved
Hide resolved
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
Outdated
Show resolved
Hide resolved
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
Outdated
Show resolved
Hide resolved
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
Outdated
Show resolved
Hide resolved
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java
Outdated
Show resolved
Hide resolved
This does not look right at all: 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 |
Please check updated PR and share your testcase |
I saw your update. I wrote this comment before you removed rendering the unselected radio button.
Here's the latest version of modified The diff between yours and mine. |
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 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. |
I refined the example program and submitted a support case to Microsoft. First there are several elements to understand about menu items (1) The menu item can have a string as the "item" (dwTypeData)
As well as those you can specify the images to use for the check state
Some questions and some answers with Microsoft Q. Why is the highlighting background gone in Windows 11 ? A. That background was part of the “Reveal” lighting effect used by the old Fluent-Design menus in Windows 10. As of WinUI 2.6 (the style set shipped with Windows 11) the UI team removed that effect across context-menus and other pop-ups to achieve a flatter look that meets new contrast guidelines. So it’s an intentional theme change, not a GDI regression. Q. Why does the item image draw over the default radio button. How can this be anything except a bug ? A. The Win32 menu engine draws the default bullet/checkmark at X = 0 unless you reserve a check-mark column by supplying hbmpChecked (and optionally hbmpUnchecked). Q. Why isn't space automatically left for the default rendering of the checkmark ? Another bug ? A. For historical reasons the menu layout engine only allocates the check-mark column when it has a concrete size for that column, i.e. when hbmpChecked is non-NULL. If you ask for the default glyph while also providing an item icon, Windows assumes you are handling layout yourself. The docs don’t call this out explicitly (agreed, that would be helpful), but the behaviour is consistent and the same on every supported version of Windows, so it is not tracked as a GDI defect. So .. in summary, two things that have been a bug so long they are considered a feature are at the root of this and the visual "cue" we've relied on for in Windows 10 was just an accident of some theming that is now gone. So GDI really does make it hard to have all 3 of these. And maybe we should just not support it ? But if (as GDI programmers) we want an item string and bitmap, and expect feedback that an item is selected, we seem to be forced into needing to specify bitmaps for the checked and un-checked state. But then you lose the default rendering of the bullet that Windows provides ! There's some hoops we can jump through to get close
rbma.cpp illustrates this. This still isn't perfect - the bullet is too large. I'll leave it to others to figure out how we make use of this in the Swing rendering case. |
I checked rbma.cpp output where I think we can only support the 4th item in the list (which is this) This is because we have JRadioButtonMenuItem(String text, Icon icon, boolean selected) The current state of PR supports this 4th item. but current PR the text is shifted to left (when there is no imageicon ie 3rd item in list) and occupies icon space, as below This is because in most cases (and in SwingSet2) the radiomenu items are drawn without imageicon and there is no extra space between radio bullet and menu text so if we cater to text shifting to same vertical line, there will be difference in behavior w.r.t existing one and user will notice a shift in text to accomodate non-existing image icon) but it is fixable, |
Not sure I know what you mean by 4th item, but clearly we aren't going to add an API to supply checked/unchecked images.
What is "this 4th item" ? You mean the case of image + text from app, and WE display a radio button checked ? Only thing different is when there is no imageicon and menuitem is selected, then the text seems to be in same vertical line as the other items (in native rbma output)
If you mean windows always lines up the start of the text, that seems to be true.
it seems to me that if there's an image in ANY menu item, Windows reserves the space in all items, hence getting the text aligned.
You mean user will notice text is now aligned ? I think that's fine.
I hope (!) we are on the same page. Let me try and say it so we are ..
|
Yes, current PR does this..
Yes, current PR does this.
Yes, current PR does this.
Yes, icon image has its own space in current PR
Current PR doesnt have this..Text string is not aligned for item that doesn't have iconimage with item that does have iconimage. I will fix this. |
That gap should only be there if at least one menu item has an image icon. It shoudn't be there if none have an icon. |
That will require to find out if current menuItem is part of ButtonGroup and then reiterate each menuitems of the ButtonGroup and see if anyone has an imageicon..It does not seem possible to find out from current menuItem if it part of ButtonGroup as that info is not passed to each MenuItemUI (ButtonGroup info is only known to application/user) Also, assuming if we do find a way to get if this menuItem is part of a ButtonGroup, then iterating everytime to find out if any menu item has imageicon will affect performance. |
Why? This depends on two factors only:
If both are true, you add two columns for the mark and for the icon; in all other cases, the layout remains as it is now with one column where icons and marks are rendered. The above is done in Metal L&F (run the test case in the default L&F), why can't it be done in Windows L&F?
No, I don't think it's a good approach. Look at how it's done in Metal, ideally re-use the layout code. |
… 1 menuitem has imageicon
@prrace Fixed now in latest iteration SwingSet2 there's no gap for RBMI since there are no imageicons |
The current layout doesn't match the layout of menus in Windows. The check mark or the bullet has to be painted where it was always painted. The menu in Windows 10 looks broken now: This applies to the radio bullet JRadioButtonMenuItem3 which has no icon. In Windows 11, I expect the check mark and the bullet render at the same location as it was before the fix, but it's not the case. Below is a portion of the menu rendered with the current fix: The part on the image in an orange rectangle is a portion of the same test case running on JDK that has no fix.
|
We have to support two cases:
To reiterate Phil's suggestion,
In the two-column layout, the first column (which is always present) is reserved for check marks and bullets of |
The layout for Windows 11 is still wrong and suffers from the same problems that I raised yesterday. The entire popup menu looks the same way that I posted about an hour ago. The layout for Windows 10 is correct now. |
Your latest change has fixed the position of the icon for a regular
This still remain unresolved: the text of all menu items should be aligned. |
It is fixed for RBMIs as they are part of ButtonGroup (where selecting one will deselect other) so we can find out if any of the RBMIs in that group has imageicon by obtaining ButtonGroup object. |
Why is it important?
Why is it important? The rule to switching to two-column layout is as simple as that:
If there's at least one
This is absolutely possible, both Metal & Nimbus L&F use this menu layout. If none of |
As I mentioned accelerators column in my previous comment, I realised that I hadn't tested accelerators so far. Indeed, accelerators rendering is broken now: This proves that the layout isn't right. You're trying to fit both check mark / bullet and an icon into the same allocated space by moving text to the right without reserving additional width for the icon. At the same time, you move the check mark / bullet to the left. Such approach doesn't respect the default insets added on the left and right of a menu item. The popup menu has to become wider if both check marks / bullets and icons are rendered at the same time. |
@@ -143,6 +155,128 @@ public static RepaintManager getDelegateRepaintManager(Component | |||
return delegate; | |||
} | |||
|
|||
public static void applyInsets(Rectangle rect, Insets insets) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't a similar helper function already exists…
rect.width -= (insets.right + rect.x); | ||
rect.height -= (insets.bottom + rect.y); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rect.width -= (insets.right + rect.x); | |
rect.height -= (insets.bottom + rect.y); | |
rect.width -= (insets.right + insets.left); | |
rect.height -= (insets.bottom + insets.top); |
The formula doesn't look right. Why do you subtract rect.x
and rect.y
instead of insets.left
and insets.top
?
This would work correctly if both rect.x
and rect.y
are zero, but it would give wrong results in other cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the same calculation used in BasicMenuItemUI
which is now moved to SwingUtilities3
so that it can be called from Basic and WindowsMenuItemUI
to avoid code duplication....you can compare the contents before the fix.....in this specific case, rect.x
is inclusive of insets.left
..
Similarly other SwingUtilities3 changes are also as it was in BasicMenuItemUI, it was just moved verbatim from BasicMenuItemUI to it so that it can be called from both Basic and WindowsMenuItemUI class with no changes..nothing more and nothing less...probably it can be optimized but I wanted to keep the call and the content in each method in SwingUtilities3 same as it was in BasicMenuItemUI
private static Color disabledForeground; | ||
private static Color acceleratorSelectionForeground; | ||
private static Color acceleratorForeground; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these static? I'm pretty sure the colors can be menu item specific, although more commonly they would be L&F specific.
SwingUtilities3.setDisabledForeground(disabledForeground); | ||
SwingUtilities3.setAcceleratorSelectionForeground( | ||
acceleratorSelectionForeground); | ||
SwingUtilities3.setAcceleratorForeground(acceleratorForeground); | ||
SwingUtilities3.paintAccText(g, lh, lr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really weird way… Pass the colors explicitly as parameters to the SwingUtilities3.paintAccText
method.
private static Color disabledForeground; | ||
private static Color acceleratorSelectionForeground; | ||
private static Color acceleratorForeground; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these needed?
BasicMenuItemUI
has fields for all three colors: disabledForeground
, acceleratorSelectionForeground
, acceleratorForeground
. Why do you need to create three new static fields instead?
/** | ||
* Paint MenuItem. | ||
*/ | ||
protected void paintMenuItem(Graphics g, JComponent c, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | |
* Paint MenuItem. | |
*/ | |
protected void paintMenuItem(Graphics g, JComponent c, | |
@Override | |
protected void paintMenuItem(Graphics g, JComponent c, |
/** | ||
* Paint MenuItem. | ||
*/ | ||
protected void paintMenuItem(Graphics g, JComponent c, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | |
* Paint MenuItem. | |
*/ | |
protected void paintMenuItem(Graphics g, JComponent c, | |
@Override | |
protected void paintMenuItem(Graphics g, JComponent c, |
String acceleratorDelimiter = | ||
UIManager.getString("MenuItem.acceleratorDelimiter"); | ||
if (acceleratorDelimiter == null) { acceleratorDelimiter = "+"; } | ||
Font acceleratorFont = UIManager.getFont("MenuItem.acceleratorFont"); | ||
if (acceleratorFont == null) { | ||
acceleratorFont = UIManager.getFont("MenuItem.font"); | ||
} | ||
MenuItemLayoutHelper lh = new MenuItemLayoutHelper(mi, checkIcon, | ||
arrowIcon, viewRect, defaultTextIconGap, acceleratorDelimiter, | ||
mi.getComponentOrientation().isLeftToRight(), mi.getFont(), | ||
acceleratorFont, MenuItemLayoutHelper.useCheckAndArrow(menuItem), | ||
prefix); | ||
MenuItemLayoutHelper.LayoutResult lr = lh.layoutMenuItem(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can acceleratorDelimiter
, acceleratorFont
be fetched from any MenuItemUI
classes?
Do I understand correctly that the original menu layout that was already performed for a class gets thrown away and is being re-evaluated here?
if (!isWindows11OrLater) { | ||
skin.paintSkin(g, x, y, | ||
getIconWidth(), getIconHeight(), backgroundState); | ||
if (icon == null) { | ||
skin = xp.getSkin(c, part); | ||
skin.paintSkin(g, x + OFFSET, y + OFFSET, state); | ||
} | ||
} else { | ||
skin.paintSkin(g, x - 2 * OFFSET, y, | ||
getIconWidth(), getIconHeight(), backgroundState); | ||
skinWidth = getIconWidth(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To achieve a better layout, the icon size, specifically its width, should increase whenever we detect a situation where both check mark / bullet and associated icon need to be painted together so that VistaMenuItemCheckIconFactory
returns an instance of VistaMenuItemCheckIcon
or Windows11MenuItemCheckIcon
which reserves the space for both check mark / bullet and icon as well as a gap between them. This way, the exiting menu layout code would position the elements correctly.
} | ||
} | ||
|
||
public static void paintCheckIcon(Graphics g, MenuItemLayoutHelper lh, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest creating a new sun.swing.MenuItemRenderHelper
class similar to sun.swing.MenuItemLayoutHelper
to keep both layout and renderer closer together.
if (!isWindows11OrLater) { | ||
skin.paintSkin(g, x, y, | ||
getIconWidth(), getIconHeight(), backgroundState); | ||
if (icon == null) { | ||
skin = xp.getSkin(c, part); | ||
skin.paintSkin(g, x + OFFSET, y + OFFSET, state); | ||
} | ||
} else { | ||
skin.paintSkin(g, x, y, | ||
getIconWidth(), getIconHeight(), backgroundState); | ||
skinWidth = getIconWidth(); | ||
skin = xp.getSkin(c, part); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I expect to see here is that the skin is always painted at (x + OFFSET, y + OFFSET
)
skin.paintSkin(g, x + OFFSET, y + OFFSET, state);
in both Windows 10 and 11.
The check mark and bullet have to remain on their own designated place.
if (!isWindows11OrLater) { | ||
icon.paintIcon(c, g, x + OFFSET, y + OFFSET); | ||
} else { | ||
icon.paintIcon(c, g, x + icon.getIconWidth(), | ||
y + OFFSET); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an icon is present, it gets painted instead of check mark or bullet in Windows 10.
In Windows 11 the icon gets painted on
- on the same location in regular case when the current popup menu contains no check or radio menu items with an icon;
- on its own column with has proper large margins when there's at least one check or radio menu item with an icon; the size of the entire popup has to increase to allocate space for the icon column.
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
Issue
Reviewers
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