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

Conversation

@prsadhuk
Copy link
Contributor

@prsadhuk prsadhuk commented Jul 29, 2021

The Nimbus look and feel ignores the configured background color of a JTextPane and always uses white.
Every other look and feel tested (Metal, Motif, and Windows) correctly honors the configured background color of a JTextPane.
Issue seems to be in the hardcoded background color in nimbus skin which is rectified via the fix.
After this fix, the background color is correctly updated to whatever is being set via JTextPane.setBackground().

CI all test run is green.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issues

  • JDK-8058704: Nimbus does not honor JTextPane background color
  • JDK-6789980: JEditorPane background color not honored with Nimbus L&F

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4930/head:pull/4930
$ git checkout pull/4930

Update a local copy of the PR:
$ git checkout pull/4930
$ git pull https://git.openjdk.java.net/jdk pull/4930/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4930

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4930.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 29, 2021

👋 Welcome back psadhukhan! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 29, 2021

@prsadhuk The following label will be automatically applied to this pull request:

  • swing

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.

Loading

@openjdk openjdk bot added the swing label Jul 29, 2021
@openjdk openjdk bot added the rfr label Jul 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 29, 2021

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Jul 30, 2021

Does the JEditorPane have the same issue?
https://bugs.openjdk.java.net/browse/JDK-6789980

Loading

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Jul 30, 2021

Does the JEditorPane have the same issue?
https://bugs.openjdk.java.net/browse/JDK-6789980

I noticed when running ButtonGroupLayoutTraversalTest.java test in Nimbus LaF that background color for JRadioButton is also ignored. The components change their color as focus is moved, but in Nimbus LaF none of the components changes the background color.

Loading

Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

You're removing the default background which was painted previously. Does it mean there's no default any more? What will paint the background if it's not set explicitly?

Loading

Loading
Loading
@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 2, 2021

Does the JEditorPane have the same issue?
https://bugs.openjdk.java.net/browse/JDK-6789980

Yes, fixed

Loading

@openjdk openjdk bot removed the rfr label Aug 2, 2021
@openjdk openjdk bot added the rfr label Aug 2, 2021
<paintPoints x1="0.25" y1="0.0" x2="0.2" y2="0.1"/>
</rectangle>
</shapes>
<shapes/>
Copy link
Member

@mrserb mrserb Aug 2, 2021

Choose a reason for hiding this comment

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

What is the difference between background and shape tags?

Loading

Copy link
Contributor Author

@prsadhuk prsadhuk Aug 3, 2021

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member

@mrserb mrserb Aug 3, 2021

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@prsadhuk prsadhuk Aug 3, 2021

Choose a reason for hiding this comment

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

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"

Loading

Copy link
Member

@mrserb mrserb Aug 3, 2021

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor Author

@prsadhuk prsadhuk Aug 3, 2021

Choose a reason for hiding this comment

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

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

Loading

Copy link
Contributor Author

@prsadhuk prsadhuk Aug 3, 2021

Choose a reason for hiding this comment

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

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)

Loading

Copy link
Member

@aivanov-jdk aivanov-jdk Aug 4, 2021

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member

@mrserb mrserb Aug 4, 2021

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@prsadhuk prsadhuk Aug 5, 2021

Choose a reason for hiding this comment

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

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.

Loading

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Aug 4, 2021

Does the JEditorPane have the same issue?
https://bugs.openjdk.java.net/browse/JDK-6789980

Yes, fixed

You should add JDK-6789980 as an additional issue resolved:

/issue add JDK-6789980

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 4, 2021

@aivanov-jdk Only the author (@prsadhuk) is allowed to issue the /issue command.

Loading

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Aug 4, 2021

The tests are nearly the same: the only difference is the component being tested. I suggest merging the two tests into one test which tests both components one after another.

Loading

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 5, 2021

Restored "shape" tag and now tackling the bug from the fact that JTextField/JTextARea setbackground works unlike JTextPane/EditorPane.
It seems TextPanePainter class is generated from skin template where for JTextField/Area we have

private void paintBackgroundEnabled(Graphics2D g) {
        this.rect = this.decodeRect1();
        g.setPaint((Color)this.componentColors[0]);
        g.fill(this.rect);
    }

but for JTextPane/EditorPane, we have

private void paintBackgroundEnabled(Graphics2D g) {
        this.rect = this.decodeRect1();
        g.setPaint((Color)this.color2);
        g.fill(this.rect);
    }

where color2 is NimubsLightBackground so it seems to always paint white.
Proposed fix is to add background componentPropertyName for ENABLED state (for the fact
getContext is always called with SynthContext context = getContext(comp, ENABLED);
so that correct background component color is painted.`

Loading

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 9, 2021

All CI test including JCK are green.

Loading

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 9, 2021

The tests are nearly the same: the only difference is the component being tested. I suggest merging the two tests into one test which tests both components one after another.

I would rather have as separate test as the fix is in 2 different component, I could have raised separate PRs as fix area is different although fix seems to be same..and I don't see it as anyhting other than test optimization..
If you absolutely cannot approve without test merged, then please let me know. I would try to do

Loading

@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 9, 2021

/issue add JDK-6789980

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 9, 2021

@prsadhuk
Adding additional issue to issue list: 6789980: JEditorPane background color not honored with Nimbus L&F.

Loading

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Aug 9, 2021

The tests are nearly the same: the only difference is the component being tested. I suggest merging the two tests into one test which tests both components one after another.

I would rather have as separate test as the fix is in 2 different component, I could have raised separate PRs as fix area is different although fix seems to be same..and I don't see it as anyhting other than test optimization..
If you absolutely cannot approve without test merged, then please let me know. I would try to do

I'm for fixing these two issues under one PR. The fix is basically the same, so it makes it easier to review and track changes later.

Yes, it's kind of a test optimisation. Yet the tests are similar in nature, they exercise the same behaviour in two different components. It just makes sense to re-use the code. If a fix is necessary to one test, it is necessary for the other — same idea why avoiding duplicate code is good.

It's not a show-stopper though if everyone agrees it's better to have two different tests.

Loading

@openjdk openjdk bot removed the rfr label Aug 9, 2021
@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 9, 2021

The tests are nearly the same: the only difference is the component being tested. I suggest merging the two tests into one test which tests both components one after another.

I would rather have as separate test as the fix is in 2 different component, I could have raised separate PRs as fix area is different although fix seems to be same..and I don't see it as anyhting other than test optimization..
If you absolutely cannot approve without test merged, then please let me know. I would try to do

I'm for fixing these two issues under one PR. The fix is basically the same, so it makes it easier to review and track changes later.

Yes, it's kind of a test optimisation. Yet the tests are similar in nature, they exercise the same behaviour in two different components. It just makes sense to re-use the code. If a fix is necessary to one test, it is necessary for the other — same idea why avoiding duplicate code is good.

It's not a show-stopper though if everyone agrees it's better to have two different tests.

OK..Not a problem. Merged...

Loading

@openjdk openjdk bot added the rfr label Aug 9, 2021
test/jdk/javax/swing/plaf/nimbus/TestNimbusBGColor.java Outdated Show resolved Hide resolved
Loading
test/jdk/javax/swing/plaf/nimbus/TestNimbusBGColor.java Outdated Show resolved Hide resolved
Loading
test/jdk/javax/swing/plaf/nimbus/TestNimbusBGColor.java Outdated Show resolved Hide resolved
Loading
@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Aug 9, 2021

Restored "shape" tag and now tackling the bug from the fact that JTextField/JTextARea setbackground works unlike JTextPane/EditorPane.

Now both JTextPane and JEditorPane have white background by default. The changes to the background color are honored.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 10, 2021

@prsadhuk 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:

8058704: Nimbus does not honor JTextPane background color
6789980: JEditorPane background color not honored with Nimbus L&F

Reviewed-by: aivanov, serb

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 189 new commits pushed to the master branch:

  • b62e742: 8213714: AttachingConnector/attach/attach001 failed due to "bind failed: Address already in use"
  • 66d1faa: 8271601: Math.floorMod(int, int) and Math.floorMod(long, long) differ in their logic
  • 57ae9fb: 8140442: Add getOutermostTypeElement to javax.lang.model utility class
  • 67869b4: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup
  • 35b399a: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
  • 2b05fae: 8260262: Use common code in function unmap_shared() in perfMemory_posix.cpp
  • f2599ad: 8272196: Remove unused class ParStrongRootsScope
  • 1f88134: 8271869: AArch64: build errors with GCC11 in frame::saved_oop_result
  • 089e83b: 8266490: Extend the OSContainer API to support the pids controller of cgroups
  • 2384e12: 8270098: ZGC: ZBarrierSetC2::clone_at_expansion fails with "Guard against surprises" assert
  • ... and 179 more: https://git.openjdk.java.net/jdk/compare/e4295ccfcdb16041d6f18fd64f7df3f740bf258f...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Aug 10, 2021
@mrserb
Copy link
Member

@mrserb mrserb commented Aug 10, 2021

This looks fine, please double-check other usages of "shapes and matte" in "skin.laf" probably we have similar bugs in other components?

Loading

mrserb
mrserb approved these changes Aug 10, 2021
@prsadhuk
Copy link
Contributor Author

@prsadhuk prsadhuk commented Aug 13, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 13, 2021

Going to push as commit 0c4be76.
Since your change was applied there have been 220 commits pushed to the master branch:

  • 020aec5: 8271366: [REDO] JDK-8266054 VectorAPI rotate operation optimization
  • 4d4ba5c: 8272116: Update PerfDisableSharedMem with FLAG_SET_ERGO in PerfMemory::create_memory_region
  • 09ab86b: 8269909: getStack method in hprof.parser.Reader should use try-with-resource
  • e4766ee: 8272391: Undeleted debug information
  • 428d516: 8140241: (fc) Data transfer from FileChannel to itself causes hang in case of overlap
  • 93cab7d: 8272315: Improve assert_different_registers
  • 9980b41: 8272107: Removal of Unsafe::defineAnonymousClass left a dangling C++ class
  • 464e874: 8048190: NoClassDefFoundError omits original ExceptionInInitializerError
  • 7e14c3c: 8272310: AArch64: Add missing changes for shared vector helper methods in m4 files
  • b29fbad: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable
  • ... and 210 more: https://git.openjdk.java.net/jdk/compare/e4295ccfcdb16041d6f18fd64f7df3f740bf258f...master

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Aug 13, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Aug 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 13, 2021

@prsadhuk Pushed as commit 0c4be76.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

@prsadhuk prsadhuk deleted the JDK-8058704 branch Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants