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

8256108: Create implementation for NSAccessibilityElement protocol peer #1290

Closed
wants to merge 5 commits into from

Conversation

@azuev-java
Copy link
Member

@azuev-java azuev-java commented Nov 18, 2020


Progress

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

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8256108: Create implementation for NSAccessibilityElement protocol peer

Reviewers

Download

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

azuev-java and others added 4 commits Oct 19, 2020
Merge changes from master
Initial implementation with two functions required for any component
accessibility peer implementation. All the new accessibility protocol
implementations should derive from this protocol.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 18, 2020

👋 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 Nov 18, 2020

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 18, 2020

Webrevs

Added comments on a reason for the common component layer existance.
// component accessibility peers.
// Please see https://developer.apple.com/documentation/appkit/nsaccessibilityprotocol
// for more details.
@interface CommonComponentAccessibility : JavaComponentAccessibility <NSAccessibilityElement> {
Copy link
Member

@mrserb mrserb Nov 19, 2020

Choose a reason for hiding this comment

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

I remember we had a plan to implement this step by step, replacing the parts of the old API with some new parts.

If the plan still in place then why we do not implement NSAccessibilityElement in the JavaComponentAccessibility class, so the usage of current (old) methods like accessibilityPositionAttribute/accessibilitySizeAttribute/accessibilityFocusedAttribute could be replaced by the new methods from the NSAccessibilityElement protocol like accessibilityParent/accessibilityFrame/isAccessibilityFocused.

If we plan to implement it from scratch then why the CommonComponentAccessibility extends JavaComponentAccessibility?

It is not obvious from the change since the accessibilityFrame() is implemented from scratch mostly duplicate accessibilityPositionAttribute+accessibilitySizeAttribute, but the accessibilityParent() uses the code from the JavaComponentAccessibility

Copy link
Member Author

@azuev-java azuev-java Nov 19, 2020

Choose a reason for hiding this comment

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

Implementing it step by step is exactly why i did it - if i make JavaComponentAccessibility implementing the NSAccessibilityElement it triggers mac os to stop using the old property based API on all of it and we are not ready for that. I'm planning to keep CommonComponentAccessibility implementation minimal so once all of our bases are covered we can just swap JavaComponentAccessibility to implementing NSAccessibilityElement and switching all the subcomponent's peers to extend it and then we will be able to drop CommonComponentAccessibility altogether (or just leave it so we can swap to older API easier for comparison testing).

Copy link
Member

@mrserb mrserb Nov 19, 2020

Choose a reason for hiding this comment

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

Then it does not look like as step by step implementation, it just adding the code which does not necessarily work properly. Are you sure that the implementation of NSAccessibilityElement will block the old for all roles not only for the elemental role? It will be useful if we start to use the new accessibilityFrame/etc API instead of accessibilityPositionAttribute/etc attributes, but still, use the old attributes for other roles.

If the plan is to make the CommonComponentAccessibility as small as possible then the CommonComponentAccessibility.accessibilityFrame could be implemented on top of accessibilityPositionAttribute+accessibilitySizeAttribute or another way around, similar to accessibilityParent which is implemented via accessibilityParentAttribute. But in this case, the whole class will have 5 rows which are never executed? Probably it is possible to mix old/new?

Note

The new, method-based accessibility API is backward compatible with the previous, key-based API. This means, if you have controls that are already accessible, you can continue to use them. You can even convert a control to the method-based API in stages, implementing the new method-based API for some features, leaving others in the key-based API.

If there’s a conflict between the method- and key-based APIs, the method-based API takes precedence. This is particularly important for frameworks that provides accessible controls. If you convert those controls to the method-based API, the system ignores any legacy code that modifies their attributes. For this reason, if you create third-party libraries, you may want to wait to upgrade your library until you’re sure that all your clients have updated their apps.
https://developer.apple.com/library/archive/documentation/Accessibility/Conceptual/AccessibilityMacOSX/

Copy link
Member Author

@azuev-java azuev-java Nov 19, 2020

Choose a reason for hiding this comment

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

It is step by step implementation since the component peers will inherit these functions and they will be called once we initialize these peers. It will be clear once you see simple peer like pushbutton peer implementation. And yes - i tested - once system detects that peer implement new protocol it stops calling the property retrieving methods giving out "unrecognized selector sent to instance" errors for not implemented functions.

Copy link
Member

@mrserb mrserb Nov 19, 2020

Choose a reason for hiding this comment

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

So do you plan to return different a11y components depends on the type of the role? So for the old API, you will return JavaComponentAccessibility and for the new, you will return ButtonAccessibility which will extend CommonComponentAccessibility and implement NSAccessibilityButton, etc? WIll it work?

BTW don't you need to implement isAccessibilityFocused to cover old accessibilityFocusedAttribute?

Copy link
Member Author

@azuev-java azuev-java Nov 19, 2020

Choose a reason for hiding this comment

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

So do you plan to return different a11y components depends on the type of the role? So for the old API, you will return JavaComponentAccessibility and for the new, you will return ButtonAccessibility which will extend CommonComponentAccessibility and implement NSAccessibilityButton, etc? WIll it work?

Yes. That's the way. I mean - that's the plan. I already tested it on couple of components and it works just fine.

BTW don't you need to implement isAccessibilityFocused to cover old accessibilityFocusedAttribute?

Not right now. I'm trying to do the minimal implementation and so far in my testing i haven't found that anyone calls that selector under any circumstances. OTOH the current implementation is in no way a complete one and selectors will be added during the component peers implementation phase. I'm trying to reuse as much code from the original class as possible but for streamlining purposes if there's no corresponding function i can use i am writing this function as a new one, because i do not want to modify the behavior of the existing functions. This should streamline the last merge in this feature which should be a general cleanup of the existing code and removal of unused functions and refactoring of the used ones to the new API.

Copy link
Member

@mrserb mrserb Nov 19, 2020

Choose a reason for hiding this comment

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

Ok, let's see how it will work.

mrserb
mrserb approved these changes Nov 19, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 19, 2020

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

8256108: Create implementation for NSAccessibilityElement protocol peer

Reviewed-by: 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 94 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 Nov 19, 2020
@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Nov 20, 2020

/integrate

@openjdk openjdk bot closed this Nov 20, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 20, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 20, 2020

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

  • 6813889: 8251317: Support for CLDR version 38
  • c816464: 4916923: In MetalRootPaneUI, MetalRootLayout does not correctly calculate minimumsize
  • fae68ff: 8256640: assert(!m->is_old() || ik()->is_being_redefined()) failed: old methods should not be in vtable
  • c140773: 8256692: Zero: remove obsolete block from ZeroInterpreter::native_entry
  • 080c707: 8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly
  • b9db002: 8256682: JDK-8202343 is incomplete
  • b8244b6: 8236926: Concurrently uncommit memory in G1
  • defdd12: 8142984: Zero: fast accessors should handle both getters and setters
  • 1718aba: 8227400: Adjust jib profiles to make 3rd party tools for creating installers available on Mach5 test machines
  • 9bb8223: 8253299: Manifest bytes are read twice when verifying a signed JAR
  • ... and 90 more: https://git.openjdk.java.net/jdk/compare/41139e31c02d78b359d4c562a4ace957b339b612...master

Your commit was automatically rebased without conflicts.

Pushed as commit 4c09525.

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