Properly prioritize the sources of an accessible name on Android.

Properly prioritize the sources of an accessible name on Android.

This spec covers the right order: http://www.w3.org/TR/wai-aria-implementation

Now Android basically matches Windows, the difference is that Android has
to pick just one accessible name while Windows can have both a name and
a description.

There were existing cross-platform tests; I updated the Android
expectations.

BUG=359216
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274017

[aboxhall, 2014-05-30 21:26:21]

lgtm

https://codereview.chromium.org/303113003/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/303113003/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_android.cc:278: else if
(title_elem_id && !name().empty())
I don't follow this logic. Why isn't it getting the name from the title element?
Does name() delegate to it if it exists? If so, perhaps a comment clarifying
that.

https://codereview.chromium.org/303113003/diff/1/content/test/data/accessibil...
File content/test/data/accessibility/button-name-calc-expected-android.txt
(right):

https://codereview.chromium.org/303113003/diff/1/content/test/data/accessibil...
content/test/data/accessibility/button-name-calc-expected-android.txt:7:
android.widget.Button clickable focusable name='LabelledBy4'
I'm confused by this: it looks like description takes precedence, but I see this
element has aria-describedby. How does that work?

[dmazzoni, 2014-05-30 22:03:33]

https://codereview.chromium.org/303113003/diff/1/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/303113003/diff/1/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_android.cc:278: else if
(title_elem_id && !name().empty())
On 2014/05/30 21:26:21, aboxhall wrote:
> I don't follow this logic. Why isn't it getting the name from the title
element?
> Does name() delegate to it if it exists? If so, perhaps a comment clarifying
> that.

I added a comment. I need to clean this up in Blink to make it less confusing.

https://codereview.chromium.org/303113003/diff/1/content/test/data/accessibil...
File content/test/data/accessibility/button-name-calc-expected-android.txt
(right):

https://codereview.chromium.org/303113003/diff/1/content/test/data/accessibil...
content/test/data/accessibility/button-name-calc-expected-android.txt:7:
android.widget.Button clickable focusable name='LabelledBy4'
On 2014/05/30 21:26:21, aboxhall wrote:
> I'm confused by this: it looks like description takes precedence, but I see
this
> element has aria-describedby. How does that work?

Dumb luck? I think the behavior is right (labelledby is more important than
describedby if we have to pick one), and the reason it works is because Blink is
doing the right thing. What we should really do soon is have Blink try to do
less logic and just expose the underlying names, then implement the platform
logic here more clearly.

[dmazzoni, 2014-05-30 22:03:44]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-05-30 22:05:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/303113003/20001

[commit-bot: I haz the power, 2014-05-31 04:31:22]

Change committed as 274017

[DaleCurtis, 2014-05-31 22:08:49]

A revert of this CL has been created in
https://codereview.chromium.org/304293006/ by dalecurtis@chromium.org.

The reason for reverting is: Broke android bots it looks like:
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%....

[tapted, 2014-06-02 01:56:59]

On 2014/05/31 22:08:49, DaleCurtis wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/304293006/ by mailto:dalecurtis@chromium.org.
> 
> The reason for reverting is: Broke android bots it looks like:
>
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%....

Looks like that revert landed in
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...
but did not resolve the issue.

Nothing else in that blamelist is grabbing me yet.

Tests still failing in builds/20698 since builds/20683

DumpAccessibilityTreeTest.AccessibilityButtonNameCalc
DumpAccessibilityTreeTest.AccessibilityInputTextNameCalc
DumpAccessibilityTreeTest.AccessibilityCheckboxNameCalc

Output like:

* Line Expected
- ---- --------
     1 android.webkit.WebView focusable focused scrollable
     2     android.view.View
     3         android.widget.CheckBox checkable clickable focusable
name='Title0'
*    4         android.widget.CheckBox checkable clickable focusable
name='Title1'
     5         android.widget.CheckBox checkable clickable focusable
name='AriaLabel2'
     6         android.widget.CheckBox checkable clickable focusable
name='LabelledBy3'
     7         android.widget.CheckBox checkable clickable focusable
name='LabelledBy4'
*    8         android.widget.CheckBox checkable clickable focusable
name='DescribedBy5'
     9 <-- End-of-file -->

Actual
------
android.webkit.WebView focusable focused scrollable
    android.view.View
        android.widget.CheckBox checkable clickable focusable name='Title0'
        android.widget.CheckBox checkable clickable focusable name='Label1'
        android.widget.CheckBox checkable clickable focusable name='AriaLabel2'
        android.widget.CheckBox checkable clickable focusable name='LabelledBy3'
        android.widget.CheckBox checkable clickable focusable name='LabelledBy4'
        android.widget.CheckBox checkable focusable

[dmazzoni, 2014-06-02 07:10:26]

I think the problem is that extract_build.py is failing, but the bot script
keeps running - so the testing slaves are running an old version of, e.g.
content_browsertests, but loading a new version of the data files - or vice
versa.

I think if extract_build fails, the whole process should stop.

On Sun, Jun 1, 2014 at 6:56 PM, <tapted@chromium.org> wrote:

> On 2014/05/31 22:08:49, DaleCurtis wrote:
>
>> A revert of this CL has been created in
>> https://codereview.chromium.org/304293006/ by mailto:
>> dalecurtis@chromium.org.
>>
>
>  The reason for reverting is: Broke android bots it looks like:
>>
>
> http://build.chromium.org/p/chromium.linux/builders/
> Android%20Tests%20%28dbg%29/builds/20683.
>
> Looks like that revert landed in
> http://build.chromium.org/p/chromium.linux/builders/
> Android%20Tests%20%28dbg%29/builds/20686
> but did not resolve the issue.
>
> Nothing else in that blamelist is grabbing me yet.
>
> Tests still failing in builds/20698 since builds/20683
>
> DumpAccessibilityTreeTest.AccessibilityButtonNameCalc
> DumpAccessibilityTreeTest.AccessibilityInputTextNameCalc
> DumpAccessibilityTreeTest.AccessibilityCheckboxNameCalc
>
> Output like:
>
> * Line Expected
> - ---- --------
>      1 android.webkit.WebView focusable focused scrollable
>      2     android.view.View
>      3         android.widget.CheckBox checkable clickable focusable
> name='Title0'
> *    4         android.widget.CheckBox checkable clickable focusable
> name='Title1'
>      5         android.widget.CheckBox checkable clickable focusable
> name='AriaLabel2'
>      6         android.widget.CheckBox checkable clickable focusable
> name='LabelledBy3'
>      7         android.widget.CheckBox checkable clickable focusable
> name='LabelledBy4'
> *    8         android.widget.CheckBox checkable clickable focusable
> name='DescribedBy5'
>      9 <-- End-of-file -->
>
> Actual
> ------
>
> android.webkit.WebView focusable focused scrollable
>     android.view.View
>         android.widget.CheckBox checkable clickable focusable name='Title0'
>         android.widget.CheckBox checkable clickable focusable name='Label1'
>         android.widget.CheckBox checkable clickable focusable
> name='AriaLabel2'
>         android.widget.CheckBox checkable clickable focusable
> name='LabelledBy3'
>         android.widget.CheckBox checkable clickable focusable
> name='LabelledBy4'
>         android.widget.CheckBox checkable focusable
>
> https://codereview.chromium.org/303113003/
>

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

[dmazzoni, 2014-06-04 17:30:56]

The Android builder seems to be working again. I'm going to try to re-land.

[dmazzoni, 2014-06-04 17:31:32]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-06-04 17:32:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/303113003/20001

[commit-bot: I haz the power, 2014-06-04 17:34:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-04 17:34:32]

Failed to apply patch for
content/browser/accessibility/browser_accessibility_android.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/accessibility/browser_accessibility_android.cc
  Hunk #1 FAILED at 268.
  1 out of 1 hunk FAILED -- saving rejects to file
content/browser/accessibility/browser_accessibility_android.cc.rej

Patch:       content/browser/accessibility/browser_accessibility_android.cc
Index: content/browser/accessibility/browser_accessibility_android.cc
diff --git a/content/browser/accessibility/browser_accessibility_android.cc
b/content/browser/accessibility/browser_accessibility_android.cc
index
cec0a664cdab41d4359171680289c0390aaeb6fe..8a8514a0d80a84db258d2a05348513a1e9ac59fc
100644
--- a/content/browser/accessibility/browser_accessibility_android.cc
+++ b/content/browser/accessibility/browser_accessibility_android.cc
@@ -268,12 +268,29 @@ base::string16 BrowserAccessibilityAndroid::GetText()
const {
     return base::string16();
   }
 
+  // See comment in browser_accessibility_win.cc for details.
+  // The difference here is that we can only expose one accessible
+  // name on Android, not 2 or 3 like on Windows or Mac.
+  //
+  // The basic rule is: prefer description (aria-labelledby or aria-label),
+  // then help (title), then name (inner text), then value (control value).
+  // However, if title_elem_id is set, that means there's a label element
+  // supplying the name and then name takes precedence over help.
+  // TODO(dmazzoni): clean this up by providing more granular labels in
+  // Blink, making the platform-specific mapping to accessible text simpler.
   base::string16 description = GetString16Attribute(ui::AX_ATTR_DESCRIPTION);
+  base::string16 help = GetString16Attribute(ui::AX_ATTR_HELP);
+  int title_elem_id = GetIntAttribute(
+      ui::AX_ATTR_TITLE_UI_ELEMENT);
   base::string16 text;
-  if (!name().empty())
-    text = base::UTF8ToUTF16(name());
-  else if (!description.empty())
+  if (!description.empty())
     text = description;
+  else if (title_elem_id && !name().empty())
+    text = base::UTF8ToUTF16(name());
+  else if (!help.empty())
+    text = help;
+  else if (!name().empty())
+    text = base::UTF8ToUTF16(name());
   else if (!value().empty())
     text = base::UTF8ToUTF16(value());

[dmazzoni, 2014-06-04 17:52:54]

nvm, this was reverted and then re-landed already