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

8322239: [macos] a11y : java.lang.NullPointerException is thrown when focus is moved on the JTabbedPane #17736

Closed
wants to merge 6 commits into from

Conversation

azuev-java
Copy link
Member

@azuev-java azuev-java commented Feb 6, 2024

Add null check for the Aqua LnF situation when tab is hidden die to the tabs overflow.


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

Issue

  • JDK-8322239: [macos] a11y : java.lang.NullPointerException is thrown when focus is moved on the JTabbedPane (Bug - P3)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17736

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

Using diff file

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

Webrev

Link to Webrev Comment

… focus is moved on the JTabbedPane

Add null check for the Aqua LnF situation when tab is hidden die to the tabs overflow.
@bridgekeeper
Copy link

bridgekeeper bot commented Feb 6, 2024

👋 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 Pull request is ready for review label Feb 6, 2024
@openjdk
Copy link

openjdk bot commented Feb 6, 2024

@azuev-java 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 Feb 6, 2024
@mlbridge
Copy link

mlbridge bot commented Feb 6, 2024

Webrevs

Copy link
Contributor

@savoptik savoptik left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kumarabhi006 kumarabhi006 left a comment

Choose a reason for hiding this comment

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

Verified the fix on MacOS 13.6.3. Looks good to me.

@aivanov-jdk
Copy link
Member

I wonder if it's appropriate that location or dimension become null. I would expect these properties always return a non-null value, maybe in exceptional conditions.

If a tab becomes hidden because of the overflow, it is appropriate to return null from getLocationOnScreen.

What about the number of tabs reported? Is the original count still reported? The accessibility software should be able to “see” the number of tabs hasn't reduced.

@azuev-java
Copy link
Member Author

wonder if it's appropriate that location or dimension become null. I would expect these properties always return a non-null value, maybe in exceptional conditions.

If a tab becomes hidden because of the overflow, it is appropriate to return null from getLocationOnScreen.

It is appropriate with the Aqua L&F since control is not showing on the screen but active and should be available for the accessibility subsystem. The accessible description of the jtabbedpane does mention correct number of tabs. Returning dimension(0,0,0,0) or location (0,0) is incorrect because in this case accessibility logic will move accessibility cursor to the wrong location and the order of navigation will be affected. This fix does not affect any other OS aside ow Mac OS because Aqua L&F only available on OS X.

@@ -2339,13 +2339,16 @@ public boolean contains(Point p) {
public Point getLocationOnScreen() {
Point parentLocation = parent.getLocationOnScreen();
Copy link
Member

Choose a reason for hiding this comment

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

possibly the "parent.getLocationOnScreen()" should be wrapped by the "try/catch IllegalComponentStateException"? If some parent is not visible then that exception will be thrown, but per the spec, the null should be returned.

I think this could be covered by the test since the change is in the public shared code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I have added the handler ICSE and a regression test.

import javax.swing.JPanel;
import javax.swing.JTabbedPane;
import javax.swing.SwingUtilities;
import java.awt.BorderLayout;
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: awt imports may be moved before swing and a11y imports.

try {
SwingUtilities.invokeAndWait(me::test);
} finally {
if(mainFrame != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(mainFrame != null) {
if (mainFrame != null) {

AccessibleComponent component = (AccessibleComponent) accessible;
Point p = component.getLocationOnScreen();
Rectangle r = component.getBounds();
} catch(NullPointerException npe){
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} catch(NullPointerException npe){
} catch (NullPointerException npe) {

try {
SwingUtilities.invokeAndWait(me::test);
} finally {
if (mainFrame != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be disposed on EDT.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

Choose a reason for hiding this comment

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

This is not thread-safe, the condition if (mainFrame != null) should also be inside invokeAndWait.

Maybe create a method to use method reference: invokeAndWait(me::dispose)? In this case, mainFrame could be made instance field.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

try {
SwingUtilities.invokeAndWait(me::test);
} finally {
if (mainFrame != null) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not thread-safe, the condition if (mainFrame != null) should also be inside invokeAndWait.

Maybe create a method to use method reference: invokeAndWait(me::dispose)? In this case, mainFrame could be made instance field.

Comment on lines 94 to 95
throw new RuntimeException("Unexpected NullPointerException " +
"while getting accessible component bounds: " + npe);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new RuntimeException("Unexpected NullPointerException " +
"while getting accessible component bounds: " + npe);
throw new RuntimeException("Unexpected NullPointerException " +
"while getting accessible component bounds", npe);

Preserve the full context of NPE for analysing the failure if it ever occurs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

AccessibleComponent component = (AccessibleComponent) accessible;
Point p = component.getLocationOnScreen();
Rectangle r = component.getBounds();
} catch (NullPointerException npe){
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} catch (NullPointerException npe){
} catch (NullPointerException npe) {

Missing space.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Comment on lines +88 to +90
if (accessible instanceof AccessibleComponent) {
try {
AccessibleComponent component = (AccessibleComponent) accessible;
Copy link
Member

Choose a reason for hiding this comment

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

You can you pattern matching if you like:

Suggested change
if (accessible instanceof AccessibleComponent) {
try {
AccessibleComponent component = (AccessibleComponent) accessible;
if (accessible instanceof AccessibleComponent component) {
try {

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this fix can be backported to the previous releases where this nice feature is not available i prefer not to use it. I guess backporting is why we can't get nice things for ourselves.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's a consideration. Pattern matching for instanceof is available since Java 16, so only 11 and below will need slight modification.

@openjdk
Copy link

openjdk bot commented Feb 15, 2024

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

8322239: [macos] a11y : java.lang.NullPointerException is thrown when focus is moved on the JTabbedPane

Reviewed-by: asemenov, abhiscxk, aivanov

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

  • 5a988a5: 8322750: Test "api/java_awt/interactive/SystemTrayTests.html" failed because A blue ball icon is added outside of the system tray
  • a231706: 8324580: SIGFPE on THP initialization on kernels < 4.10
  • 2564f0f: 8325906: Problemlist vmTestbase/vm/mlvm/meth/stress/compiler/deoptimize/Test.java#id1 until JDK-8320865 is fixed
  • 9a1b843: 8324584: Optimize Symbol and char* handling in ClassLoader
  • a0e5e16: 8325162: Remove duplicate GCMParameters class
  • 0d51b76: 8325877: Split up NativeCompilation.gmk
  • 2b1a840: 8325860: Serial: Move Generation.java to serial folder
  • b718ae3: 8325882: Serial: Move is_maximal_no_gc to TenuredGeneration
  • 0e2fdc9: 8324933: ConcurrentHashTable::statistics_calculate synchronization is expensive
  • 8cb9b47: 8321282: RISC-V: SpinPause() not implemented
  • ... and 204 more: https://git.openjdk.org/jdk/compare/e999dfcb405962bc4d77b9740d36193f1ebe4a2c...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 Pull request is ready to be integrated label Feb 15, 2024
@azuev-java
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Feb 15, 2024

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

  • 5a988a5: 8322750: Test "api/java_awt/interactive/SystemTrayTests.html" failed because A blue ball icon is added outside of the system tray
  • a231706: 8324580: SIGFPE on THP initialization on kernels < 4.10
  • 2564f0f: 8325906: Problemlist vmTestbase/vm/mlvm/meth/stress/compiler/deoptimize/Test.java#id1 until JDK-8320865 is fixed
  • 9a1b843: 8324584: Optimize Symbol and char* handling in ClassLoader
  • a0e5e16: 8325162: Remove duplicate GCMParameters class
  • 0d51b76: 8325877: Split up NativeCompilation.gmk
  • 2b1a840: 8325860: Serial: Move Generation.java to serial folder
  • b718ae3: 8325882: Serial: Move is_maximal_no_gc to TenuredGeneration
  • 0e2fdc9: 8324933: ConcurrentHashTable::statistics_calculate synchronization is expensive
  • 8cb9b47: 8321282: RISC-V: SpinPause() not implemented
  • ... and 204 more: https://git.openjdk.org/jdk/compare/e999dfcb405962bc4d77b9740d36193f1ebe4a2c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Feb 15, 2024
@openjdk openjdk bot closed this Feb 15, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 15, 2024
@openjdk
Copy link

openjdk bot commented Feb 15, 2024

@azuev-java Pushed as commit 3b1062d.

💡 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 JDK-8322239 branch March 14, 2024 12:21
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
5 participants