On Thu, 6 Apr 2023 20:43:13 GMT, Alexander Zuev <kiz...@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
