Skip to content
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.

Sign up for GitHub

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

8075917: The regression-swing case failed as the text on label is not painted red with the GTK L&F #17763

Closed
wants to merge 10 commits into from
+125 −2
Closed

8075917: The regression-swing case failed as the text on label is not painted red with the GTK L&F #17763

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ee15cbd
JLable foreground color fix for GTK LAF
kumarabhi006 Feb 8, 2024
7775db1
jtreg tag headful removed
kumarabhi006 Feb 9, 2024
4403e6c
Headful tag revert back and condition check for nimbus
kumarabhi006 Feb 20, 2024
2ccca3f
extended fix for disabled checkbox and radiobutton
kumarabhi006 Mar 1, 2024
360e21e
separate method to get LAF defined color
kumarabhi006 Mar 5, 2024
2882785
Merge
kumarabhi006 Mar 26, 2024
56268d4
Revert back the product fix and update test to skip GTK L&F
kumarabhi006 May 16, 2024
ee16ea2
bug id added
kumarabhi006 May 17, 2024
27400b2
bug id removed
kumarabhi006 May 17, 2024
57d4c66
jtreg bug id tag update
kumarabhi006 May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java
View file
Original file line number Diff line number Diff line change
@@ -184,31 +184,31 @@ else if (type == GTKColorType.WHITE) {

if (id == Region.LABEL && type == ColorType.FOREGROUND
&& (state & SynthConstants.ENABLED) != 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't SynthConstants.DISABLED need the same treatment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I am working on it to fix the disabled case.

Copy link
Contributor Author

@kumarabhi006 kumarabhi006 Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prsadhuk @aivanov-jdk
I extended the fix for disabled state in GlyphView's paint method to honor the user defined LAF color if at all it is set.
But open/test/jdk/javax/swing/text/html/Test4783068.java test failed in CI, associated bug JDK-4783068 says that Components with HTML text should gray out the text when disabled.

Now the question is "Should we handle the disabled HTML text" ?

Color c = (Color) get(context, "Label.foreground");
if (c != null) {
return c;
}
return getLAFDefinedColor(context, state, type, "Label.foreground");
}

if (id == Region.CHECK_BOX && type == ColorType.FOREGROUND
&& (state & SynthConstants.DISABLED) != 0) {
Color c = (Color) get(context, "CheckBox.disabledText");
if (c != null) {
return c;
}
return getLAFDefinedColor(context, state, type, "CheckBox.disabledText");
}

if (id == Region.RADIO_BUTTON && type == ColorType.FOREGROUND
&& (state & SynthConstants.DISABLED) != 0) {
Color c = (Color) get(context, "RadioButton.disabledText");
if (c != null) {
return c;
}
return getLAFDefinedColor(context, state, type, "RadioButton.disabledText");
}

return getStyleSpecificColor(context, state, type);
}

private Color getLAFDefinedColor(SynthContext context, int state,
ColorType type, String propertyName) {
Color c = (Color) get(context, propertyName);
Copy link
Contributor

@prsadhuk prsadhuk Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess these condition checks should also be done inside the method since you are passing the values anyway...
Also, the enabled state are still not handled..If the existing testcase does not catch those, maybe we can add subtests to it..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the enabled state are still not handled

The enabled state is not handled in Metal LAF also, tried adding the enabledText property by setting ui property --
UIManager.getDefaults().put("CheckBox.enabledText", checkboxColor);

but the checkbox/radiobutton is painted with default black color.
Should we handle the change in metal LAF as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backtracking a bit, regarding Label.foreground why it needs to be done here?

Other L&F, it is added in corresponding LookAndFeel class
for example, in Metal
and in Windows
etc
so I think it is needed to be added in GTKLookAndFeel class

and additionally there is setting of Label.foreground and other Label. property in BasicLabelUI.java installDefaults
but we dont do it for Nimbus or GTK or Synth so I guess we need to call super.installDefaults(c) in SynthLabelUI class since there is no GTKLabelUI class
or
maybe do
LookAndFeel.installColorsAndFont(c, "Label.background", "Label.foreground", "Label.font"); alone to avoid other properties being set and to not fallback

Please check if it will work
and if it does, then we need to employ similar for checbox/radiobutton/togglebutton

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check and update.

Copy link
Contributor Author

@kumarabhi006 kumarabhi006 Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backtracking a bit, regarding Label.foreground why it needs to be done here?

Other L&F, it is added in corresponding LookAndFeel class for example, in Metal and in Windows etc so I think it is needed to be added in GTKLookAndFeel class

IN GTKLookAndFeel, the color is returned specific to the style and state from native side . So, even if it is not added the default color for enabled, disabled or any other state is returned by native. Whereas In Metal the color is set by using UIManager define color.

and additionally there is setting of Label.foreground and other Label. property in BasicLabelUI.java installDefaults
but we dont do it for Nimbus or GTK or Synth so I guess we need to call super.installDefaults(c) in SynthLabelUI class since there is no GTKLabelUI class
or
maybe do
LookAndFeel.installColorsAndFont(c, "Label.background", "Label.foreground", "Label.font"); alone to avoid other properties being set and to not fallback

This properties are set via SynthStyle installDefaults method where the foreground, background are set by getting the value from getColorForState method. So, calling super.installDefaults(c) in SynthLabelUI will not make any difference.

I checked for JLabel properties set in enabled and disabled state.

In Nimbus LAF
            UIManager.getDefaults().put("Label[Enabled].textForeground", labelColor);
            UIManager.getDefaults().put("Label[Disabled].textForeground", labelDisabledColor);

Other LAF
            UIManager.getDefaults().put("Label.foreground", labelColor);
            UIManager.getDefaults().put("Label.disabledForeground", labelDisabledColor);

It works as expected for Metal and GTK. For Nimbus LAF, disabled color is not painted correctly. In stead the default color specified in skin.laf is rendered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works as expected for Metal and GTK. For Nimbus LAF, disabled color is not painted correctly. In stead the default color specified in skin.laf is rendered.

For HTML text, disabled state doesn't render in LAF defined color for Metal and GTK as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks weird… So you're saying Label[Enabled].textForeground and Label[Disabled].textForeground are used for Nimbus (and Synth and GTK) instead of Label.foreground and Label.disabledForeground which are used for other L&Fs.

Shouldn't we fix the problem by correcting the keys instead? It looks like it's what you're doing for specific components.

Is it specified anywhere that Synth-based L&Fs use different constants? It results in incorrect colors.

If a developer sets the common properties, should they override Look-and-Feel defaults?

if (c != null) {
return c;
}
return getStyleSpecificColor(context, state, type);
}

/**
* Returns color specific to the current style. This method is
* invoked when other variants don't fit.
2 changes: 1 addition & 1 deletion test/jdk/javax/swing/plaf/basic/BasicHTML/bug4248210.java
View file
Original file line number Diff line number Diff line change
@@ -37,7 +37,7 @@

/*
* @test
* @bug 4248210 8075917
* @bug 4248210 8075917 4314194
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4314194 remove from here..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess both 8075917 4314194 needs to be removed as this PR is not having any product fix and this test is just being opensourced..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

* @key headful
* @summary Tests that HTML in JLabel is painted using LAF-defined
foreground color