On Fri, 12 May 2023 18:24:29 GMT, Andy Goryachev <ango...@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
