Change condition for adding AX_STATE_EXPANDED

Change condition for adding AX_STATE_EXPANDED
This state should be decided according to aria-expanded attribute.

BUG=98982
Committed: https://crrev.com/1f7fa9c53d0d3cc256ab9e50500ade447bd64c75
Cr-Commit-Position: refs/heads/master@{#302089}

[je_julie(Not used), 2014-10-20 12:56:34]

je_julie.kim@samsung.com changed reviewers:
+ dmazzoni@chromium.org, dtseng@chromium.org

[je_julie(Not used), 2014-10-20 12:58:50]

Hi Dominic and David,

This patch changes the condition for adding AX_STATE_EXPANDED.
The related CL is https://codereview.chromium.org/662313003/

PTAL.

[dmazzoni, 2014-10-20 16:34:34]

lgtm

[je_julie(Not used), 2014-10-24 07:44:43]

je_julie.kim@samsung.com changed reviewers:
+ mkwst@chromium.org

[je_julie(Not used), 2014-10-24 07:44:44]

Hi Dominic, Mike and Davie,
I updated the patch because we need to distinguish 'undefined' and 'false' for
aria-expanded 
and I also realized that we didn't expose AXExpanded attribute for Mac.

We have another issue related to isCollapsed().
According to
http://www.w3.org/TR/wai-aria-implementation/#mapping_state-property_table,
STATE_SYSTEM_EXPANDED and STATE_SYSTEM_COLLAPSED is exclusive.
!isExpanded() means STATE_SYSTEM_COLLAPSED.
Chromium checks STATE_SYSTEM_COLLAPSED with isCollpased() without aria-expanded
attribute.
(It is set by show/hide menu status on HTMLSelectElement.)

I think that
first, we should follow that isExpanded() is STATE_SYSTEM_EXPANDED and
!isExpanded() is STATE_SYSTEM_COLLAPSED for the spec. 
and then we can check isCollapsed based on the current code for user experience.
isCollapsed() is STATE_SYSTEM_COLLAPSED and !isCollapsed() is
STATE_SYSTEM_EXPANDED

I'll update test results for window and android soon.

I'd like to get your feedback.
Thanks,

[je_julie(Not used), 2014-10-24 07:47:39]

Sorry for the typos.

On 2014/10/24 07:44:44, je_julie wrote:
> Hi Dominic, Mike and David,
> I updated the patch because we need to distinguish 'undefined' and 'false' for
> aria-expanded 
> and I also realized that we didn't expose AXExpanded attribute for Mac.
> 
> We have another issue related to isCollapsed().
> According to
> http://www.w3.org/TR/wai-aria-implementation/#mapping_state-property_table,
> STATE_SYSTEM_EXPANDED and STATE_SYSTEM_COLLAPSED is exclusive.
> !isExpanded() means STATE_SYSTEM_COLLAPSED.
> Chromium checks STATE_SYSTEM_COLLAPSED with isCollpased() without
aria-expanded
> attribute.
> (It is set by show/hide menu status on HTMLSelectElement.)
> 
> I think that
> first, we should follow that isExpanded() is STATE_SYSTEM_EXPANDED and
> !isExpanded() is STATE_SYSTEM_COLLAPSED for the spec. 
> and then we can check isCollapsed based on the current code for user
experience.
> isCollapsed() is STATE_SYSTEM_COLLAPSED and !isCollapsed() is
> STATE_SYSTEM_EXPANDED
> 
> I'd like to get your feedback.
> Thanks,

[dmazzoni, 2014-10-24 07:54:54]

lgtm

Fantastic, thanks!

[Mike West, 2014-10-24 09:53:44]

On 2014/10/24 07:54:54, dmazzoni wrote:
> lgtm
> 
> Fantastic, thanks!

content/shell (and the rest) LGTM 2.

[je_julie(Not used), 2014-10-30 00:57:23]

Patchset #5 (id:80001) has been deleted

[je_julie(Not used), 2014-10-30 00:57:36]

Patchset #4 (id:60001) has been deleted

[je_julie(Not used), 2014-10-30 04:01:10]

Hi Dominic and Mike,
I updated this patchset to handle expanded state for chromium according to
adding new enum in blink.
PTAL.

[dmazzoni, 2014-10-30 05:35:31]

Fantastic, this is definitely looking better.

My only complaint is that you're defining AXExpanded in
ui/accessibility/ax_enums but you're not ever storing that value. It doesn't
make sense to me to convert the blink enum to a ui enum in
blink_ax_enum_conversion, only to just set the collapsed and expanded states
from there.

Either of these would be fine:

1. Get rid of AXExpanded and just set the expanded and collapsed states in
blink_ax_enum_conversion.
2. Or, get rid of the collapsed and expanded states and instead add a new int
attribute for AXExpanded that's stored in each node. Any code that wants to know
if the state is expanded or collapsed could check that enum rather than checking
both of the collapsed and expanded states.

[je_julie(Not used), 2014-10-30 10:33:56]

On 2014/10/30 05:35:31, dmazzoni wrote:
> Fantastic, this is definitely looking better.
> 
> My only complaint is that you're defining AXExpanded in
> ui/accessibility/ax_enums but you're not ever storing that value. It doesn't
> make sense to me to convert the blink enum to a ui enum in
> blink_ax_enum_conversion, only to just set the collapsed and expanded states
> from there.
> 
> Either of these would be fine:
> 
> 1. Get rid of AXExpanded and just set the expanded and collapsed states in
> blink_ax_enum_conversion.
> 2. Or, get rid of the collapsed and expanded states and instead add a new int
> attribute for AXExpanded that's stored in each node. Any code that wants to
know
> if the state is expanded or collapsed could check that enum rather than
checking
> both of the collapsed and expanded states.

Dominic,
Thank you for your comment.
I removed AXExpanded and update patchset. PTAL.

[dmazzoni, 2014-10-30 15:09:16]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2014-10-30 15:09:17]

lgtm

[commit-bot: I haz the power, 2014-10-30 15:10:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/665843002/120001

[commit-bot: I haz the power, 2014-10-30 16:09:51]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2014-10-30 16:10:34]

Patchset 5 (id:??) landed as
https://crrev.com/1f7fa9c53d0d3cc256ab9e50500ade447bd64c75
Cr-Commit-Position: refs/heads/master@{#302089}