RFR: 8301312: Create implementation of NSAccessibilityButton protocol

[Kevin Rushforth]

On Thu, 6 Apr 2023 20:43:13 GMT, Alexander Zuev <kizune at openjdk.org> wrote:

> Add the common base component for all the new implementing native classes Change native peer creation to use the new base component The new code will instantiate new protocol implementation  for the given role if it exists or an old one if it does not exist
> Added BUTTON role implementing class

Initial testing looks good. I'd like to do some additional testing.

I like the approach you chose, since it allows for incremental adoption of the new a11y API. I presume that you will file an RFE to cleanup the deprecated GlassAccessible class once new a11y classes are implemented for all components?

I have a question about the scope of this RFE. The JBS issue says that it will use NSAccessiblityButton protocol for `BUTTON`, `INCREMENT_BUTTON`, `DECREMENT_BUTTON`, and `SPLIT_MENU_BUTTON`, but it looks like only the first is implemented. Will that be handled later? If so, then the Description in JBS should be updated.

I also left a few inline comments. Nothing major.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacAccessible.java line 658:

> 656: 
> 657:     MacAccessible() {
> 658:         this.peer = _createGlassAccessible();

Now that you defer the creation of the `peer`, it might be safer to change line 798 to call `getNativeAccessible()` instead of accessing `peer` directly, in case `sendNotification()` is called before `getNativeAccessible()`, unless you know that this can't happen.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacAccessible.java line 807:

> 805:         if (this.peer == 0L) {
> 806:             AccessibleRole role = (AccessibleRole) getAttribute(ROLE);
> 807:             if (role == null) role = AccessibleRole.NODE;

When would `role` be null? And if it is, is defaulting to `NODE` going to do the right thing once you implement the new a11y protocol for Node?

modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacAccessible.java line 976:

> 974:         return -1;
> 975:     }
> 976: 

This method is not used anywhere (even from native code).

modules/javafx.graphics/src/main/native-glass/mac/a11y/AccessibleBase.h line 42:

> 40: @end
> 41: 
> 42: jmethodID jAccessibilityAttributeNames;

Very minor: missing newline at end of file

modules/javafx.graphics/src/main/native-glass/mac/a11y/AccessibleBase.m line 120:

> 118:     (*env)->CallVoidMethod(env, self->jAccessible, jAccessibilityPerformAction, actionId);
> 119:     GLASS_CHECK_EXCEPTION(env);
> 120:     return TRUE;

Are there any cases where an exception is possible? If so, then might checking the exception and returning false if there is one be warranted?

modules/javafx.graphics/src/main/native-glass/mac/a11y/AccessibleBase.m line 156:

> 154:     NSString *roleName = jStringToNSString(env, forRole);
> 155:     Class classType = [AccessibleBase getComponentAccessibilityClass:roleName];
> 156:     GlassAccessible* accessible = NULL;

It might be clearer to use `NSObject` here (which is the common supertype of `GlassAccessible` and `AccessibileBase`).

modules/javafx.graphics/src/main/native-glass/mac/a11y/ButtonAccessibility.h line 37:

> 35: - (NSRect)accessibilityFrame;
> 36: @end
> 37: 

Very minor: extra newline at the end of file.

-------------

PR Review: https://git.openjdk.org/jfx/pull/1084#pullrequestreview-1376768631
PR Review Comment: https://git.openjdk.org/jfx/pull/1084#discussion_r1161117725
PR Review Comment: https://git.openjdk.org/jfx/pull/1084#discussion_r1161117486
PR Review Comment: https://git.openjdk.org/jfx/pull/1084#discussion_r1161117615
PR Review Comment: https://git.openjdk.org/jfx/pull/1084#discussion_r1161117799
PR Review Comment: https://git.openjdk.org/jfx/pull/1084#discussion_r1161118971
PR Review Comment: https://git.openjdk.org/jfx/pull/1084#discussion_r1161119661
PR Review Comment: https://git.openjdk.org/jfx/pull/1084#discussion_r1161121358