Support accessible inline text boxes.

This adds support for the InlineTextBox role in the AX tree,
and its attributes.

Depends on this Blink change that adds InlineTextBox support.
https://codereview.chromium.org/23983002/

This change only adds support and makes minimal changes to
allow tests to pass. Other changes will take advantage of the
inline text boxes to compute boundsForRange, etc.

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

[David Tseng, 2013-10-17 22:56:29]

https://codereview.chromium.org/25943003/diff/1/content/browser/accessibility...
File content/browser/accessibility/accessibility_tree_formatter.cc (right):

https://codereview.chromium.org/25943003/diff/1/content/browser/accessibility...
content/browser/accessibility/accessibility_tree_formatter.cc:69: if
(node.IsLeaf())
Btw, isLeaf worries me because it's often possible to get focus set on a
descendant of a leaf (depending on how we define leaves). We should be careful
when stating what is a leaf and what isn't.

https://codereview.chromium.org/25943003/diff/1/content/common/accessibility_...
File content/common/accessibility_node_data.h (right):

https://codereview.chromium.org/25943003/diff/1/content/common/accessibility_...
content/common/accessibility_node_data.h:156: // For inline text.
These attributes are not very obvious; perhaps add a little more detail about
what they are.

https://codereview.chromium.org/25943003/diff/1/content/renderer/accessibilit...
File content/renderer/accessibility/accessibility_node_serializer.cc (right):

https://codereview.chromium.org/25943003/diff/1/content/renderer/accessibilit...
content/renderer/accessibility/accessibility_node_serializer.cc:177:
dst->AddIntListAttribute(dst->ATTR_WORD_ENDS, word_ends);
It seems like a good idea to perhaps have a few dump accessibility tree tests to
validate these attributes on the browser end.

[dmazzoni, 2013-10-21 17:26:56]

https://codereview.chromium.org/25943003/diff/1/content/browser/accessibility...
File content/browser/accessibility/accessibility_tree_formatter.cc (right):

https://codereview.chromium.org/25943003/diff/1/content/browser/accessibility...
content/browser/accessibility/accessibility_tree_formatter.cc:69: if
(node.IsLeaf())
On 2013/10/17 22:56:29, David Tseng wrote:
> Btw, isLeaf worries me because it's often possible to get focus set on a
> descendant of a leaf (depending on how we define leaves). We should be careful
> when stating what is a leaf and what isn't.

My intent is that this is only used to really mean a leaf - we literally don't
expose any children of this node to the platform. On Android, we sometimes prune
children of a focusable node that has a title, but we're careful to only do this
when it doesn't itself have any focusable children - so there isn't any worry
about this.

Would some documentation help, or can you think of a better name?

https://codereview.chromium.org/25943003/diff/1/content/common/accessibility_...
File content/common/accessibility_node_data.h (right):

https://codereview.chromium.org/25943003/diff/1/content/common/accessibility_...
content/common/accessibility_node_data.h:156: // For inline text.
On 2013/10/17 22:56:29, David Tseng wrote:
> These attributes are not very obvious; perhaps add a little more detail about
> what they are.

Done.

https://codereview.chromium.org/25943003/diff/1/content/renderer/accessibilit...
File content/renderer/accessibility/accessibility_node_serializer.cc (right):

https://codereview.chromium.org/25943003/diff/1/content/renderer/accessibilit...
content/renderer/accessibility/accessibility_node_serializer.cc:177:
dst->AddIntListAttribute(dst->ATTR_WORD_ENDS, word_ends);
On 2013/10/17 22:56:29, David Tseng wrote:
> It seems like a good idea to perhaps have a few dump accessibility tree tests
to
> validate these attributes on the browser end.

Agreed about testing these.

I don't want to use dump accessibility tree tests because the only native APIs
that make use of these attributes are "parameterized" apis, like boundsForRange,
lineForIndex, etc. - which aren't very easy to express in DumpAccessibilityTree
tests.

Perhaps we need to work on a new type of test that's almost as easy to write as
a DumpAccessibilityTree test but lets us query the resulting tree with different
arguments.

Or maybe we need a DumpAccessibilityTree "platform" for the cross-platform
representation.

My intent is to add the tests along with the code that makes use of each
attribute - just to keep the changes small. I have another change pending for
implementing boundsForRange, for example - and I'll send one that makes use of
the word boundaries soon. Is that reasonable?

[David Tseng, 2013-10-21 18:10:25]

On Mon, Oct 21, 2013 at 10:26 AM, <dmazzoni@chromium.org> wrote:

>
> https://codereview.chromium.**org/25943003/diff/1/content/**
>
browser/accessibility/**accessibility_tree_formatter.**cc<https://codereview.chromium.org/25943003/diff/1/content/browser/accessibility/accessibility_tree_formatter.cc>
> File content/browser/accessibility/**accessibility_tree_formatter.**cc
> (right):
>
> https://codereview.chromium.**org/25943003/diff/1/content/**
>
browser/accessibility/**accessibility_tree_formatter.**cc#newcode69<https://codereview.chromium.org/25943003/diff/1/content/browser/accessibility/accessibility_tree_formatter.cc#newcode69>
> content/browser/accessibility/**accessibility_tree_formatter.**cc:69: if
> (node.IsLeaf())
> On 2013/10/17 22:56:29, David Tseng wrote:
>
>> Btw, isLeaf worries me because it's often possible to get focus set on
>>
> a
>
>> descendant of a leaf (depending on how we define leaves). We should be
>>
> careful
>
>> when stating what is a leaf and what isn't.
>>
>
> My intent is that this is only used to really mean a leaf - we literally
> don't expose any children of this node to the platform. On Android, we
> sometimes prune children of a focusable node that has a title, but we're
> careful to only do this when it doesn't itself have any focusable
> children - so there isn't any worry about this.
>
> Would some documentation help, or can you think of a better name?
>
>
Docs would help. Where does pruning occur for Windows? For example,
BrowserAccessibilityWin::get_childAt access BrowserAccessibility::children_
directly, so would probably see these children of static text nodes.



> https://codereview.chromium.**org/25943003/diff/1/content/**
>
common/accessibility_node_**data.h<https://codereview.chromium.org/25943003/diff/1/content/common/accessibility_node_data.h>
> File content/common/accessibility_**node_data.h (right):
>
> https://codereview.chromium.**org/25943003/diff/1/content/**
>
common/accessibility_node_**data.h#newcode156<https://codereview.chromium.org/25943003/diff/1/content/common/accessibility_node_data.h#newcode156>
> content/common/accessibility_**node_data.h:156: // For inline text.
> On 2013/10/17 22:56:29, David Tseng wrote:
>
>> These attributes are not very obvious; perhaps add a little more
>>
> detail about
>
>> what they are.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/25943003/diff/1/content/**
>
renderer/accessibility/**accessibility_node_serializer.**cc<https://codereview.chromium.org/25943003/diff/1/content/renderer/accessibility/accessibility_node_serializer.cc>
> File content/renderer/**accessibility/accessibility_**node_serializer.cc
> (right):
>
> https://codereview.chromium.**org/25943003/diff/1/content/**
>
renderer/accessibility/**accessibility_node_serializer.**cc#newcode177<https://codereview.chromium.org/25943003/diff/1/content/renderer/accessibility/accessibility_node_serializer.cc#newcode177>
> content/renderer/**accessibility/accessibility_**node_serializer.cc:177:
> dst->AddIntListAttribute(dst->**ATTR_WORD_ENDS, word_ends);
> On 2013/10/17 22:56:29, David Tseng wrote:
>
>> It seems like a good idea to perhaps have a few dump accessibility
>>
> tree tests to
>
>> validate these attributes on the browser end.
>>
>
> Agreed about testing these.
>
> I don't want to use dump accessibility tree tests because the only
> native APIs that make use of these attributes are "parameterized" apis,
> like boundsForRange, lineForIndex, etc. - which aren't very easy to
> express in DumpAccessibilityTree tests.
>
> Perhaps we need to work on a new type of test that's almost as easy to
> write as a DumpAccessibilityTree test but lets us query the resulting
> tree with different arguments.
>
> Or maybe we need a DumpAccessibilityTree "platform" for the
> cross-platform representation.
>
> My intent is to add the tests along with the code that makes use of each
> attribute - just to keep the changes small. I have another change
> pending for implementing boundsForRange, for example - and I'll send one
> that makes use of the word boundaries soon. Is that reasonable?
>
>
https://codereview.chromium.**org/25943003/<https://codereview.chromium.org/2...
>

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

[dmazzoni, 2013-10-21 21:00:18]

On 2013/10/21 18:10:25, David Tseng wrote:
> Docs would help. Where does pruning occur for Windows? For example,
> BrowserAccessibilityWin::get_childAt access BrowserAccessibility::children_
> directly, so would probably see these children of static text nodes.

Oh, good catch. I meant to actually check IsLeaf as part of this change.
I added support for this on each platform and added more docs.

[dmazzoni, 2013-10-22 22:30:20]

PTAL?

[aboxhall, 2013-10-23 16:15:20]

lgtm

https://codereview.chromium.org/25943003/diff/95001/content/browser/accessibi...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/25943003/diff/95001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_cocoa.mm:358: childCount =
0;
Extra-trivial nit: how about
uint32 childCount = 0;
if (!browserAccessibility_->IsLeaf())
   childCount = browserAccessibility_->child_count()

I realise !IsLeaf() will be the overwhelming majority case, it just seems funny
to be calling child_count() if we're just going to throw it away sometimes.

[David Tseng, 2013-10-23 16:42:46]

Not yet LG from me.

https://codereview.chromium.org/25943003/diff/95001/content/browser/accessibi...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/25943003/diff/95001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_win.cc:2625: if (IsLeaf() ||
child_index < children_.size()) {
I'm confused by this indexing logic;  child_index < child_.size()? So, to index
into children_, child_index needs to be >= children_.size()?

[dmazzoni, 2013-10-30 16:45:25]

Thanks for your patience...ready for another look.

I renamed three BrowserAccessibility functions to:

PlatformChildCount
PlatformGetChild
PlatformIsLeaf

All three of those return the tree that's specific to that platform, meaning
that some nodes may be dropped. I tried to
replace every use of low-level APIs like child_count() and children().size()
with these, except for cases where we really did want the low-level platform
tree.

https://codereview.chromium.org/25943003/diff/95001/content/browser/accessibi...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/25943003/diff/95001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_win.cc:2625: if (IsLeaf() ||
child_index < children_.size()) {
On 2013/10/23 16:42:46, David Tseng wrote:
> I'm confused by this indexing logic;  child_index < child_.size()? So, to
index
> into children_, child_index needs to be >= children_.size()?

Ack! Yes, you found a longstanding bug. We never caught it because it only
affected ISimpleDOMNode. I think ISimpleDOMNode is used occasionally, but no
screen reader appears to ever use it to walk the DOM tree.

I replaced this with PlatformGetChild followed by a NULL check.

[David Tseng, 2013-10-31 19:44:47]

LGTM

https://codereview.chromium.org/25943003/diff/385001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/25943003/diff/385001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.h:105: // invalid index.
Returns NULL is PlatformIsLeaf() returns true.
s/is/if

[commit-bot: I haz the power, 2013-11-01 05:22:44]

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

[commit-bot: I haz the power, 2013-11-01 07:14:59]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-01 15:06:33]

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

[commit-bot: I haz the power, 2013-11-01 17:59:45]

Change committed as 232429