8313556: Create implementation of NSAccessibilitySlider protocol #1226

azuev-java · 2023-08-29T10:10:54Z

Create implementation for Slider and Stepper accessibility protocols.
Fix mapping.
Fix performAction parameter type in declaration.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8313556: Create implementation of NSAccessibilitySlider protocol (Enhancement - P4)
JDK-8313558: Create implementation of NSAccessibilityStepper protocol (Enhancement - P4)

Reviewers

Ambarish Rapte (@arapte - Reviewer)
Andy Goryachev (@andy-goryachev-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1226/head:pull/1226
$ git checkout pull/1226

Update a local copy of the PR:
$ git checkout pull/1226
$ git pull https://git.openjdk.org/jfx.git pull/1226/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1226

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1226.diff

Using Webrev

Link to Webrev Comment

Create implementation for Slider and Stepper accessibility protocols. Fix mapping. Fix performAction parameter type in declaration.

bridgekeeper · 2023-08-29T10:11:43Z

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

mlbridge · 2023-08-29T10:17:35Z

Webrevs

azuev-java · 2023-08-29T10:26:54Z

/issue add JDK-8313558

openjdk · 2023-08-29T10:27:31Z

@azuev-java
Adding additional issue to issue list: 8313558: Create implementation of NSAccessibilityStepper protocol.

kevinrushforth · 2023-08-29T12:11:32Z

Reviewers: @arapte @andy-goryachev-oracle

kevinrushforth · 2023-08-29T12:21:34Z

/reviewers 2

openjdk · 2023-08-29T12:22:55Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

andy-goryachev-oracle · 2023-08-29T14:59:41Z

should there be an automated test for the added functionality?

kevinrushforth · 2023-08-29T15:18:20Z

should there be an automated test for the added functionality?

Automated testing of the screen reader is not feasible. Even if it were, it would be a separate enhancement to add accessibility tests. Btw, this PR isn't providing new functionality. It is providing a new implementation of existing functionality.

Having said that, it would be helpful for Alex to list the controls that will use the new implementation, so reviewers can test it manually. For this PR, I would imagine that the Slider and Spinner controls would be used to test the NSAccessibilitySlider and NSAccessibilityStepper protocols, respectively. Perhaps there are also other ways to hit the new code?

andy-goryachev-oracle · 2023-08-29T15:22:29Z

Thank you for clarification, Kevin.
The two linked tickets, and the associated umbrella tickets talk about providing a parallel implementation of the a*y feature, meaning both old and new interfaces will be available for some time, do I read it correctly? If so, how do we ensure that the new implementation is being used by the OS?

kevinrushforth · 2023-08-29T15:40:52Z

Yes, you read it correctly. Good question about how you can tell that it is going through the new implementation. Since it is supposed to transparently do the same thing, the only way I can think of is to instrument the code to add debug logging statement (which is what I did when reviewing the initial changes).

andy-goryachev-oracle · 2023-08-29T15:55:13Z

perhaps the logging statements should be added (commented out) as a part of this PR.

azuev-java · 2023-08-29T19:42:50Z

perhaps the logging statements should be added (commented out) as a part of this PR.

I can do that - i usually refrained from doing so because code would look not very clean but basically the only way to tell that it goes trough the new code should be debug output - if behavior changes it should be considered as a potential bug (not always because if i see that the old behavior is incorrect i would not go out of my way to recreate the obviously flawed logic) but in general the idea is that new code should behave exactly the same as the old one.

andy-goryachev-oracle · 2023-08-29T19:45:32Z

what I meant is that commented out logging statements should be left in the code, so the reviewers can uncomment them and test. like this:

// System.err.println("it works");

azuev-java · 2023-08-29T19:48:02Z

Having said that, it would be helpful for Alex to list the controls that will use the new implementation, so reviewers can test it manually.

I will definitely do that in the future. In this case you are right the controls are Spinner and Slider and while we have a dedicated control group for Spinner component in Ensemble there is no such thing for Slider (or i haven't found it) so the best way to test it is to go to the ColorPicker component test, invoke color chooser and click "Custom Color..." link - that will invoke the color selection dialog that has a lot of sliders to play with.

azuev-java · 2023-08-29T20:13:16Z

what I meant is that commented out logging statements should be left in the code, so the reviewers can uncomment them and test. like this:
// System.err.println("it works");

Did that. I would just appreciate if you or any other reviewer reminds me to remove these statements after we are done with the review :) The code should print out when slider or stepper values are being changed using the a11y shortcuts to manipulate controls. That will ensure that when being accessed trough the a11y handle the component state can be appropriately updated. And of course it will indicate that the new component is invoked.

arapte · 2023-08-30T14:21:48Z

I observed few warnings and behavioral differences.

Warnings:

modules/javafx.graphics/src/main/native-glass/mac/a11y/JFXSliderAccessibility.m:32:17: warning: method 'accessibilityFrame' in protocol 'NSAccessibilityElement' not implemented [-Wprotocol]
modules/javafx.graphics/src/main/native-glass/mac/a11y/JFXSliderAccessibility.m:32:17: warning: method 'accessibilityParent' in protocol 'NSAccessibilityElement' not implemented [-Wprotocol]
modules/javafx.graphics/src/main/native-glass/mac/a11y/JFXStepperAccessibility.m:32:17: warning: method 'accessibilityFrame' in protocol 'NSAccessibilityElement' not implemented [-Wprotocol]
modules/javafx.graphics/src/main/native-glass/mac/a11y/JFXStepperAccessibility.m:32:17: warning: method 'accessibilityParent' in protocol 'NSAccessibilityElement' not implemented [-Wprotocol]
modules/javafx.graphics/src/main/native-glass/mac/a11y/AccessibleBase.m:247:21: warning: 'NSObject' may not respond to 'clearParent'

Behavior differences:

Spinner: Changed values are not read by VoiceOver when we increase or decrease the value using arrow keys or using Ctrl + Option + Up/Down keys ( Tested with Spinner app in Ensemble )
Slider: Values are read without % as suffix ( Tested with Styled tool bar app in Ensemble )

andy-goryachev-oracle · 2023-08-31T15:26:25Z

Testing with macOS 13.3.1a and Voice Over, using MonkeyTester
https://github.com/andy-goryachev-oracle/MonkeyTest

Observed a number of things, some possibly not related to this PR:

had color picker custom color dialog opened when turning on VoiceOver, saw stderr output:
*** java.lang.instrument ASSERTION FAILED ***: "!errorOutstanding" with message transform method call failed at open/src/java.instrument/share/native/libinstrument/JPLISAgent.c line: 873
color picker, custom color: hue slider is not accessible
color picker, slider (opacity):

shows 46 (%), announces: 46.3
no NSLog debug output if using left/right arrow
when entering slider via shift-ctrl-option-down arrow, does not announce how to control the slider (as it did with spinner).
when controlling the spinner with ctrl-option-up/down, I see NSLog debug output:

2023-08-31 08:09:03.087 java[45918:3234828] Slider decrement action invoked
2023-08-31 08:09:04.565 java[45918:3234828] Slider increment action invoked

spinner: see debug output when using ctrl-option-up/down, but change in value is not announced
2023-08-31 08:05:12.267 java[45918:3234828] Stepper decrement action invoked
2023-08-31 08:05:13.192 java[45918:3234828] Stepper increment action invoked
spinner: when in spinner (via shift-ctrl-option-down), trying to control the spinner via ctrl-option-up/down, releasing the last button moves the focus to the menu instead of staying in the spinner.

bridgekeeper · 2023-09-28T19:57:32Z

@azuev-java This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2023-10-26T22:45:44Z

@azuev-java This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

- Removed some debug output generation

Merge

arapte

LGTM... Tested the behavior of both controls, observed no problem.
Providing a minor query, If you choose to make the change I shall re-approve.

arapte · 2025-02-28T11:43:39Z

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

+- (id)accessibilityTitle
+{
+    jobject jresult = NULL;
+    GET_MAIN_JENV;
+    if (env == NULL) return NULL;
+    jresult = (jobject)(*env)->CallLongMethod(env, self->jAccessible,
+                                              jAccessibilityAttributeValue, (jlong)@"AXTitle");
+    GLASS_CHECK_EXCEPTION(env);
+    return variantToID(env, jresult);
+}


This method is same as accessibilityLabel. Should we change implementation to call one method from the other ?

Makes sense.

andy-goryachev-oracle · 2025-02-28T23:41:18Z

With Slider, it announces the current value as "percent" (of the working envelope?), even though the min/max can be arbitrary. Is this intentional?

Also, for Spinner, it says "Stepper", is this expected?

macOS 15.3.1 M1

andy-goryachev-oracle · 2025-03-03T16:26:54Z

Is this PR related to https://git.openjdk.org/jdk/pull/23841 in any way?

azuev-java · 2025-03-03T16:54:08Z

With Slider, it announces the current value as "percent" (of the working envelope?), even though the min/max can be arbitrary. Is this intentional?

Also, for Spinner, it says "Stepper", is this expected?

macOS 15.3.1 M1

I need to check that.

azuev-java · 2025-03-03T16:55:24Z

Is this PR related to https://git.openjdk.org/jdk/pull/23841 in any way?

Not directly. The event delivery in AWT/Swing and in JavaFX works quite differently so i will check if the new implementation here is affected by the same problem - but i doubt so.

azuev-java · 2025-03-05T16:47:06Z

Also, for Spinner, it says "Stepper", is this expected?

On macOS the control we call Spinner is called Stepper, there's no Spinner.

From macOS UI guide: "A stepper is a two-segment control that people use to increase or decrease an incremental value."

andy-goryachev-oracle · 2025-03-05T16:48:22Z

Spinner is called Stepper

yep, that's what I suspected. thanks!

so the remaining question is whether it should announce "percent" of the slider position or the actual value?

…bilityTitle and add coment explaining why.

azuev-java · 2025-03-05T17:17:52Z

so the remaining question is whether it should announce "percent" of the slider position or the actual value?

Unfortunately we do not have any control on that. There is no way to tell VoiceOver if it is percent or the raw value - VO deducts it based on its own logic. The only way to customize the VO heuristics is to override accessibilityValueDescription function and return string for VO but then we would need to decode for ourselves how do we convert the value plus it will be our responsibility to do localization of this string if needed.

andy-goryachev-oracle

yep, you are right: we don't want to deal with interpreting and l10n.

azuev-java · 2025-03-06T01:46:47Z

/integrate

openjdk · 2025-03-06T01:47:08Z

@azuev-java
Your change (at version 1ae393a) is now ready to be sponsored by a Committer.

arapte · 2025-03-06T13:32:18Z

/sponsor

openjdk · 2025-03-06T13:32:52Z

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

0a20fef: 8327478: Add System test to verify TextSelection issue for webkit-617.1
fc770fb: 8349091: Charts: exception initializing in a background thread
1824db5: 8299753: Tree/TableView: Column Resizing With Fractional Scale
f3ba448: 8233179: VetoableListDecorator#sort throws IllegalArgumentException "duplicate children"

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-06T13:33:00Z

@arapte @azuev-java Pushed as commit c7091e1.

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

8313556: Create implementation of NSAccessibilitySlider protocol

3ebfc83

Create implementation for Slider and Stepper accessibility protocols. Fix mapping. Fix performAction parameter type in declaration.

openjdk bot changed the title ~~8313556: Create implementation of NSAccessibilitySlider protocol~~ 8313556: Create implementation of NSAccessibilitySlider protocol Aug 29, 2023

openjdk bot added the rfr Ready for review label Aug 29, 2023

kevinrushforth requested a review from arapte August 29, 2023 12:11

Add commented out debug statements.

ceb83cd

Fixing warnings during build

5913517

Merge remote-tracking branch 'origin/master' into JDK-8313556

ebdeb53

bridgekeeper bot closed this Oct 26, 2023

azuev-java and others added 2 commits November 15, 2023 12:50

- Added accessibilityTitle method

d890b2c

- Removed some debug output generation

Merge pull request #7 from openjdk/master

b35e693

Merge

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Feb 26, 2025

azuev-java added 3 commits February 25, 2025 23:45

Merge pull request #13 from openjdk/master

3e28d44

Merge

Merge branch 'master' into JDK-8313556

c122868

Add accessibilityTitleUIElement function to the base class.

09f6809

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Feb 26, 2025

arapte approved these changes Feb 28, 2025

View reviewed changes

Remove duplicate code make accessibilityLabel explicitly call accessi…

1ae393a

…bilityTitle and add coment explaining why.

andy-goryachev-oracle approved these changes Mar 5, 2025

View reviewed changes

azuev-java requested a review from arapte March 5, 2025 17:51

arapte approved these changes Mar 5, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Mar 5, 2025

openjdk bot added the sponsor Ready to sponsor label Mar 6, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 6, 2025

openjdk bot closed this Mar 6, 2025

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Mar 6, 2025

azuev-java deleted the JDK-8313556 branch May 30, 2025 21:02

8313556: Create implementation of NSAccessibilitySlider protocol #1226

8313556: Create implementation of NSAccessibilitySlider protocol #1226

Uh oh!

Conversation

azuev-java commented Aug 29, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Aug 29, 2023

Uh oh!

mlbridge bot commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

azuev-java commented Aug 29, 2023

Uh oh!

openjdk bot commented Aug 29, 2023

Uh oh!

kevinrushforth commented Aug 29, 2023

Uh oh!

kevinrushforth commented Aug 29, 2023

Uh oh!

openjdk bot commented Aug 29, 2023

Uh oh!

andy-goryachev-oracle commented Aug 29, 2023

Uh oh!

kevinrushforth commented Aug 29, 2023

Uh oh!

andy-goryachev-oracle commented Aug 29, 2023

Uh oh!

kevinrushforth commented Aug 29, 2023

Uh oh!

andy-goryachev-oracle commented Aug 29, 2023

Uh oh!

azuev-java commented Aug 29, 2023

Uh oh!

andy-goryachev-oracle commented Aug 29, 2023

Uh oh!

azuev-java commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azuev-java commented Aug 29, 2023

Uh oh!

arapte commented Aug 30, 2023

Uh oh!

andy-goryachev-oracle commented Aug 31, 2023

Uh oh!

bridgekeeper bot commented Sep 28, 2023

Uh oh!

bridgekeeper bot commented Oct 26, 2023

Uh oh!

arapte left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arapte Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

azuev-java Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Feb 28, 2025

Uh oh!

andy-goryachev-oracle commented Mar 3, 2025

Uh oh!

azuev-java commented Mar 3, 2025

Uh oh!

azuev-java commented Mar 3, 2025

Uh oh!

azuev-java commented Mar 5, 2025

Uh oh!

andy-goryachev-oracle commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azuev-java commented Mar 5, 2025

Uh oh!

andy-goryachev-oracle left a comment

azuev-java commented Aug 29, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 29, 2023 •

edited

Loading

azuev-java commented Aug 29, 2023 •

edited

Loading

arapte left a comment •

edited

Loading

andy-goryachev-oracle commented Mar 5, 2025 •

edited

Loading