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

8267385: Create NSAccessibilityElement implementation for JavaComponentAccessibility #4412

Closed
wants to merge 15 commits into from

Conversation

@savoptik
Copy link
Contributor

@savoptik savoptik commented Jun 8, 2021

8267385: Create NSAccessibilityElement implementation for JavaComponentAccessibility
This pull request contains solutions for the following tickets:

  • JDK-8267385 Create NSAccessibilityElement implementation for JavaComponentAccessibility;
  • JDK-8262031 Create implementation for NSAccessibilityNavigableStaticText protocol;
  • JDK-8264287 Create implementation for NSAccessibilityComboBox protocol peer;
  • JDK-8264303 Create implementation for NSAccessibilityTabGroup protocol peer;
  • JDK-8264292 Create implementation for NSAccessibilityList protocol peer;
  • JDK-8267387 Create implementation for NSAccessibilityOutline protocol;
  • JDK-8267388 Create implementation for NSAccessibilityTable protocol.

Progress

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

Issues

  • JDK-8267385: Create NSAccessibilityElement implementation for JavaComponentAccessibility
  • JDK-8262031: Create implementation for NSAccessibilityNavigableStaticText protocol
  • JDK-8264287: Create implementation for NSAccessibilityComboBox protocol peer
  • JDK-8264303: Create implementation for NSAccessibilityTabGroup protocol peer
  • JDK-8264292: Create implementation for NSAccessibilityList protocol peer
  • JDK-8267387: Create implementation for NSAccessibilityOutline protocol
  • JDK-8267388: Create implementation for NSAccessibilityTable protocol
  • JDK-8264286: Create implementation for NSAccessibilityColumn protocol peer
  • JDK-8264298: Create implementation for NSAccessibilityRow protocol peer
  • JDK-8264291: Create implementation for NSAccessibilityCell protocol peer

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4412

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 8, 2021

👋 Welcome back savoptik! 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 Jun 8, 2021

@savoptik this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8152350-6
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk
Copy link

@openjdk openjdk bot commented Jun 8, 2021

@savoptik 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 label Jun 8, 2021
@@ -719,6 +722,32 @@ public Accessible call() throws Exception {
}, c);
}

// This method is called from the native
// Each child takes up three entries in the array: one for itself, one for its role, and one for the recursion level
public static Object[] getChildrenAndRolesRecursive(final Accessible a, final Component c, final int whichChildren, final boolean allowIgnored, final int level) {
Copy link
Member

@azuev-java azuev-java Jun 8, 2021

I am not sure but i think CAccessible is considered a public API so adding public methods to it might require a CSR filling.

Copy link
Contributor Author

@savoptik savoptik Jul 21, 2021

Done

@savoptik savoptik changed the title Jdk 8152350 6 8267385: Create NSAccessibilityElement implementation for JavaComponentAccessibility Jun 10, 2021
@openjdk openjdk bot added the rfr label Jun 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 10, 2021

@azuev-java
Copy link
Member

@azuev-java azuev-java commented Jun 10, 2021

/issue add JDK-8262031

@openjdk
Copy link

@openjdk openjdk bot commented Jun 10, 2021

@azuev-java Only the author (@savoptik) is allowed to issue the /issue command.

@savoptik
Copy link
Contributor Author

@savoptik savoptik commented Jun 10, 2021

/issue add JDK-8262031, JDK-8264287, JDK-8264303, JDK-8264292, JDK-8267387, JDK-8267388

@openjdk
Copy link

@openjdk openjdk bot commented Jun 10, 2021

@savoptik
Adding additional issue to issue list: 8262031: Create implementation for NSAccessibilityNavigableStaticText protocol.

Adding additional issue to issue list: 8264287: Create implementation for NSAccessibilityComboBox protocol peer.

Adding additional issue to issue list: 8264303: Create implementation for NSAccessibilityTabGroup protocol peer.

Adding additional issue to issue list: 8264292: Create implementation for NSAccessibilityList protocol peer.

Adding additional issue to issue list: 8267387: Create implementation for NSAccessibilityOutline protocol.

Adding additional issue to issue list: 8267388: Create implementation for NSAccessibilityTable protocol.

@victordyakov
Copy link

@victordyakov victordyakov commented Jun 10, 2021

@pbansal please review

@savoptik
Copy link
Contributor Author

@savoptik savoptik commented Jun 10, 2021

/issue add JDK-8264286, JDK-8264298

@openjdk
Copy link

@openjdk openjdk bot commented Jun 10, 2021

@savoptik
Adding additional issue to issue list: 8264286: Create implementation for NSAccessibilityColumn protocol peer.

Adding additional issue to issue list: 8264298: Create implementation for NSAccessibilityRow protocol peer.

newChild = [TableAccessibility alloc];
if ([javaRole isEqualToString:@"pagetablist"]) {
newChild = [TabGroupLegacyAccessibility alloc];
} else if ([javaRole isEqualToString:@"table"]) {
Copy link
Member

@azuev-java azuev-java Jun 16, 2021

Do we even need legacy peer implementation in the new classes?

Copy link
Contributor Author

@savoptik savoptik Jun 29, 2021

The new TabGroup is also named TabGroupAccessibility, so as not to get out of the naming, the old class is renamed for this, otherwise the building will fail.

But when are these Legacy classes used? The "pagetablist" and "table" roles are being handled in commonComponentAccessibility and new implementation classes will be used there. Why can't we just remove these legacy classes and any references to them, instead of renaming them

Copy link
Contributor Author

@savoptik savoptik Jul 15, 2021

By agreement, it was decided that we do not remove the legacy code yet until at least one release has passed to make sure there are no regressions.
The legacy code can then be removed with a separate commit without any difficulty.

@savoptik
Copy link
Contributor Author

@savoptik savoptik commented Jun 16, 2021

/issue add JDK-8264291

@openjdk
Copy link

@openjdk openjdk bot commented Jun 16, 2021

@savoptik
Adding additional issue to issue list: 8264291: Create implementation for NSAccessibilityCell protocol peer.

@victordyakov
Copy link

@victordyakov victordyakov commented Jun 16, 2021

@pbansal The initial change - to move functionality from JavaComponentAccessibility to CommonComponentAccessibility requires touching up of almost all (if not all) of the new files in this project - this is why too much change being done in one PR as a reason why has been decided to push all the components in one PR go instead of doing things component by component like we did in the past. We just need to spend much more time on this review and the good thing is that this Change will be baked in 18 EA enough time versus 17 which is already on stabilization phase.

BOOL value = (*env)->CallStaticBooleanMethod(env, sjc_CAccessibility, jm_isEnabled, fAccessible, fComponent);
CHECK_EXCEPTION();
if (!value) {
NSLog(@"WARNING: %s called on component that has no accessible component: %@", __FUNCTION__, self);

I see this warning being thrown when trying to use the disabled slider. If we run SwingSet2 and go to Slider demo. Try to navigate to disabled sliders using the VO keys. This warning is thrown when the VO focus is shifted to disabled sliders. I don't see similar warning before this change. This is the warning I see

" WARNING: -[CommonComponentAccessibility isAccessibilityEnabled] called on component that has no accessible component: <SliderAccessibility: 0x7fc938147c00>"

Copy link
Contributor Author

@savoptik savoptik Jul 21, 2021

Done

@azuev-java
Copy link
Member

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

During testing of text input i have found that the previous implementation with voiceover active pronounced every entered character and then, whet typing pauses, it pronounced the last entered word. With new implementation i only hear the first typed character in the text field and then nothing - no other characters and no last entered word. Looks like we are not sending some notification to the system when text is updated. Needs further investigation.

savoptik added 3 commits Jul 13, 2021
…s, as well as a11y components nested in list rows, disappeared;

2. Added checks for exceptions;
3. corrected descriptions of tests;
4. Fixed a problem with the pronunciation of the edited text.
5. Fixed a problem due to which the line number in the table was not pronounced correctly.
…ese methods to call invokeAndWait only once per callback from the native, as it usually was done before?
@azuev-java
Copy link
Member

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

Ok, remaining issues for the table implementation:

  1. Debug information "WARNING: -[CommonComponentAccessibility isAccessibilityEnabled] called on component that has no accessible component..." printed whenever navigated to the cell that has image inside it - i think this as well as the same message when navigating with voiceover shortcuts to the disabled components - needs to be addressed in this pull request
  2. No matter what selection mode is set on the table - cell selection, row selection or column selection - when selection changes due to the table navigation the content of the row where cursor currently placed is announced. Since in previous implementation we had only "Selection changes: n elements out of nnn elements are selected" announcement and in individual cell selection mode there were no announcements at all i consider it as not a functional regression but still we need to create a bug for improving this behavior and it should be addressed as soon as possible and backported to all releases where this implementation will go.
  3. Indexes when announcing the content are not right - when full cell announcement happens it always says column 1 row 1 - no matter what cell is being announced. I think that needs to be fixed.
    Here is the image that shows the invalid announcement: text says java, SwingSet2, window, table, table Row 7 of 46 Mark, column 1, row 1 Davidson, column 1, row 1 Dark Green, column 1, row 1 - and so on. Each cell is being announced as column 1 row 1.
    image

savoptik added 4 commits Jul 20, 2021
2. Fixed a potential problem with memory in table cells;
3. Fixed a problem with the sounding of cell coordinates.
…, but it will be done before by the CHECK_EXCEPTION. The macro also will log it when necessary.

2. What happens if "fAccessible" and "fComponent" will be null due to OOM, should we rethrow this exception, or ignoring it is fine?
2. I suggest doing the opposite, leave the CHECK_EXCEPTION and remove the chech and the logging. The CHECK_EXCEPTION is better since it can be configured via properties, and it will chech itself on what thread the current code is executed.
Copy link
Member

@azuev-java azuev-java left a comment

There are still some issues - namely, accessibility cursor got stuck inside combobox popup after popup is closed and voiceover shortcuts such as control option space announced in combobox but do nothing - but they can be fixed in separate bugs. Overall the new API is working and the functional difference can be overcome later in separate changes.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 3, 2021

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

8267385: Create NSAccessibilityElement implementation for JavaComponentAccessibility
8262031: Create implementation for NSAccessibilityNavigableStaticText protocol
8264287: Create implementation for NSAccessibilityComboBox protocol peer
8264303: Create implementation for NSAccessibilityTabGroup protocol peer
8264292: Create implementation for NSAccessibilityList protocol peer
8267387: Create implementation for NSAccessibilityOutline protocol
8267388: Create implementation for NSAccessibilityTable protocol
8264286: Create implementation for NSAccessibilityColumn protocol peer
8264298: Create implementation for NSAccessibilityRow protocol peer
8264291: Create implementation for NSAccessibilityCell protocol peer

Reviewed-by: kizune, pbansal, serb

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

  • 0ac2be9: 8272123: Problem list 4 jtreg tests which regularly fail on macos-aarch64
  • 272fcb4: 8272113: Build compare script fails with differences in classlist
  • 2f7a469: 8271931: Make AbortVMOnVMOperationTimeout more resilient to OS scheduling
  • a86ac0d: 8271939: Clean up primitive raw accessors in oopDesc
  • b84a9c7: 8271956: AArch64: C1 build failed after JDK-8270947
  • 38ff85c: 8271461: CompileCommand support for hidden class methods
  • c495ede: 8272099: mark hotspot runtime/Monitor tests which ignore external VM flags
  • e882087: 8271904: mark hotspot runtime/ClassFile tests which ignore external VM flags
  • fa36e33: 8271513: support JavaThreadIteratorWithHandle replacement by new ThreadsList::Iterator
  • cc61520: 8270842: G1: Only young regions need to redirty outside references in remset.
  • ... and 815 more: https://git.openjdk.java.net/jdk/compare/2717fcb1345379d9856a33148d548eccb7b708f4...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 (@azuev-java, @pankaj-bansal, @mrserb) 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 label Aug 3, 2021
@victordyakov
Copy link

@victordyakov victordyakov commented Aug 3, 2021

@pankaj-bansal please review the latest revision

Copy link

@pankaj-bansal pankaj-bansal left a comment

I tested the latest version and all the problems I had mentioned earlier are no longer present. I also used few components like Combobox, Sliders, Buttons etc and things look good. As Alex mentioned, if there are small issues present, they can be fixed later in follow up bugs.

mrserb
mrserb approved these changes Aug 6, 2021
@savoptik
Copy link
Contributor Author

@savoptik savoptik commented Aug 9, 2021

/integrate

@forantar
Copy link
Contributor

@forantar forantar commented Aug 9, 2021

/sponsor

@openjdk openjdk bot added the sponsor label Aug 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 9, 2021

@savoptik
Your change (at version a13d6a6) is now ready to be sponsored by a Committer.

@forantar
Copy link
Contributor

@forantar forantar commented Aug 9, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Aug 9, 2021

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

  • 0ac2be9: 8272123: Problem list 4 jtreg tests which regularly fail on macos-aarch64
  • 272fcb4: 8272113: Build compare script fails with differences in classlist
  • 2f7a469: 8271931: Make AbortVMOnVMOperationTimeout more resilient to OS scheduling
  • a86ac0d: 8271939: Clean up primitive raw accessors in oopDesc
  • b84a9c7: 8271956: AArch64: C1 build failed after JDK-8270947
  • 38ff85c: 8271461: CompileCommand support for hidden class methods
  • c495ede: 8272099: mark hotspot runtime/Monitor tests which ignore external VM flags
  • e882087: 8271904: mark hotspot runtime/ClassFile tests which ignore external VM flags
  • fa36e33: 8271513: support JavaThreadIteratorWithHandle replacement by new ThreadsList::Iterator
  • cc61520: 8270842: G1: Only young regions need to redirty outside references in remset.
  • ... and 815 more: https://git.openjdk.java.net/jdk/compare/2717fcb1345379d9856a33148d548eccb7b708f4...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Aug 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 9, 2021

@forantar @savoptik Pushed as commit 9c6457f.

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

@openjdk
Copy link

@openjdk openjdk bot commented Aug 9, 2021

@forantar The command sponsor can only be used in open pull requests.

@forantar
Copy link
Contributor

@forantar forantar commented Aug 9, 2021

Thanks all for the thorough review and deep testing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants