8284542: [Accessibility] [Win] Missing attribute for toggle state of CheckBox in CheckBoxTreeItem #1088

arapte · 2023-04-12T15:13:25Z

Issue:
CheckBoxTreeItem extends TreeItem and adds a CheckBox.
The state of this CheckBox is not visible to an accessibility client application.
If we analyze a simple program that contains a CheckBoxTreeItem using a windows application "Accessibility Insights for Window", we can notice that toggle state of CheckBox is not exposed.

Fix:
Include the Toggle Control Pattern in Accessibility information of a CheckBoxTreeItem in addition to the patterns that are used for a TreeItem.

Verification:
On Windows: Do the following with and without the fix.

Run the sample program attached to JBS issue.
Launch "Accessibility Insights for Window"
Observe that patterns section for each item
Select / de-select the CheckBoxes and observe the patterns section for correctness of toggle state.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8305939 to be approved
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8284542: [Accessibility] [Win] Missing attribute for toggle state of CheckBox in CheckBoxTreeItem
JDK-8305939: [Accessibility] [Win] Missing attribute for toggle state of CheckBox in CheckBoxTreeItem (CSR)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Alexander Zuev (@azuev-java - no project role)
Andy Goryachev (@andy-goryachev-oracle - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1088

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-04-12T15:14:38Z

👋 Welcome back arapte! 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-04-12T15:19:57Z

Webrevs

kevinrushforth · 2023-04-12T16:17:15Z

@azuev-java @andy-goryachev-oracle can you take a look at this as well?

kevinrushforth · 2023-04-12T16:17:26Z

/reviewers 2
/csr

openjdk · 2023-04-12T16:18:46Z

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

openjdk · 2023-04-12T16:19:46Z

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@arapte please create a CSR request for issue JDK-8284542 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

modules/javafx.graphics/src/main/java/javafx/scene/AccessibleAttribute.java

kevinrushforth

I echo Andy's question about using an enum for the new AccessibleAttribute. More importantly, I think the values of that new attribute need to be a platform-independent. See inline comments.

modules/javafx.graphics/src/main/java/javafx/scene/AccessibleAttribute.java

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinAccessible.java

modules/javafx.controls/src/main/java/javafx/scene/control/cell/CheckBoxTreeCell.java

andy-goryachev-oracle · 2023-05-12T19:53:35Z

Testing with VoiceOver on Mac M1 Ventura 13.3.1(a): undetermined checkbox is pronounced as "mixed checkbox" - is this expected behavior?

Also, probably unrelated to this PR: the outlines placed by VoiceOver are shifted to the wrong positions on the secondary monitor, for all nodes in the javafx window. For Chrome, the positioning is correct which makes me believe there might be a problem with FX. Also, the VoiceOver popup (black thing with white outline) inexplicably spans two monitors, though I think it might be a problem with VoiceOver:

my setup is: secondary monitor (scale=1) is above the primary (scale=2)

andy-goryachev-oracle · 2023-05-12T18:12:19Z

modules/javafx.controls/src/main/java/javafx/scene/control/cell/CheckBoxTreeCell.java

+                } else if (checkBox.isSelected()) {
+                    state = ToggleState.CHECKED;
+                }
+                return state;


very minor, a question of style. i would avoid double assignment by doing this:

if (checkBox.isIndeterminate()) { return ToggleState.INDETERMINATE; } else if (checkBox.isSelected()) { return ToggleState.CHECKED; } else { return ToggleState.UNCHECKED; }

andy-goryachev-oracle · 2023-05-12T18:20:37Z

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

+                AccessibleRole.TABLE_ROW,
+                AccessibleRole.TREE_ITEM,
+                AccessibleRole.CHECK_BOX_TREE_ITEM,
+                AccessibleRole.TREE_TABLE_ROW


[question] is the order of elements significant in any way?

The order does not matter, the roles are accessed by value on the native side code. (initEnum())

andy-goryachev-oracle · 2023-05-12T18:22:04Z

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

@@ -748,8 +758,9 @@ public void sendNotification(AccessibleAttribute notification) {
                }

                AccessibleRole role = (AccessibleRole) getAttribute(ROLE);
-                if (role == AccessibleRole.TREE_ITEM || role == AccessibleRole.TREE_TABLE_ROW) {
-                    AccessibleRole containerRole = role == AccessibleRole.TREE_ITEM ? AccessibleRole.TREE_VIEW : AccessibleRole.TREE_TABLE_VIEW;
+                if (role == AccessibleRole.TREE_ITEM || role == AccessibleRole.CHECK_BOX_TREE_ITEM || role == AccessibleRole.TREE_TABLE_ROW) {


could a switch statement be used here instead?

andy-goryachev-oracle · 2023-05-12T18:24:29Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinAccessible.java

@@ -299,14 +299,16 @@ public void sendNotification(AccessibleAttribute notification) {
                break;
            }
            case INDETERMINATE: {
-                if (getAttribute(ROLE) == AccessibleRole.CHECK_BOX) {
+                if (getAttribute(ROLE) == AccessibleRole.CHECK_BOX
+                        || getAttribute(ROLE) == AccessibleRole.CHECK_BOX_TREE_ITEM) {


same Q.: could we use a switch statement to avoid double getter call? (I don't know if JVM will optimize that away anyway, but switch makes a cleaner code, in my opinion)

I think the proposed change is less intrusive than a switch, but I don't mind either way. It might make sense to use a local variable to avoid calling getAttribute twice.

andy-goryachev-oracle · 2023-05-12T18:25:00Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinAccessible.java

                    notifyToggleState();
                }
                break;
            }
            case SELECTED: {
                Object role = getAttribute(ROLE);
-                if (role == AccessibleRole.CHECK_BOX || role == AccessibleRole.TOGGLE_BUTTON) {
+                if (role == AccessibleRole.CHECK_BOX || role == AccessibleRole.TOGGLE_BUTTON


I wouldn't bother.

andy-goryachev-oracle · 2023-05-12T18:27:27Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinAccessible.java

@@ -431,6 +433,7 @@ private Accessible getContainer() {
                case LIST_ITEM: return getContainerAccessible(AccessibleRole.LIST_VIEW);
                case TAB_ITEM: return getContainerAccessible(AccessibleRole.TAB_PANE);
                case PAGE_ITEM: return getContainerAccessible(AccessibleRole.PAGINATION);
+                case CHECK_BOX_TREE_ITEM:


I would recommend reformatting the switch cases here for clarity:

private Accessible getContainer() { if (isDisposed()) { return null; } AccessibleRole role = (AccessibleRole)getAttribute(ROLE); if (role != null) { switch (role) { case TABLE_ROW: case TABLE_CELL: return getContainerAccessible(AccessibleRole.TABLE_VIEW); case LIST_ITEM: return getContainerAccessible(AccessibleRole.LIST_VIEW); case TAB_ITEM: return getContainerAccessible(AccessibleRole.TAB_PANE); case PAGE_ITEM: return getContainerAccessible(AccessibleRole.PAGINATION); case CHECK_BOX_TREE_ITEM: case TREE_ITEM: return getContainerAccessible(AccessibleRole.TREE_VIEW); case TREE_TABLE_ROW: case TREE_TABLE_CELL: return getContainerAccessible(AccessibleRole.TREE_TABLE_VIEW); } } return null; }

(and below also. but again, this is just my preference)

I usually prefer to avoid these sort of unrelated changes to minimize the diffs. Either way is fine.

in this particular case, with many fall-throughs, I think the code would benefit from reformatting.

Minimal diff eases in tracing regression and in review. May be we can take up the cleanup as a separate issue, there seems to be multiple of such instances. (of switch/replacing if with switch)

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinAccessible.java

modules/javafx.graphics/src/main/java/javafx/scene/AccessibleAttribute.java

kevinrushforth

The additional changes to add the enum look good to me. I'll test it and review the javadoc.

kevinrushforth · 2023-05-12T21:29:21Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinAccessible.java

@@ -299,14 +299,16 @@ public void sendNotification(AccessibleAttribute notification) {
                break;
            }
            case INDETERMINATE: {
-                if (getAttribute(ROLE) == AccessibleRole.CHECK_BOX) {
+                if (getAttribute(ROLE) == AccessibleRole.CHECK_BOX
+                        || getAttribute(ROLE) == AccessibleRole.CHECK_BOX_TREE_ITEM) {


I think the proposed change is less intrusive than a switch, but I don't mind either way. It might make sense to use a local variable to avoid calling getAttribute twice.

kevinrushforth · 2023-05-12T21:32:33Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinAccessible.java

                    notifyToggleState();
                }
                break;
            }
            case SELECTED: {
                Object role = getAttribute(ROLE);
-                if (role == AccessibleRole.CHECK_BOX || role == AccessibleRole.TOGGLE_BUTTON) {
+                if (role == AccessibleRole.CHECK_BOX || role == AccessibleRole.TOGGLE_BUTTON


I wouldn't bother.

kevinrushforth · 2023-05-12T21:34:20Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinAccessible.java

@@ -431,6 +433,7 @@ private Accessible getContainer() {
                case LIST_ITEM: return getContainerAccessible(AccessibleRole.LIST_VIEW);
                case TAB_ITEM: return getContainerAccessible(AccessibleRole.TAB_PANE);
                case PAGE_ITEM: return getContainerAccessible(AccessibleRole.PAGINATION);
+                case CHECK_BOX_TREE_ITEM:


I usually prefer to avoid these sort of unrelated changes to minimize the diffs. Either way is fine.

modules/javafx.graphics/src/main/java/javafx/scene/AccessibleAttribute.java

kevinrushforth · 2023-05-12T21:40:26Z

Testing with VoiceOver on Mac M1 Ventura 13.3.1(a): undetermined checkbox is pronounced as "mixed checkbox" - is this expected behavior?

This is a Windows-only fix, so the behavior should be unchanged on Mac from before this fix. The only change on Mac is to react to the new AccessibleRole value (to do what it would have done before).

andy-goryachev-oracle · 2023-05-12T22:04:11Z

it looks like the behavior on Mac is unchanged.

andy-goryachev-oracle · 2023-05-12T22:06:04Z

I can't find anything like VoiceOver on Windows 11. Do you need JAWS or similar to test?

kevinrushforth

Testing looks (and sounds) good to me. I'll reapprove if you make the suggested formatting or doc changes. Go ahead and update the CSR when ready.

kevinrushforth · 2023-05-12T22:19:00Z

I can't find anything like VoiceOver on Windows 11. Do you need JAWS or similar to test?

You can use Windows Narrator.

kevinrushforth · 2023-05-12T22:30:27Z

Also, probably unrelated to this PR: the outlines placed by VoiceOver are shifted to the wrong positions on the secondary monitor

Definitely unrelated, but it should be filed as a bug. I remember seeing this before, but I don't see a bug for it.

azuev-java · 2023-05-15T09:47:33Z

I can't find anything like VoiceOver on Windows 11. Do you need JAWS or similar to test?

On Windows 10/11 it is called Narrator and is in Start->All Apps->Accessibility->Narrator
Unlike AWT/Swing JavaFX does not require JAWS or NVDA.

azuev-java

Looks good.

azuev-java

Looks good.

andy-goryachev-oracle

looks and sounds good.

andy-goryachev-oracle · 2023-05-16T15:23:37Z

Is JDK-8087651 a duplicate of this issue? or related?

andy-goryachev-oracle · 2023-05-16T15:49:57Z

Definitely unrelated, but it should be filed as a bug. I remember seeing this before, but I don't see a bug for it.

created JDK-8308191
[macOS] VoiceOver decorations are shifted on second monitor

openjdk · 2023-05-17T15:46:54Z

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

8284542: [Accessibility] [Win] Missing attribute for toggle state of CheckBox in CheckBoxTreeItem

Reviewed-by: kcr, kizune, angorya

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

bff41c2: 8308114: Bump minimum version of macOS for x64 to 11.0 (matching aarch64)
c1a1179: 8245919: Region#padding property rendering error
e7974bc: 8308028: Replace more uses of System.getProperty("os.name") with PlatformUtil calls
8aff525: 8307960: Create Table Column PopupMenu lazily
7095364: 8307807: Replace use of System.getProperty("os.name") with PlatformUtil calls

Please see this link for an up-to-date comparison between the source branch of this pull request and 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.

arapte · 2023-05-17T16:45:39Z

/integrate

openjdk · 2023-05-17T16:46:23Z

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

6aeaff3: 8295078: TextField blurry when inside an TitledPane -> AnchorPane
bff41c2: 8308114: Bump minimum version of macOS for x64 to 11.0 (matching aarch64)
c1a1179: 8245919: Region#padding property rendering error
e7974bc: 8308028: Replace more uses of System.getProperty("os.name") with PlatformUtil calls
8aff525: 8307960: Create Table Column PopupMenu lazily
7095364: 8307807: Replace use of System.getProperty("os.name") with PlatformUtil calls

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-17T16:46:35Z

@arapte Pushed as commit bdadcb2.

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

Add CheckBoxTreeItem role and TOGGLE_STATE attribute

29dbf6c

openjdk bot added the rfr Ready for review label Apr 12, 2023

kevinrushforth self-requested a review April 12, 2023 16:15

openjdk bot added the csr Need approved CSR to integrate pull request label Apr 12, 2023

andy-goryachev-oracle reviewed May 3, 2023

View reviewed changes

modules/javafx.graphics/src/main/java/javafx/scene/AccessibleAttribute.java Outdated Show resolved Hide resolved

kevinrushforth reviewed May 9, 2023

View reviewed changes

arapte changed the title ~~8284542: Missing attribute for state of CheckBox in CheckBoxTreeItem~~ 8284542: [Accessibility] [Win] Missing attribute for toggle state of CheckBox in CheckBoxTreeItem May 12, 2023

arapte added 2 commits May 12, 2023 11:22

Review Comment: Add enum ToggleState

b5beba5

Merge branch 'master' into a11y-8284542-CheckBoxTreeItem

2d89e9f

andy-goryachev-oracle reviewed May 12, 2023

View reviewed changes

kevinrushforth reviewed May 12, 2023

View reviewed changes

kevinrushforth approved these changes May 12, 2023

View reviewed changes

azuev-java approved these changes May 15, 2023

View reviewed changes

andy-goryachev-oracle approved these changes May 15, 2023

View reviewed changes

Review comments

5c8da12

andy-goryachev-oracle approved these changes May 16, 2023

View reviewed changes

kevinrushforth approved these changes May 16, 2023

View reviewed changes

openjdk bot added ready Ready to be integrated and removed csr Need approved CSR to integrate pull request labels May 17, 2023

azuev-java approved these changes May 17, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 17, 2023

openjdk bot closed this May 17, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels May 17, 2023

arapte deleted the a11y-8284542-CheckBoxTreeItem branch October 23, 2023 12:08

8284542: [Accessibility] [Win] Missing attribute for toggle state of CheckBox in CheckBoxTreeItem #1088

8284542: [Accessibility] [Win] Missing attribute for toggle state of CheckBox in CheckBoxTreeItem #1088

Uh oh!

Conversation

arapte commented Apr 12, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Apr 12, 2023

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Apr 12, 2023

Uh oh!

kevinrushforth commented Apr 12, 2023

Uh oh!

openjdk bot commented Apr 12, 2023

Uh oh!

openjdk bot commented Apr 12, 2023

Uh oh!

Uh oh!

kevinrushforth 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!

Uh oh!

Uh oh!

Uh oh!

andy-goryachev-oracle commented May 12, 2023

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!

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!

Uh oh!

Uh oh!

Uh oh!

kevinrushforth 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!

Uh oh!

Uh oh!

kevinrushforth commented May 12, 2023

Uh oh!

andy-goryachev-oracle commented May 12, 2023

Uh oh!

andy-goryachev-oracle commented May 12, 2023

Uh oh!

arapte commented Apr 12, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 12, 2023 •

edited

Loading

kevinrushforth left a comment •

edited

Loading

andy-goryachev-oracle commented May 16, 2023 •

edited

Loading

openjdk bot commented May 17, 2023 •

edited

Loading