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.

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

8058704: Nimbus does not honor JTextPane background color #4930

Closed
wants to merge 11 commits into from
@@ -25694,9 +25694,7 @@
<style>
<textForeground/>
<textBackground/>
<background>
<matte red="255" green="255" blue="255" alpha="255" hueOffset="0.0" saturationOffset="0.0" brightnessOffset="0.0" alphaOffset="0"/>
</background>
<background/>
<cacheSettingsInherited>false</cacheSettingsInherited>
<cacheMode>NO_CACHING</cacheMode>
<uiproperties/>
@@ -25750,10 +25748,6 @@
<locked>false</locked>
<visible>true</visible>
<shapes>

This comment has been minimized.

@mrserb

mrserb Aug 2, 2021
Member Outdated

What is the difference between background and shape tags?

This comment has been minimized.

@prsadhuk

prsadhuk Aug 3, 2021
Author Contributor Outdated

I have looked at https://docs.oracle.com/javase/tutorial/uiswing/lookandfeel/_nimbusDefaults.html where default color for
EditorPane.background and TextPane.background is NimbusBlueGrey and not White what we have now, so I think we are having what is being specified in nimbus-default.
SwingSet2 background color for the demos are same before and after the fix.
I am not sure about the difference between background and shape but I have ran full JCK/JTREG tests and all are fine. I also tried passing Nimbus L&F via CI script for JCK/jtreg tests and those are also same before and after fix.

This comment has been minimized.

@mrserb

mrserb Aug 3, 2021
Member Outdated

In the linke above the
nimbusBlueGrey #a9b0be (169,176,190)
While the "EditorPane.background and TextPane.background" are:
TextPane.background #d6d9df (214,217,223)
EditorPane.background #d6d9df (214,217,223)

It is still unclear why setting these values in the XML file prevents the user to change it later, also it will be good to understand the purpose of "shape" since it also sets some coordinates values.

This comment has been minimized.

@prsadhuk

prsadhuk Aug 3, 2021
Author Contributor Outdated

Yes, I meant the grey background what is being described in EditorPane.background and TextPane.background is what is being shown now, not literally NimbusBlueGrey. Till now we were showing white which is in contrary to this default.

I could not find any purpose of shape by looking at skin.laf as it is not being used in Synth source code, so that is why I ran all JCK/jtreg testsuite and found no regression. It is also there from initial days.
I leave it for user of Synth/Nimbus L&F or anyone from open community to highlight the need of "shape"

This comment has been minimized.

@mrserb

mrserb Aug 3, 2021
Member Outdated

Does it mean that "shapes" tag is not used and we can remove all its usage in the file?

This comment has been minimized.

@prsadhuk

prsadhuk Aug 3, 2021
Author Contributor Outdated

I dont know. I am only telling by looking at source code..I dont know what NimbusL&F creator meant it for...

This comment has been minimized.

@prsadhuk

prsadhuk Aug 3, 2021
Author Contributor Outdated

I think we should give this fix a chance...sInce this is solving 2 long-standing issue where setBackground() contract is not honoured plus causing no regression to existing tests.
If we do find any testcase with "shape" in future from open community or any other related testcase being failed for this fix, we can always work on it (or at the least roll back this)

This comment has been minimized.

@aivanov-jdk

aivanov-jdk Aug 4, 2021
Member Outdated

It looks the fix doesn't work or alternatively it works in an unexpected way.

Both JEditorPane and JTextPane used to have white background but now, with these changes, their background is the same of the panel below. It's probably know what we want.

This comment has been minimized.

@mrserb

mrserb Aug 4, 2021
Member Outdated

The last time this code was touched we get a regression https://bugs.openjdk.java.net/browse/JDK-8266510 which was not found by any tests. JCK cannot found them since this is L&F dependent. And regression tests do not have related tests because we had never bugs here, it worked this way from the beginning.

So it will be good to understand why these tags prevent the color change, how the shape tags are used, instead of some unknown changes here and there.

This comment has been minimized.

@prsadhuk

prsadhuk Aug 5, 2021
Author Contributor Outdated

JDK-8266510 regression was found by SwingSet2 so it is wrong to say it was not found by any test, maybe it was not verified during fix of JDK-8249674. For this, all existing test is run.

<rectangle x1="0.0" x2="100.0" y1="0.0" y2="30.0" rounding="0.0">
<matte red="255" green="255" blue="255" alpha="255" uiDefaultParentName="nimbusLightBackground" hueOffset="0.0" saturationOffset="0.0" brightnessOffset="0.0" alphaOffset="0"/>
<paintPoints x1="0.25" y1="0.0" x2="0.2" y2="0.1"/>
</rectangle>
</shapes>
<effects/>
</layer>
@@ -0,0 +1,77 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
/*
* @test
* @bug 8058704
* @key headful
* @summary Verifies if Nimbus honor JTextPane background color
* @run main TestNimbusJTextPaneColor
*/
import java.awt.Color;
import java.awt.Dimension;
import java.awt.Point;
import java.awt.Robot;
import javax.swing.JFrame;
import javax.swing.JTextPane;
import javax.swing.SwingUtilities;
import javax.swing.UIManager;

public class TestNimbusJTextPaneColor {

static JFrame frame;

public static void main(String[] args) throws Exception {
try {
SwingUtilities.invokeAndWait(() -> {
try {
UIManager.setLookAndFeel("javax.swing.plaf.nimbus.NimbusLookAndFeel");
} catch (Exception checkedExceptionsPleaseDie) {
throw new RuntimeException(checkedExceptionsPleaseDie);
}
JTextPane tp = new JTextPane();
tp.setForeground(Color.WHITE);
tp.setBackground(Color.BLACK);
This conversation was marked as resolved by prsadhuk

This comment has been minimized.

@prsadhuk

prsadhuk Jul 29, 2021
Author Contributor Outdated

There's a brief flash of white background before the actual and proper background color is set. Since the functionality of JTextPane background is rectified/restored, I have raised the PR and I will work on the initial white flashing in a subsequent PR.

tp.setText("This text should be white on black");

frame = new JFrame();
frame.setDefaultCloseOperation(frame.DISPOSE_ON_CLOSE);
frame.add(tp);
frame.setSize(new Dimension(480, 360));
frame.setLocationRelativeTo(null);
frame.setVisible(true);
});
Thread.sleep(1000);
Robot robot = new Robot();
Point pt = frame.getLocationOnScreen();
This conversation was marked as resolved by prsadhuk

This comment has been minimized.

@aivanov-jdk

aivanov-jdk Jul 30, 2021
Member Outdated

getLocationOnScreen should be called on EDT.

if (!(robot.getPixelColor(pt.x + frame.getBounds().width/2,
pt.y + frame.getBounds().height/2)
.equals(Color.BLACK))) {
throw new RuntimeException("JTextPane Color not same as the color being set");
}
} finally {
if (frame != null) {
SwingUtilities.invokeAndWait(frame::dispose);
}
This conversation was marked as resolved by prsadhuk
Comment on lines +72 to +74

This comment has been minimized.

@aivanov-jdk

aivanov-jdk Jul 30, 2021
Member Outdated

frame is accessed on main thread without synchronisation.

This comment has been minimized.

@prsadhuk

prsadhuk Aug 2, 2021
Author Contributor Outdated

NimbusLookAndFeel is customized look and feel but I don't see mention of default background color to be white in spec..If it is not set explicitly, background color is drawn in "NimbusBlueGrey" background which seems to be same as what is used in SwingSet2 demos in Nimbus L&F

This comment has been minimized.

@mrserb

mrserb Aug 2, 2021
Member Outdated

This is not specified since the color of L&F are implementation details, for Nimbus, some of them are described here:
https://docs.oracle.com/javase/tutorial/uiswing/lookandfeel/_nimbusDefaults.html

So when you delete the color from the xml, does the SwingSet2 still use the same color as before? Then for what the color in the xml was responsible?

}
}
}