Improve accessible name calculation on Windows.

Improve accessible name calculation on Windows.

This depends on: http://codereview.chromium.org/10662003/

Based on the HTML to Platform Accessibility APIs Implementation Guide
(http://www.w3.org/TR/html-aapi/), add some new tests to assert the
correct behavior for computing the text alternative for a
checkbox, button, and text field, then fix the implementation to
match the test.

BUG=124314
TBR=dtseng

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

[dmazzoni, 2012-07-30 07:37:30]

Alice, could you do the primary review for this? In particular I'd appreciate it
if you could double-check that the tests I wrote here match what the
Accessibility implementation guide says we should be doing, to the best of your
understanding.

Thanks!

[aboxhall, 2012-08-01 01:56:56]

I've tried to review the tests as closely as possible, although as a suggestion
I think they would be easier to parse by eye if each text alternative attribute
had a number associated with it which matched the ID of the element, e.g.:

<input id="c2" type="text" title="Title2" aria-label="AriaLabel2">
<label for="c2">Label2</label>

I did get lost at some points trying to figure out which line matched up to
which.

I didn't feel like I had a good enough understanding of
browser_accessibility_win.cc to comment on that code; let me know if you'd like
me to review that more thoroughly and I'll spend some time getting to know how
it works better.

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
File content/test/data/accessibility/button-name-calc-expected-win.txt (right):

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/button-name-calc-expected-win.txt:4:
ROLE_SYSTEM_PUSHBUTTON name='InnerText' FOCUSABLE description='Title'
Is using the title as the description the de facto standard? It's not mentioned
in the spec: "An accessible description may be computed by concatenating the
text alternatives for nodes referenced by an aria-describedby attribute on the
current node."

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
File content/test/data/accessibility/button-name-calc.html (right):

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/button-name-calc.html:7: <button id="c1"
title="Title">InnerText</button>
Would it be worthwhile having a more complicated example of content, for example
an image with an alt text, or an element with an aria-labelledby which refers to
a hidden element?

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
File content/test/data/accessibility/checkbox-name-calc.html (right):

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/checkbox-name-calc.html:14: 
You might want to add a test for getting the name from contents, similar to the
button role, since checkbox has name from: contents as well.

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
File content/test/data/accessibility/input-text-name-calc-expected-win.txt
(right):

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/input-text-name-calc-expected-win.txt:7:
ROLE_SYSTEM_TEXT name='Placeholder' FOCUSABLE description=''
Placeholder is not mentioned in the accessible name computation either, but I
agree it makes sense to use it in this way (including combining it with the
title).

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/input-text-name-calc-expected-win.txt:12:
ROLE_SYSTEM_TEXT name='LabelledBy' FOCUSABLE description='DescribedBy
Placeholder'
I'm not sure we want to include the placeholder if there is an aria-describedby
attribute. I would expect developers to include all necessary information in the
description. Do other browsers do it this way?

[dmazzoni, 2012-08-01 03:22:33]

On 2012/08/01 01:56:56, aboxhall wrote:
> I've tried to review the tests as closely as possible, although as a
suggestion
> I think they would be easier to parse by eye if each text alternative
attribute
> had a number associated with it which matched the ID of the element, e.g.:

Good idea! Will do shortly.

> I didn't feel like I had a good enough understanding of
> browser_accessibility_win.cc to comment on that code; let me know if you'd
like
> me to review that more thoroughly and I'll spend some time getting to know how
> it works better.

How about this: review the code to make sure each individual line of code is
readable, safe, and consistent with the style guide. As for the overall logic, I
care more about making sure the tests are correct; we can then improve the logic
knowing that the behavior is still correct.

[dmazzoni, 2012-08-01 03:22:41]

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
File content/test/data/accessibility/button-name-calc-expected-win.txt (right):

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/button-name-calc-expected-win.txt:4:
ROLE_SYSTEM_PUSHBUTTON name='InnerText' FOCUSABLE description='Title'
On 2012/08/01 01:56:57, aboxhall wrote:
> Is using the title as the description the de facto standard? It's not
mentioned
> in the spec: "An accessible description may be computed by concatenating the
> text alternatives for nodes referenced by an aria-describedby attribute on the
> current node."

I was following: http://www.w3.org/TR/html-aapi/#calc

For example: "If the control has a non-empty associated label element and a
non-empty title attribute, use the content of the associated label element as
the control's accessible name. Use the content of the title attribute as the
control's accessible description."

The way I'm interpreting it, the title is the primary text alternative, and if
there's a 2nd-priority text alternative string, it goes in the description,
UNLESS you use aria-describedby, which overrides the description.

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
File content/test/data/accessibility/button-name-calc.html (right):

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/button-name-calc.html:7: <button id="c1"
title="Title">InnerText</button>
On 2012/08/01 01:56:57, aboxhall wrote:
> Would it be worthwhile having a more complicated example of content, for
example
> an image with an alt text, or an element with an aria-labelledby which refers
to
> a hidden element?

Yes! But maybe for the next test? This is complicated enough and the only thing
I wanted to cover in this test is the relative priorities of text alternatives,
and what MSAA attributes they get assigned to.

Let's do another set of tests on concatenating accessible titles inside inner
text, etc.

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
File content/test/data/accessibility/checkbox-name-calc.html (right):

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/checkbox-name-calc.html:14: 
On 2012/08/01 01:56:57, aboxhall wrote:
> You might want to add a test for getting the name from contents, similar to
the
> button role, since checkbox has name from: contents as well.

I could be wrong but I think that's only for an ARIA checkbox.

I'm happy to do that but that should be a different test. I don't think an
<input type="checkbox"> can have inner text.

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
File content/test/data/accessibility/input-text-name-calc-expected-win.txt
(right):

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/input-text-name-calc-expected-win.txt:7:
ROLE_SYSTEM_TEXT name='Placeholder' FOCUSABLE description=''
On 2012/08/01 01:56:57, aboxhall wrote:
> Placeholder is not mentioned in the accessible name computation either, but I
> agree it makes sense to use it in this way (including combining it with the
> title).

It's here:
http://www.w3.org/TR/html-aapi/#calc

Were you looking at the ARIA platform implementation guide? I looked at that one
too, but the AAPI seemed to be a little more specific about how to apply it to
platform-specific attributes.

It's quite possible they conflict. :(

[dmazzoni, 2012-08-04 06:34:05]

Ready for another look?

On 2012/08/01 01:56:56, aboxhall wrote:
> I've tried to review the tests as closely as possible, although as a
suggestion
> I think they would be easier to parse by eye if each text alternative
attribute
> had a number associated with it which matched the ID of the element, e.g.:
> 
> <input id="c2" type="text" title="Title2" aria-label="AriaLabel2">
> <label for="c2">Label2</label>
> 
> I did get lost at some points trying to figure out which line matched up to
> which.

This is now done, I hope it helps.

- Dominic

[aboxhall, 2012-08-06 01:55:49]

Thanks for adding the line numbers, that helps.

Sorry this took so long; we had no reliable connectivity at our offsite last
week, so there was no way to sneakily do work!

http://codereview.chromium.org/10823073/diff/2001/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/10823073/diff/2001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:2739: name_ =
description;
I'm confused by this line. Is name_ always empty at this point?

http://codereview.chromium.org/10823073/diff/2001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:2765: description =
description + L" " + placeholder;
As I said elsewhere, I'm not sure this is the best approach; I think placeholder
should be ignored if the description has come from aria-describedby (which is
probably a bit of a pain to work out, admittedly).

http://codereview.chromium.org/10823073/diff/2001/content/browser/accessibili...
File content/browser/accessibility/dump_accessibility_tree_helper_win.cc
(right):

http://codereview.chromium.org/10823073/diff/2001/content/browser/accessibili...
content/browser/accessibility/dump_accessibility_tree_helper_win.cc:49: string16
help;
Where/how is this used?

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
File content/test/data/accessibility/button-name-calc-expected-win.txt (right):

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/button-name-calc-expected-win.txt:4:
ROLE_SYSTEM_PUSHBUTTON name='InnerText' FOCUSABLE description='Title'
On 2012/08/01 03:22:41, Dominic Mazzoni wrote:
> On 2012/08/01 01:56:57, aboxhall wrote:
> > Is using the title as the description the de facto standard? It's not
> mentioned
> > in the spec: "An accessible description may be computed by concatenating the
> > text alternatives for nodes referenced by an aria-describedby attribute on
the
> > current node."
> 
> I was following: http://www.w3.org/TR/html-aapi/#calc
> 
> For example: "If the control has a non-empty associated label element and a
> non-empty title attribute, use the content of the associated label element as
> the control's accessible name. Use the content of the title attribute as the
> control's accessible description."
> 
> The way I'm interpreting it, the title is the primary text alternative, and if
> there's a 2nd-priority text alternative string, it goes in the description,
> UNLESS you use aria-describedby, which overrides the description.

Right, now I understand; I hadn't seen that spec before.

Annoyingly, the spec is kind of ambiguous as to what to do with aria-describedby
in the case where there is no aria-label or aria-labelledby, but I think your
approach is reasonable; i.e. aria-describedby overrides any other attributes. I
notice there is a "to do" under "Accessible Description Calculation", too.

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
File content/test/data/accessibility/button-name-calc.html (right):

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/button-name-calc.html:7: <button id="c1"
title="Title">InnerText</button>
On 2012/08/01 03:22:41, Dominic Mazzoni wrote:
> On 2012/08/01 01:56:57, aboxhall wrote:
> > Would it be worthwhile having a more complicated example of content, for
> example
> > an image with an alt text, or an element with an aria-labelledby which
refers
> to
> > a hidden element?
> 
> Yes! But maybe for the next test? This is complicated enough and the only
thing
> I wanted to cover in this test is the relative priorities of text
alternatives,
> and what MSAA attributes they get assigned to.
> 
> Let's do another set of tests on concatenating accessible titles inside inner
> text, etc.

Makes sense, and I've certainly caused enough delay on this change set.

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
File content/test/data/accessibility/checkbox-name-calc.html (right):

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/checkbox-name-calc.html:14: 
On 2012/08/01 03:22:41, Dominic Mazzoni wrote:
> On 2012/08/01 01:56:57, aboxhall wrote:
> > You might want to add a test for getting the name from contents, similar to
> the
> > button role, since checkbox has name from: contents as well.
> 
> I could be wrong but I think that's only for an ARIA checkbox.
> 
> I'm happy to do that but that should be a different test. I don't think an
> <input type="checkbox"> can have inner text.

Oh, oops, I misread type="checkbox" for role="checkbox". Sorry!

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/checkbox-name-calc.html:15: <p
aria-label="@NO_DUMP">
Just for my understanding for writing tests in future: I assume this means that
the dumped accessibility tree should ignore this node and its children. Is this
the only way to do this, or could it be done with a dummy attribute e.g. <p
dump="@NO_DUMP">?

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
File content/test/data/accessibility/input-text-name-calc-expected-win.txt
(right):

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/input-text-name-calc-expected-win.txt:7:
ROLE_SYSTEM_TEXT name='Placeholder' FOCUSABLE description=''
On 2012/08/01 03:22:41, Dominic Mazzoni wrote:
> On 2012/08/01 01:56:57, aboxhall wrote:
> > Placeholder is not mentioned in the accessible name computation either, but
I
> > agree it makes sense to use it in this way (including combining it with the
> > title).
> 
> It's here:
> http://www.w3.org/TR/html-aapi/#calc
> 
> Were you looking at the ARIA platform implementation guide? I looked at that
one
> too, but the AAPI seemed to be a little more specific about how to apply it to
> platform-specific attributes.
> 
> It's quite possible they conflict. :(

Yep, I was looking at the ARIA guide. They don't quite conflict (both agree on
the label > title > placeholder precedence order), but they are a bit ambiguous
taken together where the description is concerned.

http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
content/test/data/accessibility/input-text-name-calc-expected-win.txt:12:
ROLE_SYSTEM_TEXT name='LabelledBy' FOCUSABLE description='DescribedBy
Placeholder'
On 2012/08/01 01:56:57, aboxhall wrote:
> I'm not sure we want to include the placeholder if there is an
aria-describedby
> attribute. I would expect developers to include all necessary information in
the
> description. Do other browsers do it this way?

I still think this shouldn't include the placeholder, at least going by my
reading of the specs. If there is a precedent for this, and you think this is
better, then I guess an argument could be made, but based on the spec I don't
see why you'd include placeholder, and especially not placeholder but not title,
given title has a higher precedence.

[dmazzoni, 2012-08-06 05:56:02]

On 2012/08/06 01:55:49, aboxhall wrote:
> Thanks for adding the line numbers, that helps.

Great!

>
http://codereview.chromium.org/10823073/diff/2001/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_win.cc:2739: name_ =
> description;
> I'm confused by this line. Is name_ always empty at this point?

No, but description always takes precedence.

My comment was a bit wrong, I rewrote it and added more detail.

>
http://codereview.chromium.org/10823073/diff/2001/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_win.cc:2765: description =
> description + L" " + placeholder;
> As I said elsewhere, I'm not sure this is the best approach; I think
placeholder
> should be ignored if the description has come from aria-describedby (which is
> probably a bit of a pain to work out, admittedly).

I thought that's what the doc said, but I might be wrong. I agree it's a little
weird.

For now, I just got rid of that part of the logic and any tests that would have
resulted in
placeholder being combined with another attribute.

Now placeholder is still used if there's nothing else - but not if there's
another name
and description.

>
http://codereview.chromium.org/10823073/diff/2001/content/browser/accessibili...
> File content/browser/accessibility/dump_accessibility_tree_helper_win.cc
> (right):
> 
>
http://codereview.chromium.org/10823073/diff/2001/content/browser/accessibili...
> content/browser/accessibility/dump_accessibility_tree_helper_win.cc:49:
string16
> help;
> Where/how is this used?

Hopefully this is clarified better now. "Help" is what WebKit calls the value of
"title".

>
http://codereview.chromium.org/10823073/diff/2001/content/test/data/accessibi...
> content/test/data/accessibility/checkbox-name-calc.html:15: <p
> aria-label="@NO_DUMP">
> Just for my understanding for writing tests in future: I assume this means
that
> the dumped accessibility tree should ignore this node and its children. Is
this
> the only way to do this, or could it be done with a dummy attribute e.g. <p
> dump="@NO_DUMP">?

It would have to show up in dumped accessibility tree, so you couldn't make up
an
html element that has nothing to do with accessibility, you'd have to use any
html attribute that ends up being part of the accessibility tree. You could
conceivably use title, aria-label, aria-labelledby, aria-describedby,
placeholder, etc.

> I still think this shouldn't include the placeholder, at least going by my
> reading of the specs. If there is a precedent for this, and you think this is
> better, then I guess an argument could be made, but based on the spec I don't
> see why you'd include placeholder, and especially not placeholder but not
title,
> given title has a higher precedence.

Sounds good. It's an obscure corner case, I'd rather focus our attention on
cleaning
up this logic for the important cases than getting this obscure case right.

[aboxhall, 2012-08-06 06:11:53]

LGTM

Please put the aria-describedby test cases back in input-text-name-calc as
discussed :)

[commit-bot: I haz the power, 2012-08-06 06:55:36]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[commit-bot: I haz the power, 2012-08-06 06:59:17]

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

[commit-bot: I haz the power, 2012-08-06 07:22:22]

Try job failure for 10823073-8017 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-06 07:54:11]

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

[commit-bot: I haz the power, 2012-08-06 08:13:59]

Try job failure for 10823073-8017 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...