Include a role description string in node info for Android accessibility.

Include a role description string in node info for Android accessibility.

BUG=583063
Committed: https://crrev.com/646069ce1ce951dcc989ef64870fe496b6bf7e33
Cr-Commit-Position: refs/heads/master@{#375045}

[dmazzoni, 2016-02-01 20:54:12]

dmazzoni@chromium.org changed reviewers:
+ dtseng@chromium.org

[David Tseng, 2016-02-02 18:38:58]

lgtm

https://codereview.chromium.org/1654193002/diff/40001/content/app/strings/con...
File content/app/strings/content_strings.grd (left):

https://codereview.chromium.org/1654193002/diff/40001/content/app/strings/con...
content/app/strings/content_strings.grd:269: <message name="IDS_AX_ROLE_ADDRESS"
desc="accessibility role description for an address (as in a person's contact
information)">
Was this unused?

https://codereview.chromium.org/1654193002/diff/40001/content/app/strings/con...
File content/app/strings/content_strings.grd (right):

https://codereview.chromium.org/1654193002/diff/40001/content/app/strings/con...
content/app/strings/content_strings.grd:389: <message
name="IDS_AX_ROLE_DEFINITION" desc="accessibility role description for a
definition">
Siilar to name="IDS_AX_ROLE_DESCRIPTION_DETAIL"?

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_android.cc:416: if
(GetRole() == ui::AX_ROLE_HEADING) {
Why not include this in the switch below?

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_android.cc:418: if (level >=
1 && level <= 6) {
Can you check for the case when hierarchical level isn't between 1 and 6? -1?

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_android.h (right):

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_android.h:55: base::string16
GetRoleDescription() const;
const base::string16&

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_cocoa.mm:1016:
IDS_AX_ROLE_CONTENT_INFO));
Got it (regarding removal in grd).

https://codereview.chromium.org/1654193002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java
(left):

https://codereview.chromium.org/1654193002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java:19:
* AccessibilityNodeProvider and delegates its implementation to this object.
Is this comment no longer true?

https://codereview.chromium.org/1654193002/diff/40001/content/public/common/c...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1654193002/diff/40001/content/public/common/c...
content/public/common/content_client.h:113: const base::string16& string0)
const;
Is it worth it to make this take any number of string substitutions?

https://codereview.chromium.org/1654193002/diff/40001/content/test/data/acces...
File content/test/data/accessibility/html/heading-expected-android.txt (right):

https://codereview.chromium.org/1654193002/diff/40001/content/test/data/acces...
content/test/data/accessibility/html/heading-expected-android.txt:2:
++android.view.View role_description='heading level 1' clickable heading
name='Heading 1'
heading level 1 is pretty verbose. How about just heading 1? Does the name come
from the classname or did you use it as the label?

[dmazzoni, 2016-02-03 17:55:06]

dmazzoni@chromium.org changed reviewers:
+ jam@chromium.org

[dmazzoni, 2016-02-03 17:55:07]

+jam for *content_client*

https://codereview.chromium.org/1654193002/diff/40001/content/app/strings/con...
File content/app/strings/content_strings.grd (left):

https://codereview.chromium.org/1654193002/diff/40001/content/app/strings/con...
content/app/strings/content_strings.grd:269: <message name="IDS_AX_ROLE_ADDRESS"
desc="accessibility role description for an address (as in a person's contact
information)">
On 2016/02/02 18:38:57, David Tseng wrote:
> Was this unused?

For some reason we were using this for role="contentinfo". Didn't seem right to
me.

https://codereview.chromium.org/1654193002/diff/40001/content/app/strings/con...
File content/app/strings/content_strings.grd (right):

https://codereview.chromium.org/1654193002/diff/40001/content/app/strings/con...
content/app/strings/content_strings.grd:389: <message
name="IDS_AX_ROLE_DEFINITION" desc="accessibility role description for a
definition">
On 2016/02/02 18:38:57, David Tseng wrote:
> Siilar to name="IDS_AX_ROLE_DESCRIPTION_DETAIL"?

Merged.

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_android.cc (right):

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_android.cc:416: if
(GetRole() == ui::AX_ROLE_HEADING) {
On 2016/02/02 18:38:57, David Tseng wrote:
> Why not include this in the switch below?

Added a comment to clarify. I made this special case just handle the heading
with level case, and the heading without level is handled below now.

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_android.cc:418: if (level >=
1 && level <= 6) {
On 2016/02/02 18:38:57, David Tseng wrote:
> Can you check for the case when hierarchical level isn't between 1 and 6? -1?

I don't think we should crash in that case. The author could specify
role="heading" and forget to add aria-level, it seems better to just output
"heading" rather than make that an error.

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_android.h (right):

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_android.h:55: base::string16
GetRoleDescription() const;
On 2016/02/02 18:38:57, David Tseng wrote:
> const base::string16&

Can't do that because the function doesn't just return a reference to an
existing string of global scope, it returns a string object.

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/1654193002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_cocoa.mm:1016:
IDS_AX_ROLE_CONTENT_INFO));
On 2016/02/02 18:38:57, David Tseng wrote:
> Got it (regarding removal in grd).

Acknowledged.

https://codereview.chromium.org/1654193002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java
(left):

https://codereview.chromium.org/1654193002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/accessibility/LollipopBrowserAccessibilityManager.java:19:
* AccessibilityNodeProvider and delegates its implementation to this object.
On 2016/02/02 18:38:57, David Tseng wrote:
> Is this comment no longer true?

Previously BrowserAccessibilityManager.java did not implement
AccessibilityNodeProvider, but one of its subclasses did. We changed that a
while ago when we dropped ICS support, but this comment remained.

https://codereview.chromium.org/1654193002/diff/40001/content/public/common/c...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/1654193002/diff/40001/content/public/common/c...
content/public/common/content_client.h:113: const base::string16& string0)
const;
On 2016/02/02 18:38:57, David Tseng wrote:
> Is it worth it to make this take any number of string substitutions?

Not sure, I'll ask the owners. I lean towards the YAGNI approach, i.e. don't
generalize until it's actually needed.

https://codereview.chromium.org/1654193002/diff/40001/content/test/data/acces...
File content/test/data/accessibility/html/heading-expected-android.txt (right):

https://codereview.chromium.org/1654193002/diff/40001/content/test/data/acces...
content/test/data/accessibility/html/heading-expected-android.txt:2:
++android.view.View role_description='heading level 1' clickable heading
name='Heading 1'
On 2016/02/02 18:38:57, David Tseng wrote:
> heading level 1 is pretty verbose. How about just heading 1? Does the name
come
> from the classname or did you use it as the label?

The name is from the text of the heading.

I can go with just "heading 1", etc.

[jam, 2016-02-03 22:13:39]

On 2016/02/03 17:55:07, dmazzoni wrote:
> +jam for *content_client*

why not call  l10n_util::GetStringUTF16 to get the string, and then
base::ReplaceStringPlaceholders on that later? i.e. like
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/blin...

[dmazzoni, 2016-02-04 23:25:45]

On 2016/02/03 22:13:39, jam wrote:
> On 2016/02/03 17:55:07, dmazzoni wrote:
> > +jam for *content_client*
> 
> why not call  l10n_util::GetStringUTF16 to get the string, and then
> base::ReplaceStringPlaceholders on that later? i.e. like
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/blin...

Ha! Didn't realize it was that easy. Done.

[jam, 2016-02-05 18:42:34]

On 2016/02/04 23:25:45, dmazzoni wrote:
> On 2016/02/03 22:13:39, jam wrote:
> > On 2016/02/03 17:55:07, dmazzoni wrote:
> > > +jam for *content_client*
> > 
> > why not call  l10n_util::GetStringUTF16 to get the string, and then
> > base::ReplaceStringPlaceholders on that later? i.e. like
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/blin...
> 
> Ha! Didn't realize it was that easy. Done.

not sure if you still need my lgtm or not, but here it is :)

[dmazzoni, 2016-02-05 19:01:26]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-02-05 19:01:27]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1654193002/#ps100001
(title: "Make IDS_AX_ROLE_DEFINITION cross-platform to fix Mac compile")

[commit-bot: I haz the power, 2016-02-05 19:04:04]

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

[commit-bot: I haz the power, 2016-02-05 20:10:47]

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

[commit-bot: I haz the power, 2016-02-05 20:10:49]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dmazzoni, 2016-02-05 20:23:11]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-02-05 20:23:12]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1654193002/#ps120001
(title: "Fix Mac address and contentinfo role description")

[commit-bot: I haz the power, 2016-02-05 20:26:36]

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

[commit-bot: I haz the power, 2016-02-05 21:52:50]

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

[commit-bot: I haz the power, 2016-02-05 21:52:52]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dmazzoni, 2016-02-08 23:20:17]

Description was changed from

==========
Include a role description string in node info for Android accessibility.

This change also adds a new function in ContentClient to get a localized
string with a single replacement.

BUG=583063
==========

to

==========
Include a role description string in node info for Android accessibility.

BUG=583063
==========

[dmazzoni, 2016-02-11 21:53:00]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-02-11 21:53:01]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1654193002/#ps140001
(title: "Update all Android test expectations")

[commit-bot: I haz the power, 2016-02-11 21:54:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1654193002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1654193002/140001

[commit-bot: I haz the power, 2016-02-11 23:55:44]

Description was changed from

==========
Include a role description string in node info for Android accessibility.

BUG=583063
==========

to

==========
Include a role description string in node info for Android accessibility.

BUG=583063
==========

[commit-bot: I haz the power, 2016-02-11 23:55:46]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-16 22:39:36]

Description was changed from

==========
Include a role description string in node info for Android accessibility.

BUG=583063
==========

to

==========
Include a role description string in node info for Android accessibility.

BUG=583063

Committed: https://crrev.com/646069ce1ce951dcc989ef64870fe496b6bf7e33
Cr-Commit-Position: refs/heads/master@{#375045}
==========

[commit-bot: I haz the power, 2016-02-16 22:39:37]

Patchset 8 (id:??) landed as
https://crrev.com/646069ce1ce951dcc989ef64870fe496b6bf7e33
Cr-Commit-Position: refs/heads/master@{#375045}