-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
7093691: Nimbus LAF: disabled JComboBox using renderer has bad font color #12390
7093691: Nimbus LAF: disabled JComboBox using renderer has bad font color #12390
Conversation
👋 Welcome back dnguyen! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
||
hasNimbus = false; | ||
|
||
for (LookAndFeelInfo info : UIManager.getInstalledLookAndFeels()) { |
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 seems the intention to run the test is in Nimbus LAF only. I think you can directly set the Nimbus LAF using UIManager.setLookAndFeel("javax.swing.plaf.nimbus.NimbusLookAndFeel");
and can avoid the for-loop, condition check
and hasNimbus
variable.
To print the error message, LAF can be set within try-catch block.
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.
Thanks for the suggestion. I made the change. I initially had this loop to test locally for all LAF's, but ended up using this test for Nimbus only as originally designed.
@@ -0,0 +1,138 @@ | |||
/* | |||
* Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. |
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.
Copyright year can be changed to 2023.
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.
Updated
"status with the button on the right of the frame."; | ||
|
||
private static JFrame frame; | ||
private static boolean hasNimbus; |
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.
hasNimbus
variable is unused now, can be removed.
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.
Thanks, updated
try { | ||
UIManager.setLookAndFeel("javax.swing.plaf.nimbus.NimbusLookAndFeel"); | ||
} | ||
catch (Exception e) { |
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.
Just a suggestion, try-catch block can be like
try {
...
} catch {
...
}
@prsadhuk could you review this as well since I got your input about this issue previously? Initially, I thought it was the default value of the disabled combobox, but none of the skin.laf UI states/values affected the displayed text color. So, I assumed it is a state issue as you suggested. I checked the enabled state of each L&F, and the combobox's state is correct, but I noticed that the renderer in Synth has an additional condition that sets the renderer to disabled/enabled to match the combobox state. In testing, setting the renderer to disabled produced the correct behavior, so I made a listener as a workaround to access the combobox's renderer for the case with a DefaultListCellRenderer. With the test I created, this approach creates the correct behavior, so I implemented the fix the cleanest way possible. |
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.
Tested the fix on SwingSet2 too and the fix works as expected.
Copyright year needs to be updated for SynthComboBoxUI , DefaultListCellRenderer too.
jdk/src/java.desktop/share/classes/javax/swing/plaf/synth/SynthComboBoxUI.java Lines 552 to 558 in 98433a2
The present code SynthComboBoxRenderer does enable/disable the listcell renderer based on comboBox status, why it is not working? why do we need to use a listener for DefaultListCellRenderer? Also, I think it will be better to make the test automated..something like draw comboBox content into BufferedImage and pixel compare if it is same for disabled/enabled and fail if they are same and |
The setEnabled line you mentioned in SynthComboBoxUI applies to SynthComboBoxRenderer only. This issue only occurs when DefaultListCellRenderer is used for a disabled Nimbus ComboBox instead of the default renderer for Nimbus (SynthComboBoxRenderer). So this code is never reached, but is necessary to update the enabled state. IInitially, I wanted to check for this state in DefaultListCellRenderer, but there's no way to get a reference to the ComboBox from there. So, instead I used a listener and did this approach, which looks better since the ComboBox behavior stays in SynthComboBoxUI. I will work on making the test automated instead if preferred. |
I created an automated test, but for some L&F's (such as Nimbus), an enabled vs disabled ComboBox differs in more than just the text color. The background, border, and button color may change as well. So, even if I shave off the dimensions of the border and button, it seems difficult to detect just the text for comparison. Before the fix, the issue was that the ComboBox as a whole worked correctly for disabled ComboBoxes with a DLCR, but the text was black instead of grey. This was the only difference. A manual test might still be the best approach since more aspects than just the test color can be checked. So, I think it's best to not automate this test. |
|
Then can we not compare the comboBox without DLCR with comboBox with DLCR when both are disabled...both should be same as per your screenshot |
A comboBox with DLCR is slightly smaller than one without DLCR (138x25 vs 140x25). If I force the same dimensions with setSize, the RGB values are slightly off. If these comboBoxes are supposed to exactly match in dimensions, they currently don't, and this is due to what would be a separate bug if so. |
I thought the same, but Nimbus seems to act different for this instance only. I traced the conditional in SynthComboBoxRenderer, and this was added in Apr 25, 2009 here. This seems like the introduction of Nimbus and how it's enabled/disabled ComboBoxes for Nimbus, with the oversight of setting a different renderer. This looks like the only OS that does this like you mentioned. The reason why this need to setEnabled in the renderer for Nimbus wasn't stated in the comment or bug description page. But it does seem to still need it as this was the only fix I found. |
maybe you can try to compare the mid scanline rather than the whole rectangle |
Replaced the original test with an automated version using a mid scanline. I can't scan across the whole midline because of the width difference, so I adjusted the testing method slightly to accommodate for this. |
Did you try extending DLCR for
just as it is done here
|
I tried implementing this after your suggestion, but it doesn't achieve the wanted result. This makes sense from my understanding because with a new comboBox set to using a new DefaultListCellRenderer, the SynthComboBoxRenderer wouldn't be used. Changing what is extended for SynthComboBoxRenderer would only affect the normal comboBox using the default Synth renderer. The listener is the way around this because the comboBox with a DLCR still is a SynthComboBox but does not rely on the SynthComboBoxRenderer. |
+ "/" + lafName + "EnabledDLCR.png")); | ||
ImageIO.write(disabledImage2, "png", new File(testDir | ||
+ "/" + lafName + "DisabledDLCR.png")); | ||
System.out.println("DIR: " + testDir); |
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.
Is this needed? I know it is just a test but please remove the debug output when not needed.
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.
Removed the message. I added it to double check the saved images' file path, but didn't remove it before uploading.
SwingUtilities.invokeAndWait(() -> setLookAndFeel(laf)); | ||
SwingUtilities.invokeAndWait(DisabledComboBoxFontTestAuto::createCombo); | ||
SwingUtilities.invokeAndWait(DisabledComboBoxFontTestAuto::paintCombo); | ||
testMethod(); |
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.
So test will fail whenever it encounters the first wrong color? Ideally we would run test for all the installed LAFs anyways and report which ones are failed. But in this case you would need to pass the LAF name to the test method so it saves screenshots for all the LAFs for future analysis.
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 added a StringBuffer which now outputs the failed LAFs to the thrown runtime exception.
@DamonGuy 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:
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 754 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@honkar-jdk, @azuev-java, @prsadhuk) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
@prsadhuk @honkar-jdk Could you take another look because of the updates? |
It seems the test fails on windows for WIndows L&F..This is what I got
It seems it does not fail in mach5 maybe because it is not run in hidpi system. Also, I was expecting some explanation about this snippet if it is still needed for GTK (because of which, I think, you had to do the change in jdk/src/java.desktop/share/classes/javax/swing/plaf/synth/SynthComboBoxUI.java Lines 552 to 560 in 21a6ab1
You took care of |
I tested without any additional listener for orientation and set the combo's orientation to right_to_left. It's working correctly. So, I tested the code block you referenced above and removed the code for updating orientation when However, the enabled portion seems to be needed, and the additional listener added is needed to generate the correct text color. Nimbus test with a normal comboBox and a custom comboBox (with DefaultListCellRenderer) set with orientation RTL: Test with a disabled, normal comboBox and custom comboBox (with DefaultListCellRenderer) set with orientation RTL: |
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.
ok..
/integrate |
/sponsor |
Going to push as commit 87b314a.
Your commit was automatically rebased without conflicts. |
@azuev-java @DamonGuy Pushed as commit 87b314a. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
@prsadhuk Did you able to fix it, or report the new JBS bug for that? I have the same issue, the test fails on the HiDPI system. |
Before the fix, a JComboBox in Nimbus L&F would have normal black text even when the JComboBox was disabled if SynthComboBoxRenderer was replaced with a DefaultListCellRenderer. This text should be greyed out like in other L&F's. When looking into the defaults for Nimbus L&F files for attributes and states of a JComboBox, it confirm that the intention for disabled JComboBoxes is nimbusDisabledText (which is grey text).
SynthComboBoxes have an additional check in its default SynthComboBoxRenderer that enables/disables the renderer itself. The SynthComboBoxRenderer inherits its enabled state from the parent ComboBox. Since the renderer with DefaultListCellRenderer is in a separate class without a reference to the comboBox, a listener was added to SynthComboBoxUI.
An additional issue occurred in DefaultListCellRenderer because the renderer overrode the listener's re-assigned enabled state. In testing, setting the enabled state in DefaultListCellRenderer is redundant for all L&F's and is not needed here. However, instead of removing it altogether, a conditional was added specifically to allow ComboBoxes to skip setting enabled state here.
After the fix, the Nimbus JComboBox with DLCR set matches the appearance of a normal Nimbus JComboBox. I can enable/disable the JComboBoxes in the test, and the UI elements behave and appear as expected.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/12390/head:pull/12390
$ git checkout pull/12390
Update a local copy of the PR:
$ git checkout pull/12390
$ git pull https://git.openjdk.org/jdk.git pull/12390/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12390
View PR using the GUI difftool:
$ git pr show -t 12390
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12390.diff