8271923: [macos] the text color on the selected disabled tabbed pane button remains white making text unreadable #5217

prsadhuk · 2021-08-23T12:32:20Z

It is seen that if a JTabbedPane is unfocused, it's title is painted with white text on grey background
as opposed to black text on grey background in unfoucsed native app on macOSX Catalina
and is somewhat not legible.
This can be seen with SwingSet2 demo with InternalFrame or JTabbedPane demo and any native app, making focus toggle between the two.

Issue was TabbedPane always draw with "selectedTabTitleNormalColor" which is white. Although Aqua L&F defined selectedTabTitleDisabledColor but it is not used as TabbedPane does not check if focus is there in current frame and draw accordingly, which native app does.

Proposed fix is to check for frame is active or not and draw text color accordingly.
Since it is not affecting BigSur (where even if native app active or not text is always drawn in same color), it is only restricted to Catalina and lower.

Progress

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

Issue

JDK-8271923: [macos] the text color on the selected disabled tabbed pane button remains white making text unreadable

Reviewers

Jayathirth D V (@jayathirthrao - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5217

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

Using diff file

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

bridgekeeper · 2021-08-23T12:33:02Z

👋 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.

openjdk · 2021-08-23T12:35:56Z

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

2d
awt
swing

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

prsadhuk · 2021-08-23T12:38:27Z

/issue remove 2d,awt

openjdk · 2021-08-23T12:39:18Z

@prsadhuk This PR does not contain any additional solved issues that can be removed. To remove the primary solved issue, simply edit the title of this PR.

prsadhuk · 2021-08-23T12:39:38Z

/label remove 2d, awt

openjdk · 2021-08-23T12:40:25Z

@prsadhuk
The 2d label was successfully removed.

The awt label was successfully removed.

mlbridge · 2021-08-23T12:45:52Z

Webrevs

01: Full - Incremental (8ea8e3fc)
00: Full (8f0b80fb)

jayathirthrao · 2021-09-06T06:21:47Z

src/java.desktop/macosx/classes/com/apple/laf/AquaTabbedPaneContrastUI.java

@@ -77,7 +80,7 @@ protected void paintTitle(final Graphics2D g2d, final Font font, final FontMetri
    protected static Color getSelectedTabTitleColor(boolean enabled, boolean pressed) {
        if (enabled && pressed) {
            return UIManager.getColor("TabbedPane.selectedTabTitlePressedColor");
-        } else if (!enabled) {
+        } else if (!enabled || (!JRSUIUtils.isMacOSXBigSurOrAbove() && !isFrameActive)) {


Do we enter this condition in BigSur when SwingSet2 is not focused?

If yes, Will we not see difference in color between "new ColorUIResource(new Color(1, 1, 1, 0.55f))" and updated "black" color ?

If no, In BigSur from where black color is picked for selectedTabTitleDisabledColor?

In BigSur, there is no difference between if frame is focused or non-focused. It is always black on white (BigSur tabbedpane text design is different than Catalina).......it's not selectedTabTitleDisabledColor which governs this but selectedTabTitleNormalColor...There the text is governed by selectedControlTextColor via this code...

"TabbedPane.selectedTabTitleNormalColor", JRSUIUtils.isMacOSXBigSurOrAbove() ? selectedControlTextColor : selectedTabTitleNormalColor,

Does inactive frame lead to "enabled" having "false" value? What is the difference between "enabled" flag and "frameActive" ?

inactive frame is non-focused frame...
enabled false is tabbedpane is not enabled...

Let me elaborate my question, it is still not clear to me.

If i click on tabbedpane(enabled -> true), then move focus out of this Frame(SwingSet2) will that make enabled-> false? Is there any hierarchical relation between enabled & frameActive?

No, it will not. To make enabled false, you need to call tabPane.setEnabled(false);
frameActive is to decide if frame is focused or not.

Thanks for the clarification

jayathirthrao · 2021-09-06T07:08:36Z

Code change looks good to me. Any reason why we dont have regression test for this change? I think we can add simple TabbedPane scenario for non BigSur use case and read pixel data. Also there is no "noreg-*" label in JBS.

prsadhuk · 2021-09-06T07:20:23Z

We need to put focus on another window and also it canbe viewed by SwingSet2 and any other mac native window toggling the focus between the two which is why no reg test is added..I can add noreg label to it..

jayathirthrao · 2021-09-06T07:35:09Z

Does reading pixel data using Robot bring back focus on the frame? I am not sure about it. If that is the case then i can see why we might have issues adding regression test. Otherwise we can add regression test for this patch.

prsadhuk · 2021-09-06T07:38:48Z

No, the issue is we need to have 2 window...I am not sure how to add 2 window and bring focus to the other non-Java window automatically. So, I have relied on SwingSet2 to see the issue. You can have SwingSet2 and Preferences->Keyboard system dialog and toggle between the 2...It is already mentioned in JBS.

BTW, I am now using nonFocus color and not changing anything in if (!enabled)
condition so enabled case will not be affected at all and will not cause any regression for that case and it will solve text issue for focus case.

jayathirthrao · 2021-09-06T07:49:01Z

I dont think we need non-Java or 2 windows to reproduce the issue. We can just create 1 Java Frame with TabbedPane -> click on the tabbed pane -> and click Robot outside the scope of the Java Frame to see this issue.

After that we can read the pixel data of the text in TabbedPane to check whether we have issue or not(As already mentioned if focus comes back when we try to read pixel data, then i can see issue with writing regression test). Please clarify if i am missing something.

And its good that we are not touching if (!enabled) use case.

prsadhuk · 2021-09-06T09:04:33Z

It will be difficult to prove the issue for text color as robot will pick the pixels for tabbedpane along with background, not only text. Also, it's not for all macos, it's only for Catalina...we don't have requires tag to omit specific macos..

openjdk · 2021-09-07T13:47:15Z

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

8271923: [macos] the text color on the selected disabled tabbed pane button remains white making text unreadable

Reviewed-by: jdv

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

377b186: 8269119: C2: Avoid redundant memory barriers in Unsafe.copyMemory0 intrinsic
70157c7: 8272135: jshell: Method cannot use its overloaded version
5caa77b: 8263364: sun/net/www/http/KeepAliveStream/KeepAliveStreamCloseWithWrongContentLength.java wedged in getInputStream
a522d6b: 8273376: Zero: Disable vtable/itableStub gtests
3cd95a2: 8231356: Fix broken ResourceObj::operator new[] in debug builds
81c719b: 8273333: Zero should warn about unimplemented -XX:+LogTouchedMethods
649c22c: 8270832: Aarch64: Update algorithm annotations for MacroAssembler::fill_words
eb22181: 8273386: Remove duplicated code in G1DCQS::abandon_completed_buffers
fc546d6: 8273378: Shenandoah: Remove the remaining uses of os::is_MP
7bd4f49: 8264207: CodeStrings does not honour fixed address assumption.
... and 166 more: https://git.openjdk.java.net/jdk/compare/79a06df8113ba1da55db5c38fe34608c3507c223...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.

prsadhuk · 2021-09-07T15:49:05Z

/integrate

openjdk · 2021-09-07T15:50:38Z

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

2abf3b3: 8271340: Crash PhaseIdealLoop::clone_outer_loop
99fb12c: 8271341: Opcode() != Op_If && Opcode() != Op_RangeCheck) || outcnt() == 2 assert failure with Test7179138_1.java
041ae20: 8268558: [TESTBUG] Case 2 in TestP11KeyFactoryGetRSAKeySpec is skipped
377b186: 8269119: C2: Avoid redundant memory barriers in Unsafe.copyMemory0 intrinsic
70157c7: 8272135: jshell: Method cannot use its overloaded version
5caa77b: 8263364: sun/net/www/http/KeepAliveStream/KeepAliveStreamCloseWithWrongContentLength.java wedged in getInputStream
a522d6b: 8273376: Zero: Disable vtable/itableStub gtests
3cd95a2: 8231356: Fix broken ResourceObj::operator new[] in debug builds
81c719b: 8273333: Zero should warn about unimplemented -XX:+LogTouchedMethods
649c22c: 8270832: Aarch64: Update algorithm annotations for MacroAssembler::fill_words
... and 169 more: https://git.openjdk.java.net/jdk/compare/79a06df8113ba1da55db5c38fe34608c3507c223...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-09-07T15:50:52Z

@prsadhuk Pushed as commit df05b4d.

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

mrserb · 2021-09-08T06:24:23Z

I just completed a review of the previous fix(JDK-8269984) for jdk11, and found that the text is still unreadable if the selected tab is pressed by the user, look like it is still reproduced after this fix as well, please take a look.

prsadhuk · 2021-09-08T08:28:24Z

It's not clear where you tried 8269984 and this fix, is it on BigSur or Catalina? FYI, this fix is to cater to Catalina problem and will not have any effect on BigSur.

mrserb · 2021-09-08T18:19:36Z

I tested macos 11.5

prsadhuk · 2021-09-09T05:10:29Z

I raised https://git.openjdk.java.net/jdk/pull/5409 for the "pressed" issue on BigSur.

mrserb · 2021-09-10T04:31:31Z

I raised https://git.openjdk.java.net/jdk/pull/5409 for the "pressed" issue on BigSur.

Thank you!

Fix

e4290d2

openjdk bot added the rfr Pull request is ready for review label Aug 23, 2021

openjdk bot added 2d client-libs-dev@openjdk.org swing client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org labels Aug 23, 2021

openjdk bot removed 2d client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org labels Aug 23, 2021

Fix

8f0b80f

jayathirthrao reviewed Sep 6, 2021

View reviewed changes

Use NonFocus color

8ea8e3f

jayathirthrao approved these changes Sep 7, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 7, 2021

openjdk bot closed this Sep 7, 2021

openjdk bot added the integrated Pull request has been integrated label Sep 7, 2021

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 7, 2021

prsadhuk deleted the JDK-8271923 branch September 7, 2021 15:51

8271923: [macos] the text color on the selected disabled tabbed pane button remains white making text unreadable #5217

8271923: [macos] the text color on the selected disabled tabbed pane button remains white making text unreadable #5217

prsadhuk commented Aug 23, 2021 •

edited by openjdk bot

bridgekeeper bot commented Aug 23, 2021

openjdk bot commented Aug 23, 2021

prsadhuk commented Aug 23, 2021

openjdk bot commented Aug 23, 2021

prsadhuk commented Aug 23, 2021

openjdk bot commented Aug 23, 2021

mlbridge bot commented Aug 23, 2021 •

edited

jayathirthrao Sep 6, 2021

prsadhuk Sep 6, 2021

jayathirthrao Sep 6, 2021

prsadhuk Sep 6, 2021

jayathirthrao Sep 6, 2021

prsadhuk Sep 6, 2021

jayathirthrao Sep 6, 2021

jayathirthrao commented Sep 6, 2021

prsadhuk commented Sep 6, 2021

jayathirthrao commented Sep 6, 2021

prsadhuk commented Sep 6, 2021

jayathirthrao commented Sep 6, 2021

prsadhuk commented Sep 6, 2021

openjdk bot commented Sep 7, 2021 •

edited

prsadhuk commented Sep 7, 2021

openjdk bot commented Sep 7, 2021

openjdk bot commented Sep 7, 2021

mrserb commented Sep 8, 2021

prsadhuk commented Sep 8, 2021

mrserb commented Sep 8, 2021

prsadhuk commented Sep 9, 2021

mrserb commented Sep 10, 2021

8271923: [macos] the text color on the selected disabled tabbed pane button remains white making text unreadable #5217

8271923: [macos] the text color on the selected disabled tabbed pane button remains white making text unreadable #5217

Conversation

prsadhuk commented Aug 23, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 23, 2021

openjdk bot commented Aug 23, 2021

prsadhuk commented Aug 23, 2021

openjdk bot commented Aug 23, 2021

prsadhuk commented Aug 23, 2021

openjdk bot commented Aug 23, 2021

mlbridge bot commented Aug 23, 2021 • edited

Webrevs

jayathirthrao Sep 6, 2021

Choose a reason for hiding this comment

prsadhuk Sep 6, 2021

Choose a reason for hiding this comment

jayathirthrao Sep 6, 2021

Choose a reason for hiding this comment

prsadhuk Sep 6, 2021

Choose a reason for hiding this comment

jayathirthrao Sep 6, 2021

Choose a reason for hiding this comment

prsadhuk Sep 6, 2021

Choose a reason for hiding this comment

jayathirthrao Sep 6, 2021

Choose a reason for hiding this comment

jayathirthrao commented Sep 6, 2021

prsadhuk commented Sep 6, 2021

jayathirthrao commented Sep 6, 2021

prsadhuk commented Sep 6, 2021

jayathirthrao commented Sep 6, 2021

prsadhuk commented Sep 6, 2021

openjdk bot commented Sep 7, 2021 • edited

prsadhuk commented Sep 7, 2021

openjdk bot commented Sep 7, 2021

openjdk bot commented Sep 7, 2021

mrserb commented Sep 8, 2021

prsadhuk commented Sep 8, 2021

mrserb commented Sep 8, 2021

prsadhuk commented Sep 9, 2021

mrserb commented Sep 10, 2021

prsadhuk commented Aug 23, 2021 •

edited by openjdk bot

mlbridge bot commented Aug 23, 2021 •

edited

openjdk bot commented Sep 7, 2021 •

edited