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

8216358: [accessibility] [macos] The focus is invisible when tab to "Image Radio Buttons" and "Image CheckBoxes" #2384

Closed
wants to merge 4 commits into from

Conversation

@azuev-java
Copy link
Member

@azuev-java azuev-java commented Feb 3, 2021


Progress

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

Issue

  • JDK-8216358: [accessibility] [macos] The focus is invisible when tab to "Image Radio Buttons" and "Image CheckBoxes"

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2384/head:pull/2384
$ git checkout pull/2384

…Image Radio Buttons" and "Image CheckBoxes"

Since there are no borders around components Aqua L&F depends on the
checkbox and radiobox icon appearance to carry information about weither
it is focused or not. The standard icons have a halo effect that depicts
the focus present but when custom icons are used then there are no halo
effect exists. The idea here is to alter the icon of the focused
component adding the effect that will demonstrate the presence of the
keyboard focus on the component.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 3, 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
Copy link

@openjdk openjdk bot commented Feb 3, 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 Feb 3, 2021

@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Feb 3, 2021

This is how radio button group with custom icons looks without the fix. The focus is on the middle radio button.
nofix
This is how the same widget looks with the fix. Presence of the focus is clearly visible.
withfix


public void performTest() throws Exception {
try {
BufferedImage imageFocus1 = null;

This comment has been minimized.

@mrserb

mrserb Feb 3, 2021
Member

Probably the usage of Robot/Frame could be removed? Could you try to create the buffered image and button then paint the button to the graphics via button.paint(Graphics)? I guess in this case you can check the content of the buffered image directly w/o the robot.

This comment has been minimized.

@mrserb

mrserb Feb 4, 2021
Member

How it will work if the button was too small say w=4 pixels?

This comment has been minimized.

@azuev-java

azuev-java Feb 4, 2021
Author Member

  1. Fixed.
  2. If size of the icon based checkbox/radio button is less or equal than the icon size then the central part of the icon will be cropped and the halo will not be visible unless there are transparent pixels in the icon or icon is procedural and not paints on the whole icon area. We will not draw over other components in this case.
altIcon = AquaFocus.createFocusedIcon(altIcon, c, 2);
}

altIcon.paintIcon(c, g, iconRect.x - offset, iconRect.y - offset);

This comment has been minimized.

@mrserb

mrserb Feb 3, 2021
Member

Is it possible that due to offset on the left/top we will draw the icon outside the button bounds on the right/bottom?

This comment has been minimized.

@azuev-java

azuev-java Feb 4, 2021
Author Member

No. I am substracting the offset to keep drawn image inside the checkbox/radiobutton icon drawing box which is by default size of the icon+4 in width and height, hence i am shifting it up and left to avoid drawing outside the icon box. If size of the image button is artificially limited to the value less or equal of the icon size then halo will be clipped to stay within the borders of the component. In any case we will not overdraw outside of the component.

ImageIO.write(imageNoFocus, "png", new File("imageNoFocus.png"));
throw new Exception("Changing focus is not visualized");
}
} finally {

This comment has been minimized.

@mrserb

mrserb Feb 5, 2021
Member

try/finally block is not needed?

This comment has been minimized.

@azuev-java

azuev-java Feb 5, 2021
Author Member

No, forgot to remove it. Not that it makes test invalid, but better get rid of it. Fixed.

This comment has been minimized.

@mrserb

mrserb Feb 5, 2021
Member

Looks fine, I assume that this test was checked via mach5 since the fix is in aqua and the test is cross-platform.

This comment has been minimized.

@azuev-java

azuev-java Feb 5, 2021
Author Member

Yes, did two separate runs - with and without actual fix applied. Fails without the fix on mac os x, passes with the fix on all platforms.

@mrserb
mrserb approved these changes Feb 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 5, 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:

8216358: [accessibility] [macos] The focus is invisible when tab to "Image Radio Buttons" and "Image CheckBoxes"

Reviewed-by: serb, pbansal

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

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 Feb 5, 2021
@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Feb 5, 2021

/integrate

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

@openjdk openjdk bot commented Feb 5, 2021

@azuev-java Since your change was applied there have been 52 commits pushed to the master branch:

  • fac3c2d: 8254702: jpackage app launcher crashes on CentOS
  • 7a6c176: 8260736: Shenandoah: Cleanup includes in ShenandoahGC and families
  • 4a89733: 8261198: [macOS] Incorrect JNI parameters in number conversion in A11Y code
  • 4a1814c: 8261179: Norwegian Bokmål Locale fallback issue
  • 0218917: 8258732: runtime/cds/appcds/dynamicArchive/DynamicArchiveRelocationTest.java fails
  • f9df366: 8242300: SystemDictionary::resolve_super_or_fail() should look for the super class first
  • 43ae0cf: 8261167: print_process_memory_info add a close call after fopen
  • 48f5220: 8260369: [PPC64] Add support for JDK-8200555
  • 224c166: 8261213: [BACKOUT] MutableSpace's end should be atomic
  • 3495feb: 8260296: SA's dumpreplaydata fails
  • ... and 42 more: https://git.openjdk.java.net/jdk/compare/5324b5c582325b1703000fc9dbf8a2a1002bd2fc...master

Your commit was automatically rebased without conflicts.

Pushed as commit 440db35.

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

@azuev-java azuev-java deleted the azuev-java:JDK-8216358 branch Feb 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants