IAccessible2 state not defined for input type checkbox.

IAccessible2 state not defined for input type checkbox.

Input type checkbox should have IAccessiblie2 state as
IA2_STATE_CHECKABLE.

BUG=418013
Committed: https://crrev.com/ef044b5b099eb06d5fd0557dda25be846d23b272
Cr-Commit-Position: refs/heads/master@{#298287}

[shreeramk, 2014-09-26 18:42:15]

shreeram.k@samsung.com changed reviewers:
+ dmazzoni@chromium.org, dtseng@chromium.org

[shreeramk, 2014-09-26 18:42:16]

@Dominic:

Each MSAA / IAccessible roles is also a valid IAccessible2 role, the IA_ROLE_MAP
macro adds it to both.

But for MSAA / IAccessible states, unlike roles, these are not also IA2 states.

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
File content/browser/accessibility/accessibility_tree_formatter_utils_win.cc
(right):

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
content/browser/accessibility/accessibility_tree_formatter_utils_win.cc:200:
IA2_STATE_MAP(IA2_STATE_CHECKABLE)
IA2_STATE_CHECKABLE was handled here but how to handle following IA2 states -
STATE_SYSTEM_CHECKED and STATE_SYSTEM_MIXED??

[dmazzoni, 2014-09-26 18:52:43]

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
File content/browser/accessibility/accessibility_tree_formatter_utils_win.cc
(right):

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
content/browser/accessibility/accessibility_tree_formatter_utils_win.cc:200:
IA2_STATE_MAP(IA2_STATE_CHECKABLE)
On 2014/09/26 18:42:16, shreeram.k wrote:
> IA2_STATE_CHECKABLE was handled here but how to handle following IA2 states -
> STATE_SYSTEM_CHECKED and STATE_SYSTEM_MIXED??

Those are defined above:

IA_STATE_MAP(CHECKED)
IA_STATE_MAP(MIXED)

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3429:
&is_aria_checked_defined, &is_mixed_check)) {
nit: indent 4 chars for line continuation

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3433: ia_state_ =
STATE_SYSTEM_MIXED;
This should be an |= to preserve any other state flags

[shreeramk, 2014-09-26 19:14:43]

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
File content/browser/accessibility/accessibility_tree_formatter_utils_win.cc
(right):

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
content/browser/accessibility/accessibility_tree_formatter_utils_win.cc:200:
IA2_STATE_MAP(IA2_STATE_CHECKABLE)
On 2014/09/26 18:52:43, dmazzoni wrote:
> On 2014/09/26 18:42:16, shreeram.k wrote:
> > IA2_STATE_CHECKABLE was handled here but how to handle following IA2 states
-
> > STATE_SYSTEM_CHECKED and STATE_SYSTEM_MIXED??
> 
> Those are defined above:
> 
> IA_STATE_MAP(CHECKED)
> IA_STATE_MAP(MIXED)
> 

But according to specs STATE_SYSTEM_CHECKED and STATE_SYSTEM_MIXED has to be
defined for IA2_STATE.

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3430: ia_state_ =
STATE_SYSTEM_CHECKED;
I think I made a mistake here in uploading the patch. (instead of ia2_state_, I
used ia_state_.

Because According to specs following states STATE_SYSTEM_CHECKED and
STATE_SYSTEM_MIXED has to be exposed through IA2_STATE.

But, we won't be able to expose those states as IA2_STATE, since these are not
defined using IA2_STATE_MAP macro.

Rather it has been defined using IA_STATE_MAP macro.

[shreeramk, 2014-09-29 05:37:28]

PTAL!

[dmazzoni, 2014-09-29 05:44:07]

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
File content/browser/accessibility/accessibility_tree_formatter_utils_win.cc
(right):

https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
content/browser/accessibility/accessibility_tree_formatter_utils_win.cc:200:
IA2_STATE_MAP(IA2_STATE_CHECKABLE)
On 2014/09/26 19:14:43, shreeram.k wrote:
> On 2014/09/26 18:52:43, dmazzoni wrote:
> > On 2014/09/26 18:42:16, shreeram.k wrote:
> > > IA2_STATE_CHECKABLE was handled here but how to handle following IA2
states
> -
> > > STATE_SYSTEM_CHECKED and STATE_SYSTEM_MIXED??
> > 
> > Those are defined above:
> > 
> > IA_STATE_MAP(CHECKED)
> > IA_STATE_MAP(MIXED)
> > 
> 
> But according to specs STATE_SYSTEM_CHECKED and STATE_SYSTEM_MIXED has to be
> defined for IA2_STATE.

No, you're misunderstanding the spec.

IAccessible2 is an extension of MSAA/IAccessible. It doesn't replace it, it
extends it.

The spec has a column for what a MSAA+IAccessible2 browser should do. That's the
column we should follow for Chrome Windows support. Anything in that column
that's an IA2-only role or attribute, we return from IA2 functions (using:
ia2_role_ and ia2_state_). Anything that's a role or attribute from MSAA/IA, we
return from the MSAA/IA functions.

In this case, STATE_SYSTEM_CHECKED and STATE_SYSTEM_MIXED are already part of
MSAA/IA, so we set them in ia_state_ and nothing IA2-specific is needed.

[shreeramk, 2014-09-29 05:45:43]

On 2014/09/29 05:44:07, dmazzoni wrote:
>
https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
> File content/browser/accessibility/accessibility_tree_formatter_utils_win.cc
> (right):
> 
>
https://codereview.chromium.org/607803002/diff/40001/content/browser/accessib...
> content/browser/accessibility/accessibility_tree_formatter_utils_win.cc:200:
> IA2_STATE_MAP(IA2_STATE_CHECKABLE)
> On 2014/09/26 19:14:43, shreeram.k wrote:
> > On 2014/09/26 18:52:43, dmazzoni wrote:
> > > On 2014/09/26 18:42:16, shreeram.k wrote:
> > > > IA2_STATE_CHECKABLE was handled here but how to handle following IA2
> states
> > -
> > > > STATE_SYSTEM_CHECKED and STATE_SYSTEM_MIXED??
> > > 
> > > Those are defined above:
> > > 
> > > IA_STATE_MAP(CHECKED)
> > > IA_STATE_MAP(MIXED)
> > > 
> > 
> > But according to specs STATE_SYSTEM_CHECKED and STATE_SYSTEM_MIXED has to be
> > defined for IA2_STATE.
> 
> No, you're misunderstanding the spec.
> 
> IAccessible2 is an extension of MSAA/IAccessible. It doesn't replace it, it
> extends it.
> 
> The spec has a column for what a MSAA+IAccessible2 browser should do. That's
the
> column we should follow for Chrome Windows support. Anything in that column
> that's an IA2-only role or attribute, we return from IA2 functions (using:
> ia2_role_ and ia2_state_). Anything that's a role or attribute from MSAA/IA,
we
> return from the MSAA/IA functions.
> 
> In this case, STATE_SYSTEM_CHECKED and STATE_SYSTEM_MIXED are already part of
> MSAA/IA, so we set them in ia_state_ and nothing IA2-specific is needed.

Ok. Got it. Thanks a lot for this clarification.

Patch set 4 is based on your above explanation.

[dmazzoni, 2014-09-29 05:49:51]

https://codereview.chromium.org/607803002/diff/60001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/607803002/diff/60001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3430: ia_state_ |=
STATE_SYSTEM_CHECKED;
This seems wrong - you're setting it to CHECKED whether it's checked or not?

https://codereview.chromium.org/607803002/diff/60001/content/test/data/access...
File content/test/data/accessibility/aria-checkbox-expected-win.txt (right):

https://codereview.chromium.org/607803002/diff/60001/content/test/data/access...
content/test/data/accessibility/aria-checkbox-expected-win.txt:3:
ROLE_SYSTEM_CHECKBUTTON xml-roles:checkbox checkable:true
Shouldn't IA2_STATE_CHECKABLE be set on all of these?

[shreeramk, 2014-09-29 05:58:32]

I ll upload the changed in the next patchset

https://codereview.chromium.org/607803002/diff/60001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/607803002/diff/60001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_win.cc:3430: ia_state_ |=
STATE_SYSTEM_CHECKED;
On 2014/09/29 05:49:51, dmazzoni wrote:
> This seems wrong - you're setting it to CHECKED whether it's checked or not?

GetAriaTristate() compares the value of the first parameter we pass and returns
"true"  if the first parmeter is set to "true".

There is one mistake here, 
if (is_mixed_check) should be outside of this if
(GetAriaTristate("aria-checked",....

[shreeramk, 2014-09-29 06:02:23]

Would it be like this??

case ui::AX_ROLE_CHECK_BOX:
      ia_role_ = ROLE_SYSTEM_CHECKBUTTON;
      bool is_aria_checked_defined;
      bool is_mixed_check;
      if (GetAriaTristate("aria-checked",
          &is_aria_checked_defined, &is_mixed_check))
        ia2_state_ = IA2_STATE_CHECKABLE;
      if (is_aria_checked_defined)
        ia_state_ |= STATE_SYSTEM_CHECKED;
      if (is_mixed_check)
          ia_state_ |= STATE_SYSTEM_MIXED;
      break;

I ll test and upload the correct patch in sometime.

[dmazzoni, 2014-09-29 06:17:18]

> case ui::AX_ROLE_CHECK_BOX:
>       ia_role_ = ROLE_SYSTEM_CHECKBUTTON;
>       bool is_aria_checked_defined;
>       bool is_mixed_check;
>       if (GetAriaTristate("aria-checked",
>           &is_aria_checked_defined, &is_mixed_check))
>         ia2_state_ = IA2_STATE_CHECKABLE;
>       if (is_aria_checked_defined)
>         ia_state_ |= STATE_SYSTEM_CHECKED;
>       if (is_mixed_check)
>           ia_state_ |= STATE_SYSTEM_MIXED;
>       break;
> 
> I ll test and upload the correct patch in sometime.

Now this is backwards.

Thinking about it some more, I think we should always set CHECKABLE if the role
is check box. It should be CHECKABLE even if the author forgot to add
aria-checked="".: Note that IA2_STATE_CHECKABLE must be set on ia2_state_, not
ia_state_.

       ia2_state_ |= IA2_STATE_CHECKABLE;

I think the remaining cases should look like this:

       if (GetAriaTristate("aria-checked",
           &is_aria_checked_defined, &is_mixed_check))
         ia2_state_ = STATE_SYSTEM_CHECKED;
       if (is_mixed_check)
           ia_state_ |= STATE_SYSTEM_MIXED;

[shreeramk, 2014-09-29 15:40:26]

PTAL!

Thanks :)

[dmazzoni, 2014-09-29 15:55:04]

Looking much better! Two more small issues, thanks

https://codereview.chromium.org/607803002/diff/80001/content/test/data/access...
File content/test/data/accessibility/aria-checkbox.html (right):

https://codereview.chromium.org/607803002/diff/80001/content/test/data/access...
content/test/data/accessibility/aria-checkbox.html:11: <div role="checkbox"
aria-checked="true"></div>
Add tabindex=0 to each of these to make them focusable, then the ARIA and
non-ARIA examples really are equivalent.

https://codereview.chromium.org/607803002/diff/80001/content/test/data/access...
File content/test/data/accessibility/checkbox-expected-android.txt (right):

https://codereview.chromium.org/607803002/diff/80001/content/test/data/access...
content/test/data/accessibility/checkbox-expected-android.txt:3:
android.widget.CheckBox checkable focusable
Looks like this one should be "checked", right?

[dmazzoni, 2014-09-29 15:55:05]

Looking much better! Two more small issues, thanks

[shreeramk, 2014-09-30 08:53:08]

shreeram.k@samsung.com changed reviewers:
+ dmazzoni@chromium.org, dtseng@chromium.org

[shreeramk, 2014-09-30 08:53:39]

On 2014/09/29 15:55:05, dmazzoni wrote:
> Looking much better! Two more small issues, thanks

I have incorporated both the comments.

PTAL.

Thanks :)

[dmazzoni, 2014-09-30 15:25:55]

This ended up being trickier than it looked to either of us at first!

I'm sorry I didn't catch this earlier, but I just realized that you are *not*
allowed to use aria-checked to specify the checked state of an <input
type="checkbox">. It's pretty obscure, but I found the following paragraph in
the ARIA User Agent Implementation Guide:

http://www.w3.org/TR/wai-aria-implementation/#mapping_conflicts

"When WAI-ARIA states and properties correspond to host language features that
have the same implicit WAI-ARIA semantic, it can be problematic if the values
become out of sync. For example, the HTML checked attribute and the aria-checked
attribute could have conflicting values. Therefore to prevent providing
conflicting states and properties to assistive technologies, host languages will
explicitly declare where the use of WAI-ARIA attributes on a host language
element conflict with native attributes for that element. When a host language
declares a WAI-ARIA attribute to be in direct semantic conflict with a native
attribute for a given element, user agents MUST ignore the WAI-ARIA attribute
and instead use the host language attribute with the same implicit semantic."

So for anything with role="checkbox", we're supposed to use aria-checked -
however for <input type="checkbox"> we're supposed to ignore it, and only pay
attention to the CHECKED attribute.

Ideally we should test at least four different scenarios:

1. <div tabIndex=0 role="checkbox" aria-checked="true">
2. <input type="checkbox" checked>
3. <input type="checkbox" aria-checked="true">
4. <input type="checkbox" role="checkbox" aria-checked="true">

#3 should not be checked; we should ignore aria-checked. #4 is allowed, we
should not ignore aria-checked.

Finally, in terms of implementation, I think we shouldn't have to call
GetAriaTristate. We already have AX_ATTR_BUTTON_MIXED that's supposed to be
correctly set if the checkbox is in "mixed" state. If that's not returning the
correct value, we should fix it in Blink.

[shreeramk, 2014-10-06 08:28:49]

On 2014/09/30 15:25:55, dmazzoni wrote:
> This ended up being trickier than it looked to either of us at first!
> 
> I'm sorry I didn't catch this earlier, but I just realized that you are *not*
> allowed to use aria-checked to specify the checked state of an <input
> type="checkbox">. It's pretty obscure, but I found the following paragraph in
> the ARIA User Agent Implementation Guide:
> 
> http://www.w3.org/TR/wai-aria-implementation/#mapping_conflicts
> 
> "When WAI-ARIA states and properties correspond to host language features that
> have the same implicit WAI-ARIA semantic, it can be problematic if the values
> become out of sync. For example, the HTML checked attribute and the
aria-checked
> attribute could have conflicting values. Therefore to prevent providing
> conflicting states and properties to assistive technologies, host languages
will
> explicitly declare where the use of WAI-ARIA attributes on a host language
> element conflict with native attributes for that element. When a host language
> declares a WAI-ARIA attribute to be in direct semantic conflict with a native
> attribute for a given element, user agents MUST ignore the WAI-ARIA attribute
> and instead use the host language attribute with the same implicit semantic."
> 
> So for anything with role="checkbox", we're supposed to use aria-checked -
> however for <input type="checkbox"> we're supposed to ignore it, and only pay
> attention to the CHECKED attribute.
> 
> Ideally we should test at least four different scenarios:
> 
> 1. <div tabIndex=0 role="checkbox" aria-checked="true">
> 2. <input type="checkbox" checked>
> 3. <input type="checkbox" aria-checked="true">
> 4. <input type="checkbox" role="checkbox" aria-checked="true">
> 
> #3 should not be checked; we should ignore aria-checked. #4 is allowed, we
> should not ignore aria-checked.
> 
> Finally, in terms of implementation, I think we shouldn't have to call
> GetAriaTristate. We already have AX_ATTR_BUTTON_MIXED that's supposed to be
> correctly set if the checkbox is in "mixed" state. If that's not returning the
> correct value, we should fix it in Blink.

Ok. If this is what the specs say then the implementation is already present.

I ll just handle IA2_STATE_CHECKABLE in this CL.

[shreeramk, 2014-10-06 15:55:36]

Please review.

Thanks!

[dmazzoni, 2014-10-06 16:05:54]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2014-10-06 16:05:55]

lgtm

[commit-bot: I haz the power, 2014-10-06 16:07:01]

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

[commit-bot: I haz the power, 2014-10-06 18:51:23]

Committed patchset #10 (id:180001) as 9feadabdf96f629e42c4045717f2ef3ea4445733

[commit-bot: I haz the power, 2014-10-06 19:07:15]

Patchset 10 (id:??) landed as
https://crrev.com/ef044b5b099eb06d5fd0557dda25be846d23b272
Cr-Commit-Position: refs/heads/master@{#298287}