8289539: The color returned by CheckBox.interiorBackground is incorrect #10385

TejeshR13 · 2022-09-22T06:59:59Z

The color returned for InteriorBackground property is the default color used for only WindowsClassicLookAndFeel. For WindowsLookAndFeel the InteriorBackground color is not been used when checkbox paint happens. In WindowsLookAndFeel the CheckBox check/uncheck is drawn using ImageCache which is totally independent of InteriorBackground color in which the user expects it to be.
The proposed fix is to return null for WindowsLookAndFeel (which is what happens in other LookAndFeel like Metal/Synth/Motif/Aqua ) and return default color which is the actual color used in WindowsClassicLookAndFeel.

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-8289539: The color returned by CheckBox.interiorBackground is incorrect

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10385

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

Using diff file

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

bridgekeeper · 2022-09-22T07:01:00Z

👋 Welcome back tr! 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 · 2022-09-22T07:03:03Z

⚠️ @TejeshR13 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk · 2022-09-22T07:04:16Z

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

mlbridge · 2022-09-22T07:07:27Z

Webrevs

00: Full (ff011fb2)

honkar-jdk · 2022-09-22T19:15:05Z

test/jdk/javax/swing/JCheckBox/InteriorBackgroundTest.java

+/*
+ * @test
+ * @bug 8289539
+ * @key headful


Is this test marked headful to load system colors associated with the LAF?
Since we are not displaying anything on screen.

I specified as headful since I'm using JFrame to subclass and change the Look and Feel at Runtime. Not sure if I can make is headless.

Just because we are extending JFrame, i think there is no need to make it headful.
You can remove the @headful tag and see whether it runs in our CI systems.

jayathirthrao

Spent some time to check where all we are using these properties.
I see that we use "CheckBox.background" for CheckBoxIcon when it is pressed at https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java#L389

Can you check why we use "CheckBox.background" instead of "CheckBox.interiorBackground" in WindowsIconFactory and what is its behaviour in Windows 11?

jayathirthrao · 2022-09-26T15:14:53Z

test/jdk/javax/swing/JCheckBox/InteriorBackgroundTest.java

+/*
+ * @test
+ * @bug 8289539
+ * @key headful


Just because we are extending JFrame, i think there is no need to make it headful.
You can remove the @headful tag and see whether it runs in our CI systems.

jayathirthrao · 2022-09-26T15:18:26Z

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

@@ -657,7 +658,8 @@ protected void initComponentDefaults(UIDefaults table)
                  new WindowsDesktopProperty("win.caret.width", null),

            "CheckBox.font", ControlFont,
-            "CheckBox.interiorBackground", WindowBackgroundColor,
+            "CheckBox.interiorBackground", (xp != null)?


Do we return any color other than the default FFFFFF/null?
What is the use of "win.frame.backgroundColor" property when we always return table.get("window")?

win.frame.backgroundColor is key and value is color set for window. Default color is FFFFF/null, when it is set by user the color updates.

From WindowsDesktopProperty.java constructor it looks like these are not key value pairs. Similar observation is there in DesktopProperty.java.
It looks like table.get("window") is fallback when win.frame.backgroundColor is null.

Ok, but we are not modifying win.frame.backgroundColor property, only "CheckBox.interiorBackground" is been set to null. WindowBackgroundColor is also used in other properties, so nothing is modified except "CheckBox.interiorBackground".

TejeshR13 · 2022-09-27T04:32:48Z

Spent some time to check where all we are using these properties. I see that we use "CheckBox.background" for CheckBoxIcon when it is pressed at https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsIconFactory.java#L389

Can you check why we use "CheckBox.background" instead of "CheckBox.interiorBackground" in WindowsIconFactory and what is its behaviour in Windows 11?

Here Green color is interior background and red is Background. This we can set in WindowsClassic, unlike other Look and Feel.

jayathirthrao

We use GetSysColor(https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getsyscolor) to set different values for desktop properties at https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/native/libawt/windows/awt_DesktopProperties.cpp#L511

We store these values(or some default value if desktop property is null) as shown at : https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLookAndFeel.java#L508

And then map them to individual UI components like checkbox: https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLookAndFeel.java#L660

From https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getsyscolor we can see that some parameters are not supported in particular windows platforms.

I think we should find out whether there is any way to get proper interior background color or need to map to different Desktop color property in some platforms. If there is no way to get appropriate interior background color then we may return null after analysis. Just because we are returning null in other LAF's we should not right away return null here, it needs more analysis.

Useful link about how Windows desktop properties are mapped : https://docs.oracle.com/javase/8/docs/technotes/guides/swing/1.4/w2k_props.html

TejeshR13 · 2022-09-27T08:08:01Z

In other L&F there is no such checkbox.interiorBackground property hence it is null value when user tries to access it. In windows since in Classic its been used, that property exist for windowsL&F also (Which is not using that property). And also in Windows L&F I couldn't find a way to change the interior background of checkbox as in WindowsClassic, since the image is painted using ImageCache.

jayathirthrao · 2022-09-27T08:33:00Z

In other L&F there is no such checkbox.interiorBackground property hence it is null value when user tries to access it. In windows since in Classic its been used, that property exist for windowsL&F also (Which is not using that property). And also in Windows L&F I couldn't find a way to change the interior background of checkbox as in WindowsClassic, since the image is painted using ImageCache.

From where the ImageCache knows that it needs to draw/use blue bitmap for checkbox.interiorBackground for Windows 11?

TejeshR13 · 2022-09-27T08:41:47Z

In other L&F there is no such checkbox.interiorBackground property hence it is null value when user tries to access it. In windows since in Classic its been used, that property exist for windowsL&F also (Which is not using that property). And also in Windows L&F I couldn't find a way to change the interior background of checkbox as in WindowsClassic, since the image is painted using ImageCache.

From where the ImageCache knows that it needs to draw/use blue bitmap for checkbox.interiorBackground for Windows 11?

Couldn't find the exact source, but was able to verify while debugging in PaintSkin() where checkBox check/uncheck was handled by using ImageBuffer where it just toggle the image for check and uncheck in windows L&F. This image in windows10 and windows11 differs in interior background color.

jayathirthrao · 2022-09-27T09:00:35Z

There are multiple ImageCache/ImageBuffer pointers, Please point to github source code. I think we should find how this ImageCache knows why it should use "blue" colored bitmap and try to replicate the same for interiorBackgroundColor property. Since we don't have such property in other LAF's, we should avoid replicating same behaviour(of returning null) in Windows LAF.

TejeshR13 · 2022-09-27T10:03:30Z

There are multiple ImageCache/ImageBuffer pointers, Please point to github source code. I think we should find how this ImageCache knows why it should use "blue" colored bitmap and try to replicate the same for interiorBackgroundColor property. Since we don't have such property in other LAF's, we should avoid replicating same behaviour(of returning null) in Windows LAF.

https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/swing/CachedPainter.java#L205
Here in Image image = cache.getImage(key, config, w, h, args); the image is been returned.

jayathirthrao · 2022-09-28T07:32:37Z

Lets get inputs from @prsadhuk also on this. Whether to keep checkBox.interiorBackground valid only for WindowsClassicLAF and return null for WindowsLAF Or try to find ways to return valid value for WindowsLAF.

bridgekeeper · 2022-10-26T15:46:39Z

@TejeshR13 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

turbanoff · 2022-11-02T07:29:23Z

test/jdk/javax/swing/JCheckBox/InteriorBackgroundTest.java

+        SwingUtilities.updateComponentTreeUI(this);
+        Color color = UIManager.getColor("CheckBox.interiorBackground");
+
+        if(laf.contains("WindowsLookAndFeel") && color != null) {


Suggested change

if(laf.contains("WindowsLookAndFeel") && color != null) {

if (laf.contains("WindowsLookAndFeel") && color != null) {

turbanoff · 2022-11-02T07:29:30Z

test/jdk/javax/swing/JCheckBox/InteriorBackgroundTest.java

+        if(laf.contains("WindowsLookAndFeel") && color != null) {
+            throw new RuntimeException("CheckBox.interiorBackground not Null for " +
+                    laf);
+        } else if(laf.contains("WindowsClassicLookAndFeel") && color == null) {


Suggested change

} else if(laf.contains("WindowsClassicLookAndFeel") && color == null) {

} else if (laf.contains("WindowsClassicLookAndFeel") && color == null) {

bridgekeeper · 2022-11-30T10:00:52Z

@TejeshR13 This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

jayathirthrao · 2022-12-01T08:44:56Z

@TejeshR13 I see that this is PR is withdrawn because it was inactive. Are you working on it or is it on hold?

TejeshR13 · 2022-12-01T11:32:12Z

@TejeshR13 I see that this is PR is withdrawn because it was inactive. Are you working on it or is it on hold?

I have put this on hold and will raise a new PR as the fix for this will take time.

TejeshR13 added 13 commits January 2, 2022 20:01

Initial Commit.

288c597

Merge branch 'master' of github.com:TejeshR13/jdk

2403494

Merge remote-tracking branch 'upstream/master'

92823e0

Merge branch 'openjdk:master' into master

347af6e

Merge remote-tracking branch 'upstream/master'

98cf2b7

Merge branch 'master' of github.com:TejeshR13/jdk

8788395

Merge remote-tracking branch 'upstream/master'

a6d536c

Merge remote-tracking branch 'upstream/master'

c36d137

Merge remote-tracking branch 'upstream/master'

5a80d75

Merge remote-tracking branch 'upstream/master'

a7788e4

Merge remote-tracking branch 'upstream/master'

b0c4ead

Initial Test Fix

4b7a26f

Added Test case

d027961

New line added at end of test case

ff011fb

openjdk bot added the rfr Pull request is ready for review label Sep 22, 2022

openjdk bot added the client client-libs-dev@openjdk.org label Sep 22, 2022

honkar-jdk reviewed Sep 22, 2022

View reviewed changes

jayathirthrao reviewed Sep 26, 2022

View reviewed changes

jayathirthrao reviewed Sep 27, 2022

View reviewed changes

turbanoff reviewed Nov 2, 2022

View reviewed changes

bridgekeeper bot closed this Nov 30, 2022

8289539: The color returned by CheckBox.interiorBackground is incorrect #10385

8289539: The color returned by CheckBox.interiorBackground is incorrect #10385

TejeshR13 commented Sep 22, 2022 •

edited by openjdk bot

bridgekeeper bot commented Sep 22, 2022

openjdk bot commented Sep 22, 2022

openjdk bot commented Sep 22, 2022

mlbridge bot commented Sep 22, 2022

honkar-jdk Sep 22, 2022

TejeshR13 Sep 26, 2022

jayathirthrao Sep 26, 2022

jayathirthrao left a comment

jayathirthrao Sep 26, 2022

jayathirthrao Sep 26, 2022

TejeshR13 Sep 27, 2022

jayathirthrao Sep 27, 2022 •

edited

TejeshR13 Sep 27, 2022

TejeshR13 commented Sep 27, 2022

jayathirthrao left a comment

TejeshR13 commented Sep 27, 2022

jayathirthrao commented Sep 27, 2022

TejeshR13 commented Sep 27, 2022

jayathirthrao commented Sep 27, 2022

TejeshR13 commented Sep 27, 2022

jayathirthrao commented Sep 28, 2022

bridgekeeper bot commented Oct 26, 2022

turbanoff Nov 2, 2022

turbanoff Nov 2, 2022

bridgekeeper bot commented Nov 30, 2022

jayathirthrao commented Dec 1, 2022

TejeshR13 commented Dec 1, 2022

	if(laf.contains("WindowsLookAndFeel") && color != null) {
	if (laf.contains("WindowsLookAndFeel") && color != null) {

	} else if(laf.contains("WindowsClassicLookAndFeel") && color == null) {
	} else if (laf.contains("WindowsClassicLookAndFeel") && color == null) {

8289539: The color returned by CheckBox.interiorBackground is incorrect #10385

8289539: The color returned by CheckBox.interiorBackground is incorrect #10385

Conversation

TejeshR13 commented Sep 22, 2022 • edited by openjdk bot

Progress

Issue

Reviewing

bridgekeeper bot commented Sep 22, 2022

openjdk bot commented Sep 22, 2022

openjdk bot commented Sep 22, 2022

mlbridge bot commented Sep 22, 2022

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayathirthrao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayathirthrao Sep 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejeshR13 commented Sep 27, 2022

jayathirthrao left a comment

Choose a reason for hiding this comment

TejeshR13 commented Sep 27, 2022

jayathirthrao commented Sep 27, 2022

TejeshR13 commented Sep 27, 2022

jayathirthrao commented Sep 27, 2022

TejeshR13 commented Sep 27, 2022

jayathirthrao commented Sep 28, 2022

bridgekeeper bot commented Oct 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridgekeeper bot commented Nov 30, 2022

jayathirthrao commented Dec 1, 2022

TejeshR13 commented Dec 1, 2022

TejeshR13 commented Sep 22, 2022 •

edited by openjdk bot

jayathirthrao Sep 27, 2022 •

edited