Add DumpAccessibilityTree tests (11 of 20)

Add DumpAccessibilityTree tests (11 of 20)

HTML Tag: time, colgroup, var, link, area
ARIA Role: textbox, search
ARIA state/property: aria-valuenow, aria-busy

Already present: aria-flowto, aria-invalid, aria-pressed

BUG=170583
Committed: https://crrev.com/c8d59974c578e1d6a88b6948fec86536ddb43870
Cr-Commit-Position: refs/heads/master@{#302989}

[shreeramk, 2014-10-30 00:19:13]

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

[shreeramk, 2014-10-30 00:19:14]

Please review! 
Thanks

[dmazzoni, 2014-10-30 17:16:44]

Just a heads-up, I'm about to land a change that slightly modifies the result of
ARIA live region tests like aria-busy.

https://codereview.chromium.org/689653002/diff/40001/content/test/data/access...
File content/test/data/accessibility/area-expected-android.txt (right):

https://codereview.chromium.org/689653002/diff/40001/content/test/data/access...
content/test/data/accessibility/area-expected-android.txt:4: android.view.View
clickable editable_text name='pipe1'
editable_text seems wrong here, can you figure out where that came from?

https://codereview.chromium.org/689653002/diff/40001/content/test/data/access...
File content/test/data/accessibility/area-expected-mac.txt (right):

https://codereview.chromium.org/689653002/diff/40001/content/test/data/access...
content/test/data/accessibility/area-expected-mac.txt:4: AXLink
AXRoleDescription='link'
Shouldn't the link title be here somewhere? If not the AXTitle, maybe the
AXDescription?

[shreeramk, 2014-10-30 17:31:32]

Ok. I ll wait for your cl to land.

https://codereview.chromium.org/689653002/diff/40001/content/test/data/access...
File content/test/data/accessibility/area-expected-android.txt (right):

https://codereview.chromium.org/689653002/diff/40001/content/test/data/access...
content/test/data/accessibility/area-expected-android.txt:4: android.view.View
clickable editable_text name='pipe1'
On 2014/10/30 17:16:44, dmazzoni wrote:
> editable_text seems wrong here, can you figure out where that came from?

Yes this is wrong. This issue is seen on windows platform as well in many cases.

IA2_EDITABLE... is getting exposed with many elements unnecessary.I m looking
into this bug. Will submit a patch on monday.

[dmazzoni, 2014-10-30 17:38:54]

On 2014/10/30 17:31:32, shreeramk_OOO_2nd_Nov wrote:
> Ok. I ll wait for your cl to land.
> 
>
https://codereview.chromium.org/689653002/diff/40001/content/test/data/access...
> File content/test/data/accessibility/area-expected-android.txt (right):
> 
>
https://codereview.chromium.org/689653002/diff/40001/content/test/data/access...
> content/test/data/accessibility/area-expected-android.txt:4: android.view.View
> clickable editable_text name='pipe1'
> On 2014/10/30 17:16:44, dmazzoni wrote:
> > editable_text seems wrong here, can you figure out where that came from?
> 
> Yes this is wrong. This issue is seen on windows platform as well in many
cases.
> 
> IA2_EDITABLE... is getting exposed with many elements unnecessary.I m looking
> into this bug. Will submit a patch on monday.

Ah, it's that bug. Yeah, the issue seems to be that WebKit was designed with Mac
in mind and it doesn't use "readonly" the same way Windows does at all. I think
Blink should expose a isInContentEditable attribute that directly maps to
IA2_EDITABLE on Windows.

[shreeramk, 2014-10-31 00:45:04]

On 2014/10/30 17:38:54, dmazzoni wrote:
> On 2014/10/30 17:31:32, shreeramk_OOO_2nd_Nov wrote:
> > Ok. I ll wait for your cl to land.
> > 
> >
>
https://codereview.chromium.org/689653002/diff/40001/content/test/data/access...
> > File content/test/data/accessibility/area-expected-android.txt (right):
> > 
> >
>
https://codereview.chromium.org/689653002/diff/40001/content/test/data/access...
> > content/test/data/accessibility/area-expected-android.txt:4:
android.view.View
> > clickable editable_text name='pipe1'
> > On 2014/10/30 17:16:44, dmazzoni wrote:
> > > editable_text seems wrong here, can you figure out where that came from?
> > 
> > Yes this is wrong. This issue is seen on windows platform as well in many
> cases.
> > 
> > IA2_EDITABLE... is getting exposed with many elements unnecessary.I m
looking
> > into this bug. Will submit a patch on monday.
> 
> Ah, it's that bug. Yeah, the issue seems to be that WebKit was designed with
Mac
> in mind and it doesn't use "readonly" the same way Windows does at all. I
think
> Blink should expose a isInContentEditable attribute that directly maps to
> IA2_EDITABLE on Windows.

Yes.

I have seen bugs like this on windows...both READONLY IA2_EDITABLE... would be
exposed on the same elements.

For ex: Have a look on the last comment by me on this cl
https://codereview.chromium.org/631183002/

<html>
<body>
  <input aria-readonly="false" tabindex="0" value="Editable text">
  <input aria-readonly="true" tabindex="1" value="Uneditable text">
  <input readonly tabindex="1" value="Uneditable text">
</body>
</html>

The AX tree on win is
ROLE_SYSTEM_DOCUMENT READONLY FOCUSABLE
    IA2_ROLE_SECTION READONLY
        ROLE_SYSTEM_TEXT value='Editable text' FOCUSABLE IA2_STATE_EDITABLE
IA2_STATE_SELECTABLE_TEXT IA2_STATE_SINGLE_LINE
        ROLE_SYSTEM_TEXT value='Uneditable text' READONLY FOCUSABLE
IA2_STATE_EDITABLE IA2_STATE_SELECTABLE_TEXT IA2_STATE_SINGLE_LINE
        ROLE_SYSTEM_TEXT value='Uneditable text' FOCUSABLE IA2_STATE_EDITABLE
IA2_STATE_SELECTABLE_TEXT IA2_STATE_SINGLE_LINE

If you see, when aria-readonly="true" then both READONLY and IA2_STATE_EDITABLE
are getting exposed, which is totally wrong.

Also for html input attribute readonly is not respected in the AX tree, its
giving IA2_STATE_EDITABLE instead of READONLY.

[dmazzoni, 2014-10-31 07:41:48]

On Thu, Oct 30, 2014 at 10:16 AM, <dmazzoni@chromium.org> wrote:

> Just a heads-up, I'm about to land a change that slightly modifies the
> result of
> ARIA live region tests like aria-busy.
>

Done:

https://codereview.chromium.org/682583002/
Patchset 4 (id:??) landed as
https://crrev.com/9cabe256d4cff48ffba428c872c750f6b9000ee3
Cr-Commit-Position: refs/heads/master@{#302236}

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shreeramk, 2014-11-01 11:34:00]

Comments incorporated.

PTAL!

Thanks.

[shreeramk, 2014-11-05 02:55:10]

On 2014/11/01 11:34:00, shreeramk_OOO_2nd_Nov wrote:
> Comments incorporated.
> 
> PTAL!
> 
> Thanks.

@Dominic: PTAL. Thanks!

[dmazzoni, 2014-11-05 07:10:49]

lgtm

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

https://codereview.chromium.org/689653002/diff/80001/content/test/data/access...
content/test/data/accessibility/aria-pressed.html:15: <div tabindex=4
role="button" aria-pressed="undefined">Toggle button undefined</div>
When the spec says that aria-pressed is undefined, I don't think they mean
aria-pressed="undefined", they mean that the aria-pressed attribute is not
present - that case is handled by the first one, a regular button.

Right now our logic is that if the aria-pressed attribute is present, it's a
toggle button, even if it's not true, false, or mixed - that's probably wrong.

[shreeramk, 2014-11-05 07:25:17]

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

https://codereview.chromium.org/689653002/diff/80001/content/test/data/access...
content/test/data/accessibility/aria-pressed.html:15: <div tabindex=4
role="button" aria-pressed="undefined">Toggle button undefined</div>
On 2014/11/05 07:10:49, dmazzoni wrote:
> When the spec says that aria-pressed is undefined, I don't think they mean
> aria-pressed="undefined", they mean that the aria-pressed attribute is not
> present - that case is handled by the first one, a regular button.
> 
> Right now our logic is that if the aria-pressed attribute is present, it's a
> toggle button, even if it's not true, false, or mixed - that's probably wrong.

But Why values other than true, false or mixed will be assigned to aria-pressed?

Are you talking about a content of this type??

<div role="button" aria-pressed="ooo">Toggle button unknown</div>

Following was the AX output:
IA2_ROLE_TOGGLE_BUTTON name='Toggle button unknown' FOCUSABLE xml-roles:button
checkable:true

[dmazzoni, 2014-11-05 07:55:28]

On Tue, Nov 4, 2014 at 11:25 PM, <shreeram.k@samsung.com> wrote:

> But Why values other than true, false or mixed will be assigned to
> aria-pressed?
>
> Are you talking about a content of this type??
>
> <div role="button" aria-pressed="ooo">Toggle button unknown</div>
>
> Following was the AX output:
> IA2_ROLE_TOGGLE_BUTTON name='Toggle button unknown' FOCUSABLE
> xml-roles:button checkable:true
>

Yes. It should probably not be interpreted as a toggle button in this case.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shreeramk, 2014-11-05 09:16:16]

On 2014/11/05 07:55:28, dmazzoni wrote:
> On Tue, Nov 4, 2014 at 11:25 PM, <mailto:shreeram.k@samsung.com> wrote:
> 
> > But Why values other than true, false or mixed will be assigned to
> > aria-pressed?
> >
> > Are you talking about a content of this type??
> >
> > <div role="button" aria-pressed="ooo">Toggle button unknown</div>
> >
> > Following was the AX output:
> > IA2_ROLE_TOGGLE_BUTTON name='Toggle button unknown' FOCUSABLE
> > xml-roles:button checkable:true
> >
> 
> Yes. It should probably not be interpreted as a toggle button in this case.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

But the spec says
IAccessible2: If aria-pressed is not "undefined", it SHOULD be exposed as
IA2_ROLE_TOGGLE_BUTTON

Is undefined here means, any value other than true, false, or mixed?

[dmazzoni, 2014-11-05 16:41:01]

On Wed, Nov 5, 2014 at 1:16 AM, <shreeram.k@samsung.com> wrote:

> But the spec says
> IAccessible2: If aria-pressed is not "undefined", it SHOULD be exposed as
> IA2_ROLE_TOGGLE_BUTTON
>
> Is undefined here means, any value other than true, false, or mixed?
>
> OK, that makes sense. I'm not 100% sure what the spec intended but that
seems just as reasonable.

How about changing the test to be something like:

aria-pressed="garbage"

or something else obviously wrong? Even if it was correct for
aria-pressed="undefined" it looks plausibly correct and I don't want the
test to be confusing.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shreeramk, 2014-11-05 17:55:37]

On 2014/11/05 16:41:01, dmazzoni wrote:
> On Wed, Nov 5, 2014 at 1:16 AM, <mailto:shreeram.k@samsung.com> wrote:
> 
> > But the spec says
> > IAccessible2: If aria-pressed is not "undefined", it SHOULD be exposed as
> > IA2_ROLE_TOGGLE_BUTTON
> >
> > Is undefined here means, any value other than true, false, or mixed?
> >
> > OK, that makes sense. I'm not 100% sure what the spec intended but that
> seems just as reasonable.
> 
> How about changing the test to be something like:
> 
> aria-pressed="garbage"
> 
> or something else obviously wrong? Even if it was correct for
> aria-pressed="undefined" it looks plausibly correct and I don't want the
> test to be confusing.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

For this change I have to change definition of GetAriaTriState(), but then it
cannot be used anywhere else since with this change it would be defined
according to aria pressed for aria role button.

bool BrowserAccessibility::GetAriaTristate(
    const char* html_attr,
    bool* is_defined,
    bool* is_mixed) const {
  *is_defined = false;
  *is_mixed = false;

  base::string16 value;
  if (!GetHtmlAttribute(html_attr, &value) ||
      value.empty() ||
      EqualsASCII(value, "undefined") ||
+     !EqualsASCII(value, "true") ||
+     !EqualsASCII(value, "false") ||
+     !EqualsASCII(value, "mixed") ||) {
    return false;  // Not set (and *is_defined is also false)
  }

  *is_defined = true;

  if (EqualsASCII(value, "true"))
    return true;

  if (EqualsASCII(value, "mixed"))
    *is_mixed = true;

  return false;  // Not set
}

[shreeramk, 2014-11-05 19:07:10]

@Dominic: I would like to fix this in a fresh CL. The changes needs to be done
on all three platforms. I ll take care of this in this crbug.com/171625.

In this CL, I am not including the case of aria-pressed="garbage". Also,
aria-pressed="undefined" is not working working correctly on all three
platforms.

[dmazzoni, 2014-11-06 07:13:30]

lgtm

[shreeramk, 2014-11-06 07:46:26]

The CQ bit was checked by shreeram.k@samsung.com

[commit-bot: I haz the power, 2014-11-06 07:47:36]

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

[commit-bot: I haz the power, 2014-11-06 08:58:29]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-06 08:59:06]

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