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

8269951: [macos] Focus not painted in JButton when setBorderPainted(false) is invoked #5082

Closed
wants to merge 7 commits into from

Conversation

@azuev-java
Copy link
Member

@azuev-java azuev-java commented Aug 11, 2021

Initial implementation and a test case.

The problem is that Aqua LaF shows the focused component with the glow on the border, hence when the border is not painted the foxus is not displayed. The idea is to paint the glowing border on the focused component anyways.


Progress

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

Issue

  • JDK-8269951: [macos] Focus not painted in JButton when setBorderPainted(false) is invoked

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5082

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

Using diff file

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

azuev-java added 2 commits Aug 11, 2021
…alse) is invoked

Initial implementation of fix and automated test
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 11, 2021

👋 Welcome back kizune! 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 added the rfr label Aug 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 11, 2021

@azuev-java 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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 11, 2021

Rectangle viewRect, Rectangle textRect, Rectangle iconRect) {
final Border border = b.getBorder();
if (border instanceof AquaButtonBorder) {
((AquaButtonBorder)border).paintButton(b, g, 0, 0, b.getWidth(), b.getHeight());
Copy link
Contributor

@prsadhuk prsadhuk Aug 11, 2021

Choose a reason for hiding this comment

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

Does having 0,0 not create a problem in multiscreen environment?

Copy link
Member Author

@azuev-java azuev-java Aug 11, 2021

Choose a reason for hiding this comment

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

Does having 0,0 not create a problem in multiscreen environment?

These coordinates are relative within the Graphics clip of the button so no, that does not cause any issue. Plus, if it would then button painting would be broken already since that's exactly what is passed to the border painter in case when borders are not switched off.

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 12, 2021

Painting the border while the user tries to disable the border via setBorderPainted(false) does not look like a correct solution. The user might be drawn something there already. I remember we discussed a similar a11y bug related to focus and the only possible solution was to draw the focus ourselves or request this feature in the JRS.

@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Aug 12, 2021

Painting the border while the user tries to disable the border via setBorderPainted(false) does not look like a correct solution. The user might be drawn something there already. I remember we discussed a similar a11y bug related to focus and the only possible solution was to draw the focus ourselves or request this feature in the JRS.

There is no other way on Mac OS to indicate focused button but the glowing border. The same situation happens on Motif L&F - the only way to differentiate focused button from unfocused is by the border and in Motif - just as in Aqua - once border painting is disabled there is no way to tell if button is focused or not. User might draw something but if button becomes focused the focus indication will be drawn over it. If focus indicator disrupts user's painting on top of the button insets area user can always disable focus painting on such a component deliberately by calling JButton.setFocusPainted(boolean)

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 12, 2021

The motif L&F is not supported for a while and is ready to remove, so that L&F behavior cannot be referenced as a proper way to work.

The focused button on macOS is highlighted by the "blue" rounded rectangle. There are two choices: draw this rectangle ourselves or request this feature from the JRS. But the change should not break the behavior of the "setBorderPainted(false)".

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 12, 2021

BTW this issue was known for a long time, and even the spec was updated specifically for it:
https://bugs.openjdk.java.net/browse/JDK-4416026

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 12, 2021

Probably solution should be somewhere similar to this one:
440db35

@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Aug 13, 2021

There are two choices: draw this rectangle ourselves or request this feature from the JRS.

There i pushed the code that only draws the focus ring without the rest of the border.

RenderingHints.VALUE_ANTIALIAS_ON);

}
Color ringColor = UIManager.getLookAndFeelDefaults().getColor("Focus.color");
Copy link
Contributor

@prsadhuk prsadhuk Aug 13, 2021

Choose a reason for hiding this comment

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

I guess we normally call like this in Basic L&F which is extended by different L&Fs so that it will pick up the defaults from the particular L&F in question, otherwise UIManager.getColor(). should suffice as Focus.color is defined in AquaLookAndFeel.
But I am not sure with this hardcoded values..Can't we leverage viewRect or textRect to get the required coordinates?

Copy link
Member Author

@azuev-java azuev-java Aug 13, 2021

Choose a reason for hiding this comment

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

UIManager.getColor(). should suffice

Fixed.

But I am not sure with this hardcoded values..Can't we leverage viewRect or textRect to get the required coordinates?

viewRect is set to be exactly (0, 0, b.width, b.height). Insets are buried deep inside JRS classes and used as hardcoded valued, i can not get them from there and both textRect and iconRect are not representing what needs to be drawn to be consistent with the way OS draws focus. Plus they are being initialized only later down the code and at the time of this call they are empty. Not that it would matter.

@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Aug 15, 2021

Probably solution should be somewhere similar to this one

That works for icons but for text it would create a terrible mess, text with glow effect will be unreadable. I made code that draws the focus ring instead without drawing the rest of the border.

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 16, 2021

Can you please create of blend of two images, one for common focused button(with border) and another one w/o. Just to check that the border is drawn in the similar location and using similar "shape".

@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Aug 16, 2021

Can you please create of blend of two images, one for common focused button(with border) and another one w/o. Just to check that the border is drawn in the similar location and using similar "shape".

Here is the overlap of two images - with and without borders painted, the without borders painted has 80% opacity so borders look faint. I do not think that the focus ring will always be pixel perfect match with the natively painted - but they are close enough that switching border painting on and off i can not tell difference visually and the new focus ring is always contained within the button shape so it will not disrupt anything beyond it.

focus1b

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 16, 2021

That looks great.

}
Color ringColor = UIManager.getColor("Focus.color");
g.setColor(ringColor);
g.drawRoundRect(5, 3, b.getWidth() - 10, b.getHeight() - 7, 15, 15);
Copy link
Member

@mrserb mrserb Aug 16, 2021

Choose a reason for hiding this comment

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

Don't you need to restore the properties of Graphics2D?

Copy link
Member Author

@azuev-java azuev-java Aug 16, 2021

Choose a reason for hiding this comment

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

Don't you need to restore the properties of Graphics2D?

I might as well.

Copy link
Member

@mrserb mrserb Aug 16, 2021

Choose a reason for hiding this comment

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

What about the color property?

Copy link
Member Author

@azuev-java azuev-java Aug 16, 2021

Choose a reason for hiding this comment

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

What about the color property?

That would be unnecessary, whoever does painting does not just assumes that the color is set to his preferred choice, everywhere in the code color is set up before doing any painting and i do not see anywhere that old color being preserved.

Copy link
Member

@mrserb mrserb Aug 16, 2021

Choose a reason for hiding this comment

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

You can trace the usage of graphics.getColor() for example in AquaMenuItemUI/WindowsMenuUI/BevelBorder/etc to check that the old color property is usually saved and then restored.

Copy link
Member Author

@azuev-java azuev-java Aug 17, 2021

Choose a reason for hiding this comment

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

You can trace the usage of graphics.getColor() for example in AquaMenuItemUI/WindowsMenuUI/BevelBorder/etc to check that the old color property is usually saved and then restored.

A few lines above paintFocus() is called there is a section that sets color to something different in case of opaque button and that does not affect the next step - text painting - since text painter will grab text color and set the graphics draw color to it. However in the interest of moving forward i will ad saving the color - after al it is not a performance critical task.

Copy link
Contributor

@prsadhuk prsadhuk Aug 18, 2021

Choose a reason for hiding this comment

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

Have you checked if button does not have any text? It might affect the drawRoundRect values which are hardcoded now...In MetalButtonUI#paintFocus, it seems they cater to Button focus ring with and without text by taking care of setBounds(). Do we need to do something similar here?

Copy link
Member Author

@azuev-java azuev-java Aug 18, 2021

Choose a reason for hiding this comment

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

Have you checked if button does not have any text?

Yes, i did. Neither my code nor native focus painter is affected by it.

mrserb
mrserb approved these changes Aug 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 17, 2021

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

8269951: [macos] Focus not painted in JButton when  setBorderPainted(false) is invoked

Reviewed-by: serb, psadhukhan

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

  • 5189047: 8272558: IR Test Framework README misses some flags
  • ec63957: 8272398: Update DockerTestUtils.buildJdkDockerImage()
  • 14623cd: 8270835: regression after JDK-8261006
  • fe72197: 8272551: mark hotspot runtime/modules tests which ignore external VM flags
  • 05d64da: 8272291: mark hotspot runtime/logging tests which ignore external VM flags
  • a68b5b9: 8272369: java/io/File/GetXSpace.java failed with "RuntimeException: java.nio.file.NoSuchFileException: /run/user/0"
  • a199ebc: 8272581: sun/security/pkcs11/Provider/MultipleLogins.sh fails after JDK-8266182
  • 1cbf41a: 8225083: Remove Google certificate that is expiring in December 2021
  • cf64c3e: 8272326: java/util/Random/RandomTestMoments.java had two Gaussian fails
  • 2ed7b70: 8272521: Remove unused PSPromotionManager::_claimed_stack_breadth
  • ... and 116 more: https://git.openjdk.java.net/jdk/compare/6c8441f075b349d95ef26f51e8b9fd473748ac64...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.

@openjdk openjdk bot added the ready label Aug 17, 2021
@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Aug 18, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Aug 18, 2021

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

  • 5189047: 8272558: IR Test Framework README misses some flags
  • ec63957: 8272398: Update DockerTestUtils.buildJdkDockerImage()
  • 14623cd: 8270835: regression after JDK-8261006
  • fe72197: 8272551: mark hotspot runtime/modules tests which ignore external VM flags
  • 05d64da: 8272291: mark hotspot runtime/logging tests which ignore external VM flags
  • a68b5b9: 8272369: java/io/File/GetXSpace.java failed with "RuntimeException: java.nio.file.NoSuchFileException: /run/user/0"
  • a199ebc: 8272581: sun/security/pkcs11/Provider/MultipleLogins.sh fails after JDK-8266182
  • 1cbf41a: 8225083: Remove Google certificate that is expiring in December 2021
  • cf64c3e: 8272326: java/util/Random/RandomTestMoments.java had two Gaussian fails
  • 2ed7b70: 8272521: Remove unused PSPromotionManager::_claimed_stack_breadth
  • ... and 116 more: https://git.openjdk.java.net/jdk/compare/6c8441f075b349d95ef26f51e8b9fd473748ac64...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Aug 18, 2021

@azuev-java Pushed as commit 481c1f0.

💡 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
3 participants