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

8208747: [a11y] [macos] In Optionpane Demo, inside ComponentDialog Example, unable to navigate to all items, with VO on #4084

Closed
wants to merge 1 commit into from

Conversation

azuev-java
Copy link
Member

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

Added accessibilityIndex function that correctly returns the index of a
child in parent container


Progress

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

Issue

  • JDK-8208747: [a11y] [macos] In Optionpane Demo, inside ComponentDialog Example, unable to navigate to all items, with VO on

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4084

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

Using diff file

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

…ample, unable to navigate to all items, with VO on

Added accessibilityIndex function that correctly returns the index of a
child in parent container
@bridgekeeper
Copy link

bridgekeeper bot commented May 18, 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 Pull request is ready for review label May 18, 2021
@openjdk
Copy link

openjdk bot commented May 18, 2021

@azuev-java The following label will be automatically applied to this pull request:

  • awt

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 awt client-libs-dev@openjdk.org label May 18, 2021
@mlbridge
Copy link

mlbridge bot commented May 18, 2021

Webrevs

@pankaj-bansal
Copy link

pankaj-bansal commented May 18, 2021

The bug is a confidential issue. Either the bug should not be confidential or this review should not be done here.

@azuev-java
Copy link
Member Author

azuev-java commented May 18, 2021

The bug is a confidential issue. Either the bug should not be confidential or this review should not be done here.

I do not see any potential reason why this issue might be confidential. It is not security related, can be reproduced using the readily available demo and affects a large part of functionality. Removing the confidential label from the issue.

@pankaj-bansal
Copy link

pankaj-bansal commented May 18, 2021

The bug is a confidential issue. Either the bug should not be confidential or this review should not be done here.

I do not see any potential reason why this issue might be confidential. It is not security related, can be reproduced using the readily available demo and affects a large part of functionality. Removing the confidential label from the issue.

That is what I thought. Anyway, I have done some basic testing and the fix looks fine. It does solve the issue. I will do some more testing and will approve accordingly.

@pankaj-bansal
Copy link

pankaj-bansal commented May 20, 2021

Is there a reason it is not causing an issue in case if JList? JList also has elements inside a container, but I see that we can navigate the elements in JList though there is also one issue that the VO always says "1 on n" (n is total number of elements) on selecting any element of list. I am just wondering why is this only needed for combobox and other components seem to work fine.

@azuev-java
Copy link
Member Author

azuev-java commented May 20, 2021

Is there a reason it is not causing an issue in case if JList? JList also has elements inside a container, but I see that we can navigate the elements in JList though there is also one issue that the VO always says "1 on n" (n is total number of elements) on selecting any element of list. I am just wondering why is this only needed for combobox and other components seem to work fine.

It is by far is not limited with JList. It also affects menu navigation with the accessibility shortcuts - in open menu with AS we can'[t walk past first item. It is particularly bad in JList because traversing trough JList causes a lot of events to be generated such as selection/deselection of the list elements, focus change and such. This causes OS to re-evaluate the current selection by asking either container or child about the current selection index and that is what generates so fierce special effect of multiple selection and selections changing because depending on the order in which system events got populated and developed we update selection in different (but always incorrect) order.

@pankaj-bansal
Copy link

pankaj-bansal commented May 20, 2021

Is there a reason it is not causing an issue in case if JList? JList also has elements inside a container, but I see that we can navigate the elements in JList though there is also one issue that the VO always says "1 on n" (n is total number of elements) on selecting any element of list. I am just wondering why is this only needed for combobox and other components seem to work fine.

It is by far is not limited with JList. It also affects menu navigation with the accessibility shortcuts - in open menu with AS we can'[t walk past first item. It is particularly bad in JList because traversing trough JList causes a lot of events to be generated such as selection/deselection of the list elements, focus change and such. This causes OS to re-evaluate the current selection by asking either container or child about the current selection index and that is what generates so fierce special effect of multiple selection and selections changing because depending on the order in which system events got populated and developed we update selection in different (but always incorrect) order.

No I did not mean this. The current bug is about selection issue in combobox. User can not navigate the combobox items using VO using the VO hot Keys and is always stuck on first element. Your change is fixing that error.

What I am asking is why the same issue is not observed in JList. Current issue is not present in JList and user can navigate the items using VO in JList. The VO output is always like "1 of n" (n is number of items) and this is also an issue, but I can navigate properly. I think the logic would be similar to navigate in the children and it should not cause issue in one component and not cause issue in other.
Also, I tried to navigate the items in Menu and I am able to navigate the items properly using VO keys + Up/Down keys. So the issue is not present there as well.

I have mostly used SwingSet2 and demos from https://docs.oracle.com/javase/tutorial/uiswing/examples/components/index.html to test these 3 components. I am just trying to find if there is something else done in wrong way in Combobox which is correct in JList and Menu. That may be the correct/better approach to this issue.

@azuev-java
Copy link
Member Author

azuev-java commented May 24, 2021

What I am asking is why the same issue is not observed in JList.

In JList it is observed differently because there the multiple selection is allowed and because of that fever events about items selected/deselected and focus change being generated. When we move cursor with the VO hot keys we got selection change events with currently chosen item and the first item in the list, that leads to adding of the chosen item to the selection. That does not change focus and focus event is not being generated but instead of moving cursor we just adjusting selection. But if you will try to move cursor with simple up/down arrows you will see that you can not navigate past second or third item - eventually the order of selection/focus events will bring it back to the beginning of the list. It all starts with the discrepancy between the container reporting which children is selected and the index of the item in parent container reported by the selected children.

@openjdk
Copy link

openjdk bot commented May 24, 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:

8208747: [a11y] [macos] In Optionpane Demo, inside ComponentDialog Example, unable to navigate to all items, with VO on

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

  • 54520fb: 8267580: The method JavacParser#peekToken is wrong when the first parameter is not zero
  • 3113910: 8267553: Extra JavaThread assignment in ClassLoader::create_class_path_entry()
  • 4d26f22: 8264304: Create implementation for NSAccessibilityToolbar protocol peer
  • 6288a99: 8267404: vmTestbase/vm/mlvm/anonloader/stress/oome/metaspace/Test.java failed with OutOfMemoryError
  • 71e2fa2: 8267531: [x86] Assembler::andb(Address,Register) encoding is incorrect
  • 4023646: 8266528: Optimize C2 VerifyIterativeGVN execution time
  • 2462316: 8261354: SIGSEGV at MethodIteratorHost
  • 72c9567: 8263486: Clean up MTLSurfaceDataBase.h
  • fe33343: 8256304: should MonitorUsedDeflationThreshold be experimental or diagnostic
  • 8f10c5a: 8267190: Optimize Vector API test operations
  • ... and 93 more: https://git.openjdk.java.net/jdk/compare/02f895c5f6f6de38549337d45ed8ba4c446e9677...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 May 24, 2021
@azuev-java
Copy link
Member Author

azuev-java commented May 24, 2021

/integrate

@openjdk openjdk bot closed this May 24, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 24, 2021
@openjdk
Copy link

openjdk bot commented May 24, 2021

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

  • 54520fb: 8267580: The method JavacParser#peekToken is wrong when the first parameter is not zero
  • 3113910: 8267553: Extra JavaThread assignment in ClassLoader::create_class_path_entry()
  • 4d26f22: 8264304: Create implementation for NSAccessibilityToolbar protocol peer
  • 6288a99: 8267404: vmTestbase/vm/mlvm/anonloader/stress/oome/metaspace/Test.java failed with OutOfMemoryError
  • 71e2fa2: 8267531: [x86] Assembler::andb(Address,Register) encoding is incorrect
  • 4023646: 8266528: Optimize C2 VerifyIterativeGVN execution time
  • 2462316: 8261354: SIGSEGV at MethodIteratorHost
  • 72c9567: 8263486: Clean up MTLSurfaceDataBase.h
  • fe33343: 8256304: should MonitorUsedDeflationThreshold be experimental or diagnostic
  • 8f10c5a: 8267190: Optimize Vector API test operations
  • ... and 93 more: https://git.openjdk.java.net/jdk/compare/02f895c5f6f6de38549337d45ed8ba4c446e9677...master

Your commit was automatically rebased without conflicts.

Pushed as commit 49f622c.

💡 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-8208747 branch Apr 7, 2022
@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awt client-libs-dev@openjdk.org client client-libs-dev@openjdk.org integrated Pull request has been integrated
2 participants