8361379: [macos] Refactor accessibility code to retrieve attribute by name #1840

azuev-java · 2025-07-03T22:17:01Z

Copyright year update;
Introduce new function requestNodeAttribute and refactor code to use it;
Fix some typos;
Enable new code to handle TabPages since TabGroup was implemented;

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)

Issue

JDK-8361379: [macos] Refactor accessibility code to retrieve attribute by name (Enhancement - P3)

Reviewers

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1840

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

Using diff file

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

Using Webrev

Link to Webrev Comment

… name - Copyright year update - Introduce new function requestNodeAttribute and refactor code to use it - Fix some typos - Enable new code to handle TabPages since TabGroup was implemented

bridgekeeper · 2025-07-03T22:17:37Z

👋 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 · 2025-07-03T22:18:34Z

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

8361379: [macos] Refactor accessibility code to retrieve attribute by name

Reviewed-by: angorya, arapte

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

99866ae: 8141391: Manual JFXPanel DnD test doesn't work
1a2a50b: 8362095: HeaderButtonMetrics should not be used across toolkit boundary
a574d92: 8359598: [TestBug] VirtualFlowTestUtils should not create a temporary Stage
... and 7 more: https://git.openjdk.org/jfx/compare/0270847b85ab6d4701abb201ec4b2aeee96129dd...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mlbridge · 2025-07-03T22:21:52Z

Webrevs

victordyakov · 2025-07-08T18:02:55Z

@andy-goryachev-oracle @arapte please review

andy-goryachev-oracle · 2025-07-08T18:14:36Z

/reviewers 2

openjdk · 2025-07-08T18:15:30Z

@andy-goryachev-oracle
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 · 2025-07-08T21:29:31Z

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

    [rolesMap setObject:@"JFXRadiobuttonAccessibility" forKey:@"RADIO_BUTTON"];
-//  Requires TAB_GROUP to be implemented first
-//  [rolesMap setObject:@"JFXRadiobuttonAccessibility" forKey:@"TAB_ITEM"];
+    [rolesMap setObject:@"JFXRadiobuttonAccessibility" forKey:@"TAB_ITEM"];


this change seems unrelated - is it a part of some other work?

When i implemented radio buttons one of the roles this implementation was supposed to fulfill was tab button for the tabbed pane. But without the new implementation of Tab Group it did not work - focus was funky and announcement was not correct. So i commented out this role assignment. When i pushed Tab Group implementation i haven't uncommented this line so now since i do clean up and refactoring on this exact file i tested that now it works properly and uncommented it. Just trying not to spawn too many pull requests for the minimal changes.

does it mean we missed something during the testing, something was supposed to work and did not?

since there will be more a-y work, I think it should be ok.

andy-goryachev-oracle

Like this very much, a good cleanup.

andy-goryachev-oracle · 2025-07-08T21:31:14Z

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


- (jobject)getJAccessible
-{
+- (jobject)getJAccessible {


should this K&R brace be reverted to the objectively better style used elsewhere in this file?

I would object to objectively - i think this is subjective - but sure, i will fix that :)

andy-goryachev-oracle · 2025-07-08T21:33:00Z

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

    GET_MAIN_JENV;
    if (env == NULL) return NULL;
-    jresult = (jobject)(*env)->CallLongMethod(env, self->jAccessible, jAccessibilityAttributeValue, (jlong)@"AXValue");
+    jresult = (jobject)(*env)->CallLongMethod(env, [self getJAccessible],


not an expert, but do we really need to initialize jresult to NULL in line 110?
can the variable be declared in line 113 ?

Well this pattern was used to initialize the result value to the default return value so if assignment does not happen due to the Java exception we have something meaningful to return but in case of NULL it is not really required because it will produce the same result anyways.

andy-goryachev-oracle · 2025-07-08T21:37:40Z

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

-    NSSize size = [variantToID(env, jresult) sizeValue];
+    id p = [self requestNodeAttribute:@"AXPosition"];
+    id s = [self requestNodeAttribute:@"AXSize"];
+    if (p == NULL || s == NULL) {


not a big issue, but would it be (marginally) better to bail out early if p == NULL ?

But that would require two conditions - one for position and one for size - with the same logic which would look not that compact and pretty.

but it will save a couple of nanoseconds!

the code is ok. one question though: what are the circumstances when these calls would return NULL?

Custom component that has no role assigned at all

Exception happens on Java side processing the request
That was just two popped out - probably there are more scenarios.

right, thank you for clarification!

andy-goryachev-oracle · 2025-07-08T21:57:59Z

I don't see any obvious issues in the monkey tester with the Logging -> Accessibility enabled.

Here, for example, what it logs for the focusable label:

{time:1752011850468, log:"accessibility", "TEXT":"\"[🇺🇦❤️🇺🇸🦋🏁🔥\n😀😃😄😁😆😅🤣😂\n🙂🙃😉😊😇]\""}
{time:1752011850468, log:"accessibility", "ROLE":"\"TEXT\""}
{time:1752011850468, log:"accessibility", "LABELED_BY":"null"}

azuev-java · 2025-07-14T17:55:21Z

@arapte Can you please take a look?

arapte

LGTM

azuev-java · 2025-07-16T20:47:06Z

/integrate

openjdk · 2025-07-16T21:08:15Z

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

99866ae: 8141391: Manual JFXPanel DnD test doesn't work
1a2a50b: 8362095: HeaderButtonMetrics should not be used across toolkit boundary
a574d92: 8359598: [TestBug] VirtualFlowTestUtils should not create a temporary Stage
... and 7 more: https://git.openjdk.org/jfx/compare/0270847b85ab6d4701abb201ec4b2aeee96129dd...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-07-16T21:08:50Z

@azuev-java Pushed as commit 2dd9026.

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

8361379: [macos] Refactor accessibility code to retrieve attribute by…

92c74cb

… name - Copyright year update - Introduce new function requestNodeAttribute and refactor code to use it - Fix some typos - Enable new code to handle TabPages since TabGroup was implemented

openjdk bot added the rfr Ready for review label Jul 3, 2025

andy-goryachev-oracle reviewed Jul 8, 2025

View reviewed changes

azuev-java added 2 commits July 8, 2025 21:20

Reverting the curly bracket placement

9bee13a

Remove explicit jresult declaration

3609f52

andy-goryachev-oracle approved these changes Jul 9, 2025

View reviewed changes

arapte approved these changes Jul 16, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jul 16, 2025

openjdk bot added the integrated Pull request has been integrated label Jul 16, 2025

openjdk bot closed this Jul 16, 2025

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Jul 16, 2025

8361379: [macos] Refactor accessibility code to retrieve attribute by name #1840

8361379: [macos] Refactor accessibility code to retrieve attribute by name #1840

Conversation

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

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 3, 2025

Uh oh!

openjdk bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

victordyakov commented Jul 8, 2025

Uh oh!

andy-goryachev-oracle commented Jul 8, 2025

Uh oh!

openjdk bot commented Jul 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Jul 8, 2025

Uh oh!

azuev-java commented Jul 14, 2025

Uh oh!

arapte left a comment

Choose a reason for hiding this comment

Uh oh!

azuev-java commented Jul 16, 2025

Uh oh!

openjdk bot commented Jul 16, 2025

Uh oh!

openjdk bot commented Jul 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

azuev-java commented Jul 3, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jul 3, 2025 •

edited

Loading

mlbridge bot commented Jul 3, 2025 •

edited

Loading