7188058: Background of TextComponents are not changing colors to the default disabled color when set to uneditable #19876

alisenchung · 2024-06-25T04:05:00Z

Currently the bug described in the issue is that the colors of the TextComponents do not change when setting TextComponents to uneditable. The default uneditable color (SystemColor.control) happens to be the same as the default for the editable color for some L&Fs, so the fix may not be initially noticeable. However, the bug still exists where the the color is not being changed when changing between editable and uneditable. You can check by changing TextComponent.getBackground() code to return Color.GRAY on line 342 and you can see that TextComponents are not changing to a gray background when set to uneditable.

This fix adds a private setBackground method in TextComponent so that TextArea and TextField can change the background color to the correct color (SystemColor.control) when set uneditable by overriding the TextComponent setEditable. You can verify the fix by changing this color to Color.GRAY and verifying the backgrounds change to gray when the TextComponents are disabled.

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

Integration blocker

⚠️ Title mismatch between PR and JBS for issue JDK-7188058

Issue

JDK-7188058: Background of uneditable TextComponents doesn't change to disabled color (Bug - P3) ⚠️ Title mismatch between PR and JBS.

Reviewers

Harshitha Onkar (@honkar-jdk - Reviewer) 🔄 Re-review required (review applies to 0b0f049c)
Damon Nguyen (@DamonGuy - Committer) 🔄 Re-review required (review applies to e83c29fe)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19876

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-25T04:05:23Z

👋 Welcome back achung! 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 · 2024-06-25T04:06:02Z

@alisenchung This change is no longer ready for integration - check the PR body for details.

openjdk · 2024-06-25T04:06:22Z

@alisenchung 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 · 2024-07-01T16:43:44Z

Webrevs

honkar-jdk · 2024-07-03T05:20:02Z

src/java.desktop/share/classes/java/awt/TextArea.java

+        super.setEditable(b);
+        Color defaultBackground = UIManager.getColor("TextArea.background");
+        if (!backgroundSetByClientCode) {
+            setBackground(b ? defaultBackground : SystemColor.control, false);


Is there a reason behind choosing SystemColor.control as the default TextComponent system color?
I see another option SystemColor.text - the description sounds more close to the background color of text components.

the SystemColor.control comes from the docs on TextComponent.setEditable

* If the flag is set to {@code true}, this text component * becomes user editable. If the flag is set to {@code false}, * the user cannot change the text of this text component. * By default, non-editable text components have a background color * of SystemColor.control. This default can be overridden by * calling setBackground.

DamonGuy · 2024-07-03T19:39:10Z

src/java.desktop/share/classes/java/awt/TextComponent.java

+    void setBackground(Color c, boolean setByClient) {
+        backgroundSetByClientCode = setByClient;
+        super.setBackground(c);
+    }
+


Does this need a method need a javadoc comment block like the setBackground method before it or like any of the methods in the class?

since this method isn't a public or protected method (it's package-private, so it can't be accessed by users) there's no need for a javadoc comment

Right. OK I'll wait for you to move the test to the AWT directory then because I do agree, the components involved seem to be AWT rather than Swing ones.

honkar-jdk · 2024-07-04T00:17:48Z

@alisenchung I missed noticing it earlier, since this is an AWT test a more appropriate location would be java/awt/TextComponent, currently it is in swing test folder.

alisenchung · 2024-07-25T18:32:54Z

@honkar-jdk @DamonGuy i've moved the test to the awt folder, can you take a look?

honkar-jdk

Changes look good.

prsadhuk · 2024-08-02T03:42:51Z

src/java.desktop/share/classes/java/awt/TextArea.java

+    @Override
+    public synchronized void setEditable(boolean b) {
+        super.setEditable(b);
+        Color defaultBackground = UIManager.getColor("TextArea.background");


I am not sure if it is ok to use swing "UIManager" class in awt
I guess long time back somebody maybe @prrace mentioned that we should avoid calling swing class from awt?
I see we use AttributeSet in TextComponent for accessibility usage but am not sure if we can use without restriction?

This call is only to get the default background of TextArea. Is there another way to grab the default color without using UIManager?

Yes, but as far I could see TextArea.background property is being set in
AquaLookAndFeel.java, MotifLookAndFeel.java, BasicLookAndFeel.java, XAWTLookAndFeel.java and WindowsLookAndFeel.java
and this L&F will not be set in AWT environment, if I am not wrong so you will either get garbage or wrong default color if you use UIManager.getColor..so I guess you need to use TextComponent.getBackground() which returns SystemColor.control if nothing is set

i've made the change to use TextComponent.getBackground() instead to get the correct background color

AWT components must not depend on javax.swing.UIManager — pluggable Look and Feels and UIManager are features of Swing.

Some AWT components are implemented using lightweight components from Swing, in this case it's appropriate to use javax.swing packages but not in the common code — in the peers.

As far as I can see, JDK-7188058 references Linux and macOS only. AWT component on Mac use Swing components; not sure about Linux though.

alisenchung · 2024-08-08T16:19:32Z

@prsadhuk @DamonGuy can you take a look at the latest changes?

DamonGuy

Updates since my last comment look good.

src/java.desktop/share/classes/java/awt/TextField.java

alisenchung · 2024-08-13T20:36:03Z

@prsadhuk can you take another look?

src/java.desktop/share/classes/java/awt/TextArea.java

prsadhuk · 2024-08-23T04:26:30Z

Code change looks good now but for me testcase is passing even without the fix..
Also, since the testcase is just testing the background color, I think it can be made automated and even headless by using textfield/textarea rendering to BufferedImage

kumarabhi006 · 2024-08-23T05:24:16Z

src/java.desktop/share/classes/java/awt/TextArea.java

    }

+    @Override
+    public synchronized void setEditable(boolean b) {


setEditable method is overridden in both TextArea and TextField class and method implementation is same. Since both of them are inherited from TextComponent class, could we move the code to TextComponent's setEditable method and remove them from respective classes?

This is a good suggestion.

kumarabhi006 · 2024-08-23T05:25:03Z

test/jdk/java/awt/TextComponent/BackgroundTest.java

@ Ran the test on mac machine and I observed no changes with or without your fix. And the text on first textfield seems selected and due to that it is of different color. Is it possible to make them unselected at test start up, so that the background color can be verified for enabled and disabled case easily?

Attached the screenshot for test ui.

Test_UI_On_StartUp

Test_UI_After_Click_DisableText_Button

Test_UI_After_Click_EnableText_Button

bridgekeeper · 2024-09-20T06:54:40Z

@alisenchung 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!

aivanov-jdk · 2024-09-23T13:08:21Z

Also, since the testcase is just testing the background color, I think it can be made automated and even headless by using textfield/textarea rendering to BufferedImage

AWT components cannot be created in headless environment.

aivanov-jdk · 2024-09-23T13:24:50Z

src/java.desktop/share/classes/java/awt/TextComponent.java

    }

+    void setBackground(Color c, boolean setByClient) {
+        backgroundSetByClientCode = setByClient;


Assigning the passed value to backgroundSetByClientCode does not make sense to me: the code in TextArea.setEditable already depends on the value of backgroundSetByClientCode, and the field shouldn't change as the result.

aivanov-jdk · 2024-09-23T13:25:59Z

src/java.desktop/share/classes/java/awt/TextArea.java

    }

+    @Override
+    public synchronized void setEditable(boolean b) {


This is a good suggestion.

aivanov-jdk · 2024-09-23T13:53:48Z

Is it a bug?

How do native text fields and areas behave on Linux and macOS? Does any of them change colours when it's switched into uneditable mode?

AWT components should behave as native controls do. On Windows, uneditable text field and text area usually have a different background which corresponds to background of a dialog box. Is it the case on Linux and macOS?

bridgekeeper · 2024-10-21T18:46:03Z

@alisenchung 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!

bridgekeeper · 2024-10-31T02:15:14Z

@alisenchung 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!

bridgekeeper · 2024-10-31T08:14:36Z

@alisenchung 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!

bridgekeeper · 2024-10-31T14:20:29Z

@alisenchung 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!

bridgekeeper · 2024-11-28T19:41:21Z

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

init commit

611bc7f

alisenchung marked this pull request as draft June 25, 2024 04:05

openjdk bot added the client client-libs-dev@openjdk.org label Jun 25, 2024

add test

2a0cdbd

alisenchung marked this pull request as ready for review July 1, 2024 16:39

openjdk bot added the rfr Pull request is ready for review label Jul 1, 2024

honkar-jdk reviewed Jul 3, 2024

View reviewed changes

DamonGuy reviewed Jul 3, 2024

View reviewed changes

moved test to awt

0b0f049

honkar-jdk approved these changes Jul 27, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 27, 2024

prsadhuk reviewed Aug 2, 2024

View reviewed changes

use getbackground

e83c29f

openjdk bot removed the ready Pull request is ready to be integrated label Aug 6, 2024

DamonGuy approved these changes Aug 8, 2024

View reviewed changes

prsadhuk reviewed Aug 9, 2024

View reviewed changes

src/java.desktop/share/classes/java/awt/TextField.java Outdated Show resolved Hide resolved

alisenchung added 2 commits August 12, 2024 09:26

remove uimanager from textfield

18f896b

remove import

ccf63c8

prsadhuk reviewed Aug 15, 2024

View reviewed changes

src/java.desktop/share/classes/java/awt/TextArea.java Outdated Show resolved Hide resolved

fix import

f8b9468

alisenchung requested a review from prsadhuk August 22, 2024 21:20

kumarabhi006 reviewed Aug 23, 2024

View reviewed changes

aivanov-jdk suggested changes Sep 23, 2024

View reviewed changes

bridgekeeper bot closed this Nov 28, 2024

Uh oh!

7188058: Background of TextComponents are not changing colors to the default disabled color when set to uneditable #19876

7188058: Background of TextComponents are not changing colors to the default disabled color when set to uneditable #19876

Uh oh!

Conversation

alisenchung commented Jun 25, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Integration blocker

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 25, 2024

Uh oh!

openjdk bot commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 25, 2024

Uh oh!

mlbridge bot commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

honkar-jdk commented Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alisenchung commented Jul 25, 2024

Uh oh!

honkar-jdk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alisenchung commented Aug 8, 2024

Uh oh!

DamonGuy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alisenchung commented Aug 13, 2024

Uh oh!

Uh oh!

prsadhuk commented Aug 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumarabhi006 Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Sep 20, 2024

Uh oh!

aivanov-jdk commented Sep 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk commented Sep 23, 2024

alisenchung commented Jun 25, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 25, 2024 •

edited

Loading

mlbridge bot commented Jul 1, 2024 •

edited

Loading

honkar-jdk commented Jul 4, 2024 •

edited

Loading

kumarabhi006 Aug 23, 2024 •

edited

Loading