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

[Kevin Rushforth]

On Fri, 12 May 2023 18:24:29 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

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

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.

> 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?

I wouldn't bother.

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

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 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?

Hopefully, this is just an Eclipse limitation. I'll check the actual javadoc (which I was planning to do anyway).

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

Yes, this is where I would recommend putting it.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192809729
PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192811319
PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192812188
PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192812665
PR Review Comment: https://git.openjdk.org/jfx/pull/1088#discussion_r1192812774