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

7124282: [macosx] Can't see table cell highlighter when the highlight border is the same color as the cell. #7768

Closed
wants to merge 14 commits into from

Conversation

honkar-jdk
Copy link
Contributor

@honkar-jdk honkar-jdk commented Mar 10, 2022

Previously while tabbing through the JTable cell, the cell highlighter/focus ring was not visible against the selection background.

Changes are made to Aqua LAF to derive a lighter focus ring color by changing saturation and setting brightness component to 100% of original focus ring color so that it is visible while tabbing through JTable cells. A new method is added for this purpose which takes in focusRingColor, does adjustment to saturation and brightness and returns a new focus ring color. There are edge cases where the HSB transformation does not yield the right focus ring color, for these cases a default color is returned.

A test case is added to compare the RGB difference between the original focus ring color & selection background and the new focus ring color & selection background. A note on the test case, since the test case is automated it tests the cell focus ring with colors loaded during the start of program execution and any on-the-fly accent color changes were tested manually.

Edge Cases
image

The edge case condition consists of two parts –

  • To handle white/black colors - (hsbValues[0] == 0 && hsbValues[1] == 0) - representing hue and saturation of zero obtained for black, white or exactly grey (red=green=color) conditions

  • To handle grayish colors - hsbValues[1] <= satGrayScale where satGrayScale <= 0.10 . For any given hue and brightness, a saturation of less than or equal to 10% represents grayish tint colors. (The second case was added to accommodate grayish focus ring color returned by system when Accent color = Graphite. The returned color is not exactly gray but grayish (r=135, g=135, b=140)). To accommodate a more generic case of grayish colors, the satGrayScale has a threshold or tolerance of 0.10 or 10%.

Used the following resources to test out different grayish colors and optimal saturation offsets used in deriveLighterFocusRing().

A note on the system-colors:

For Mac 10.14 below:
keyboardfocusindicatorcolor
Issue: Works well for most of the scenarios (including on-the-fly changes) and follows the accent color changes except when we start the application with accent color set to Graphite and then change accent color on-the-fly The cell focus ring remains gray. The issue is with the system color returned by the type-property "keyboardfocusindicatorcolor" under this setting and NOT a JDK product bug.

For Mac 10.14+:
controlAccentColor
is available and it follows accent color changes (even when application started with Graphite as accent color). The proposed fix uses this type property when mac version is 10.14+

PS: The native L&F (Mac OS) and Swing L&F for JTable cell tabbing differs (on native tables the cell background turns white on focus with a cell focus ring). Since the background for Swing tables can be set by users and also overridden by subclassing DefaultTableCellRenderer, and to adhere to current implementation of Swing, the white cell background changes are not incorporated. Only the Focus Ring/ Cell Highlighter is made more prominent.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (1 review required, with at least 1 reviewer)

Issue

  • JDK-7124282: [macosx] Can't see table cell highlighter when the highlight border is the same color as the cell.

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7768

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 10, 2022

👋 Welcome back honkar-jdk! 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 openjdk bot changed the title JDK-7124282: [macosx] Can't see table cell highlighter when the highlight border is the same color as the cell 7124282: [macosx] Can't see table cell highlighter when the highlight border is the same color as the cell. Mar 10, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 10, 2022
@openjdk
Copy link

openjdk bot commented Mar 10, 2022

@honkar-jdk 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.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Mar 10, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 10, 2022

@honkar-jdk
Copy link
Contributor Author

honkar-jdk commented Mar 10, 2022

Before and After images of cell focus ring changes on SwingSet2. (cell focus on first cell)
CellFocusRing_JTable

@@ -889,7 +889,7 @@ public Object createValue(UIDefaults defaultsTable) {
"Table.gridColor", white, // grid line color
"Table.focusCellBackground", textHighlightText,
"Table.focusCellForeground", textHighlight,
"Table.focusCellHighlightBorder", focusCellHighlightBorder,
"Table.focusCellHighlightBorder", new BorderUIResource.LineBorderUIResource(deriveContrastFocusRing(selectionBackground), 2),
Copy link
Contributor

Choose a reason for hiding this comment

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

You have used border thickness as 2 without relying on scale factor. Did you check if it looks same in retina as well as non-retina display?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to do the same for List.focusCellHighlightBorder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsadhuk Thank you for reviewing.

Earlier when I used the default thickness (thickness=1) the cell focus ring was visible but not prominent as shown in the following screenshot. Please let me know if I need to change thickness to 1 or keep it the same?

Thickness=1

image

The thickness looks the same on retina as well as non-retina display. I have included the screenshots for comparison-

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since List.focusCellHighlightBorder uses the same focusRingColor as used by Table we can change it for List as well. Can I make changes to List using this issue and PR or should I track it by adding a new JBS bug/PR ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just thinking, Do we need to manipulate hue and saturation for focus ring?
It seems from native "Numbers" app which is closest to Tables & cells, I see the focus ring is always blue irrespective of what the selection background (and system accent color) is
and since Aqua should mimic native look, so I guess just this change will probably do

- Table.focusCellHighlightBorder", focusCellHighlightBorder,
+ Table.focusCellHighlightBorder", new BorderUIResouce.LineBorderUIResouce(Color.blue), 2),

Copy link
Contributor

@prsadhuk prsadhuk Mar 29, 2022

Choose a reason for hiding this comment

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

With latest repo and your fix, I put a debug log in deriveContrastFocusRing() and it seems to produce this output (for scenario above)

selectedBackgroundColor com.apple.laf.AquaImageFactory$SystemColorProxy[r=54,g=128,b=34]

I see, even if I change the highlight Color to blue, it does not call deriveContrastFocusRing() again and the focus ring remains green although background Color of JTable cell changes to blue [ I think deriveContrastFocusRing needs to be called every time accent color is changed in System->Preferences]

Screenshot 2022-03-29 at 12 49 59 PM

.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsadhuk I'm currently testing on mac 11.6 and observe that the background color for JTable follows accent color changes (in mac 11.6). Below are screenshots on mac 10.15 vs mac 11.6 under same settings

On Mac 10.15, JTable's background color seems to follow the highlight color changes whereas in 11.6 it follows the accent color changes. In both versions the selectionBackground is obtained from the same property - alternateSelectedControlColor

image

The other issue which was related to on-the-fly accent/highlight color changes corresponding to following case - deriveContrastFocusRing needs to be called every time there is change to System Preference colors is fixed and the code will be pushed shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsadhuk Pushed new changes related to -

  • On-the-fly focus ring changes
  • Since selectionBackground was following highlight color changes in mac 10.15 and accent color changes in mac 11.6, I have updated deriveContrastFocusRing() to use original focusRingColor (which follows accent color changes in both versions) to obtain the lighter version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we also need to do the same for List.focusCellHighlightBorder?

@prsadhuk Just wanted to include these screenshots for reference in case we decided to do similar changes for JList. Before and after images if the above changes are applied to Lists as well.

BEFORE

Single Selection

image

Multiple Selection

image

AFTER

Single Selection

image

Multiple Selection

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsadhuk I might have missed considering a more generic condition for the edge case. I'll push the line formatting changes, along with the these changes.

hsbValues[2] = brightnessValue;

//create and return Color corresponding to new hsbValues
return Color.getHSBColor(hsbValues[0], hsbValues[1], hsbValues[2]);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems you are only manipulating hue and saturation, not the brightness which is set to 1.0f throughout..need to change the javadoc comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I will change the Javadocs comments as suggested.
The reason for setting brightness to max value is that for any color returned by hue and saturation offset, setting brightness=1 gives the brightest color for the new color used for cell highlighting purpose.

Copy link
Contributor

@prsadhuk prsadhuk left a comment

Choose a reason for hiding this comment

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

Looks ok. There's some 80 character limit issue in testcase which you can rectify before integrating.

@openjdk
Copy link

openjdk bot commented Apr 1, 2022

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

7124282: [macosx] Can't see table cell highlighter when the highlight border is the same color as the cell.

Reviewed-by: psadhukhan, prr

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

  • 6a7c023: 8284316: Support accessibility ManualTestFrame.java for non SwingSet tests
  • 40470d8: 8284550: test failure_handler is not properly invoking jhsdb jstack, resulting in failure to produce a stack when a test times out
  • 837928b: 8222323: ChildAlwaysOnTopTest.java fails with "RuntimeException: Failed to unset alwaysOnTop"
  • 397d095: 8285743: Ensure each IntegerPolynomial object is only created once
  • 29ccb8f: 8285914: AppCDS crash when using shared archive with old class file
  • fe6e0c0: 8286371: Avoid use of deprecated str[n]icmp
  • 97a9835: 8274517: java/util/DoubleStreamSums/CompensatedSums.java fails with expected [true] but found [false]
  • 034f20f: 8212136: Remove finalizer implementation in SSLSocketImpl
  • 36e4df9: 8285516: clearPassword should be called in a finally try block
  • b849efd: 8285923: [REDO] JDK-8285802 AArch64: Consistently handle offsets in MacroAssembler as 64-bit quantities
  • ... and 160 more: https://git.openjdk.java.net/jdk/compare/d3ea4b7bb41a55143a125b451f4e2b0e1d03f38f...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prsadhuk, @prrace) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 1, 2022
@honkar-jdk
Copy link
Contributor Author

Looks ok. There's some 80 character limit issue in testcase which you can rectify before integrating.

@prsadhuk The code might require re-review as I have changed the edge case condition to accommodate grayish color (red, blue, green components are not exactly equal) and formatted the line lengths.

* @param focusRingColor - the {@code Color} object
* @return the {@code Color} object corresponding to new HSB values
*/
static Color deriveLighterFocusRing(Color focusRingColor) {
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 we are not generating a "lighter" focus ring now, just a prominent one, so I think this function needs renaming..

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I see if we start off with "Graphite" accent color then the focus ring is "Graphite" but then even if we change to any other accent color, the focus ring remains "Graphite"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsadhuk I was able to replicate the above scenario and as mentioned it oddly happens only when we start with Graphite as accent color. Looking into it further.

Copy link
Contributor Author

@honkar-jdk honkar-jdk Apr 6, 2022

Choose a reason for hiding this comment

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

Following type properties where checked for cellFocusRing color

  • alternateSelectedControlColor
    Issue: Doesn't follow accent color in macos 10.15 and deprecated property.

  • selectedContentBackgroundColor
    Issue: New property corresponding to alternateSelectedControlColor (deprecated). Causes build issues as the property is supported on macOS 10.14+ and the macOS deployment target version is 10.12 for jdk.

  • controlAccentColor
    Issue: Causes build issues due to version compatibility, property supported on only macOS 10.14+

  • accentColor
    Issue: Property supported on only macOS 10.15+, compatibility and run time issues.

  • keyboardfocusindicatorcolor
    Issue: Works well for most of the scenarios (including on-the-fly changes) and follows the accent color changes except when we start the application with accent color set to Graphite and then change accent color on-the-fly (as mentioned above). The cell focus ring remains gray. The issue seems to be with the system color returned by the type-property "keyboardfocusindicatorcolor" under this setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prrace @prsadhuk
The issue with the system color returned by keyboardFocusIndicatorColor mentioned above, persists on macOS 12.3 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.

It seems KEYBOARD_FOCUS_COLOR via focusRingColor is also used for menuSelectedBackgroundColor which is used for

CheckBoxMenuItem.selectionBackground
ComboBox.selectionBackground
Menu.selectionBackground
MenuBar.selectionBackground
MenuItem.selectionBackground
"PopupMenu.selectionBackground", menuSelectedBackgroundColor,
RadioButtonMenuItem.selectionBackground

which might also get affected if you use new property, so you need to test all those widgets if you intend to use reuse KEYBOARD_FOCUS_COLOR so I guess safe bet is to use new color as you mentioned to limit the change to highlight color.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsadhuk Following code changes were pushed and ready for re-review

  • New color added to CSystemColor.m - CELL_HIGHLIGHT_COLOR, to limit the effects of highlight color changes
  • AquaImageFactory and LWCToolkit changed as part of addition of new color
  • method renamed from deriveLighterFocusRing() to deriveProminentFocusRing()
  • Test case made headful to load system colors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsadhuk With the new changes a public method is added to AquaImageFactory, does this require a CSR?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are talking about getCellHighlightColorUIResource() then no. We don't have specs for this class.
I have added similar getSelectedControlColorUIResource method for JDK-8251377 without CSR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsadhuk Thank you! Noted. Yes, it is thegetCellHighlightColorUIResource() method.

Copy link
Contributor

@prsadhuk prsadhuk left a comment

Choose a reason for hiding this comment

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

Need more change

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Apr 5, 2022
@honkar-jdk
Copy link
Contributor Author

honkar-jdk commented Apr 25, 2022

Need more change

@prsadhuk Suggested changes have been incorporated. PR might require a re-review.

@@ -388,6 +388,9 @@ public Object createValue(UIDefaults defaultsTable) {
final Color focusRingColor = AquaImageFactory.getFocusRingColorUIResource();
final Border focusCellHighlightBorder = new BorderUIResource.LineBorderUIResource(focusRingColor);

// for table cell highlighter
final Color cellFocusRing = AquaImageFactory.getCellHighlightColorUIResource();
Copy link
Contributor

Choose a reason for hiding this comment

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

overall looks ok. I believe you have tested well in BigSur and Monterey also..
Minor nit, this cellFocusRing should be "cellFocusRingColor" like other color variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsadhuk I have tested it on BigSur and Monterey, the new changes are working well. I will change the variable name as suggested and update the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsadhuk Updated the PR. A note on the test case, since the test case is automated it tests the cell focus ring with colors loaded during the start of program execution and any on-the-fly accent color changes were tested manually.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 27, 2022
@honkar-jdk
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label May 9, 2022
@openjdk
Copy link

openjdk bot commented May 9, 2022

@honkar-jdk
Your change (at version e82a1ca) is now ready to be sponsored by a Committer.

@prsadhuk
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented May 10, 2022

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

  • 9a3cb93: 8030121: java/awt/dnd/MissingDragExitEventTest/MissingDragExitEventTest.java fails
  • ace4230: 8261650: Add a comment with details for MTLVC_MAX_INDEX
  • 2939553: 8282351: jpackage does not work if class file has $$ in the name on windows
  • 61450bb: 8283651: nsk/jvmti/SuspendThread/suspendthrd003 may leak native memory
  • c28a636: 8286442: ProblemList compiler/c2/irTests/TestSkeletonPredicates.java in -Xcomp mode
  • 54e3308: 8286348: incorrect use of @serial
  • b0d2b0a: 8286331: jni_GetStringUTFChars() uses wrong heap allocator
  • 902b1dd: 8284686: Interval of < 1 ms disables ExecutionSample events
  • 02e5fc0: 8286435: JDK-8284316 caused validate-source to fail in Tier1
  • 6a7c023: 8284316: Support accessibility ManualTestFrame.java for non SwingSet tests
  • ... and 169 more: https://git.openjdk.java.net/jdk/compare/d3ea4b7bb41a55143a125b451f4e2b0e1d03f38f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 10, 2022
@openjdk openjdk bot closed this May 10, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels May 10, 2022
@openjdk
Copy link

openjdk bot commented May 10, 2022

@prsadhuk @honkar-jdk Pushed as commit bd6026c.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
3 participants