On Fri, 12 May 2023 10:36:12 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> 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](https://learn.microsoft.com/en-us/windows/win32/winauto/uiauto-implementingtoggle) >> 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. >> 1. Run the sample program attached to JBS issue. >> 2. Launch "Accessibility Insights for Window" >> 3. Observe that patterns section for each item >> 4. Select / de-select the CheckBoxes and observe the patterns section for >> correctness of toggle state. > > Ambarish Rapte has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Merge branch 'master' into a11y-8284542-CheckBoxTreeItem > - Review Comment: Add enum ToggleState > - Add CheckBoxTreeItem role and TOGGLE_STATE attribute 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) modules/javafx.controls/src/main/java/javafx/scene/control/cell/CheckBoxTreeCell.java line 498: > 496: state = ToggleState.CHECKED; > 497: } > 498: 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; } modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacAccessible.java line 369: > 367: AccessibleRole.TREE_ITEM, > 368: AccessibleRole.CHECK_BOX_TREE_ITEM, > 369: AccessibleRole.TREE_TABLE_ROW [question] is the order of elements significant in any way? modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacAccessible.java line 761: > 759: > 760: AccessibleRole role = (AccessibleRole) > getAttribute(ROLE); > 761: if (role == AccessibleRole.TREE_ITEM || role == > AccessibleRole.CHECK_BOX_TREE_ITEM || role == AccessibleRole.TREE_TABLE_ROW) { could a `switch` statement be used here instead? modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinAccessible.java line 303: > 301: case INDETERMINATE: { > 302: if (getAttribute(ROLE) == AccessibleRole.CHECK_BOX > 303: || 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) modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinAccessible.java line 310: > 308: case SELECTED: { > 309: Object role = getAttribute(ROLE); > 310: if (role == AccessibleRole.CHECK_BOX || role == > AccessibleRole.TOGGLE_BUTTON switch? modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinAccessible.java line 436: > 434: case TAB_ITEM: return > getContainerAccessible(AccessibleRole.TAB_PANE); > 435: case PAGE_ITEM: return > getContainerAccessible(AccessibleRole.PAGINATION); > 436: 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) modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinAccessible.java line 1594: > 1592: return ToggleState_On; > 1593: } > 1594: return ToggleState_Off; also here, I'd do if { } else if { } else { } modules/javafx.graphics/src/main/java/javafx/scene/AccessibleAttribute.java line 828: > 826: * This enum describes the values for TOGGLE_STATE attribute. > 827: * > 828: * @see TOGGLE_STATE this might be Eclipse limitation, but this link does not resolve (in Eclipse javadoc widget). should it? modules/javafx.graphics/src/main/java/javafx/scene/AccessibleAttribute.java line 846: > 844: */ > 845: INDETERMINATE > 846: } is this the right place for ToggleState? looks like the right place, just want to confirm. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1088#issuecomment-1546216763 PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192668641 PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192674675 PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192675679 PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192677649 PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192678082 PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192679829 PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192681071 PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192695563 PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192747447
