Support IAccessibleHypertext.

Support IAccessibleHypertext.

- This modifies the way we build the browser accessibility tree in CreateAccessibilityTree. We still build the parent child links while recursing depth first through the WebAccessibility structure, but this patch changes the initialization of a node to occur *after* the children have been fully populated into the subtree of any given node.
- implements IAccessibleHypertext on a BrowserAccessibilityWin. Adds:
-- a string for the hypertext which contains the text of the static text children concatinated together along with the embedded characters for the non-text children.
-- map from the character offset within the hypertext to a hyperlink index.
-- a collection of children that are hyperlinks (basically nodes that are not static texts).
- adds no-op implementations of IAccessibleHyperlink and IAccessibleAction as required by the usage and interface inheritance of IAccessibleHyperlink.

BUG=99629
TEST=manually tested with NVDA.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108211

[David Tseng, 2011-10-28 17:10:26]

Ready for review.

Unit tests to come. Please take a look.

[dmazzoni, 2011-10-28 17:33:53]

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:1607: *n_characters =
hypertext_.length();
Can this method use TextForIAccessibleText instead?

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:1859:
*hyperlink_count = hyperlink_offset_to_index_.size();
Need to check all pointer arguments and return E_INVALIDARG if they're null.

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:1869: if (index < 0
|| index >= static_cast<long>(children().size()) || !hyperlink)
You only have an embedded object character for non-static-text children, right?
So does it make sense for this to support getting hyperlinks to static text
children?

Instead of accessing children, could you access hyperlink_offset_to_index_ here?

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:2473: hypertext_ +=
L"\xfffc";
Define a constant for this

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.h (right):

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.h:500: CONTENT_EXPORT
STDMETHODIMP get_anchorTarget(long index,
Nit: fit all arguments on one line or all on separate lines.

We're not consistently following this style rule in this file now, we should
clean it up...

[David Tseng, 2011-10-28 18:25:14]

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:1607: *n_characters =
hypertext_.length();
On 2011/10/28 17:33:53, Dominic Mazzoni wrote:
> Can this method use TextForIAccessibleText instead?

Done.

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:1859:
*hyperlink_count = hyperlink_offset_to_index_.size();
On 2011/10/28 17:33:53, Dominic Mazzoni wrote:
> Need to check all pointer arguments and return E_INVALIDARG if they're null.

Done.

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:1869: if (index < 0
|| index >= static_cast<long>(children().size()) || !hyperlink)
On 2011/10/28 17:33:53, Dominic Mazzoni wrote:
> You only have an embedded object character for non-static-text children,
right?
> So does it make sense for this to support getting hyperlinks to static text
> children?
> 
> Instead of accessing children, could you access hyperlink_offset_to_index_
here?
> 

The documentation states that clients are supposed to first call
get_hyperlinkIndex to get the hyperlink index corresponding to a provided
offset. That index is then passed into this method.

I guess I could just return the offset as the index in get_hyperlinkIndex and
check for it's existence in the map here.

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:2473: hypertext_ +=
L"\xfffc";
On 2011/10/28 17:33:53, Dominic Mazzoni wrote:
> Define a constant for this

Done.

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.h (right):

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.h:500: CONTENT_EXPORT
STDMETHODIMP get_anchorTarget(long index,
On 2011/10/28 17:33:53, Dominic Mazzoni wrote:
> Nit: fit all arguments on one line or all on separate lines.
> 
> We're not consistently following this style rule in this file now, we should
> clean it up...

Done.

[David Tseng, 2011-10-31 23:11:13]

PTAL; I've included some unit tests to exercise these new codepaths.

On 10/28/11, dtseng@chromium.org <dtseng@chromium.org> wrote:
>
>
http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
> File content/browser/accessibility/browser_accessibility_win.cc (right):
>
>
http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility_win.cc:1607:
> *n_characters = hypertext_.length();
> On 2011/10/28 17:33:53, Dominic Mazzoni wrote:
>> Can this method use TextForIAccessibleText instead?
>
> Done.
>
>
http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility_win.cc:1859:
> *hyperlink_count = hyperlink_offset_to_index_.size();
> On 2011/10/28 17:33:53, Dominic Mazzoni wrote:
>> Need to check all pointer arguments and return E_INVALIDARG if they're
> null.
>
> Done.
>
>
http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility_win.cc:1869: if
> (index < 0 || index >= static_cast<long>(children().size()) ||
> !hyperlink)
> On 2011/10/28 17:33:53, Dominic Mazzoni wrote:
>> You only have an embedded object character for non-static-text
> children, right?
>> So does it make sense for this to support getting hyperlinks to static
> text
>> children?
>
>> Instead of accessing children, could you access
> hyperlink_offset_to_index_ here?
>
>
> The documentation states that clients are supposed to first call
> get_hyperlinkIndex to get the hyperlink index corresponding to a
> provided offset. That index is then passed into this method.
>
> I guess I could just return the offset as the index in
> get_hyperlinkIndex and check for it's existence in the map here.
>
>
http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility_win.cc:2473:
> hypertext_ += L"\xfffc";
> On 2011/10/28 17:33:53, Dominic Mazzoni wrote:
>> Define a constant for this
>
> Done.
>
>
http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
> File content/browser/accessibility/browser_accessibility_win.h (right):
>
>
http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility_win.h:500:
> CONTENT_EXPORT STDMETHODIMP get_anchorTarget(long index,
> On 2011/10/28 17:33:53, Dominic Mazzoni wrote:
>> Nit: fit all arguments on one line or all on separate lines.
>
>> We're not consistently following this style rule in this file now, we
> should
>> clean it up...
>
> Done.
>
> http://codereview.chromium.org/8416034/
>

[dmazzoni, 2011-11-01 05:08:34]

Great tests. Looks ready go after fixing get_hyperlink so it's a 0-based index.

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/8416034/diff/5002/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.cc:1869: if (index < 0
|| index >= static_cast<long>(children().size()) || !hyperlink)
On 2011/10/28 18:25:15, David Tseng wrote:
> On 2011/10/28 17:33:53, Dominic Mazzoni wrote:
> > You only have an embedded object character for non-static-text children,
> right?
> > So does it make sense for this to support getting hyperlinks to static text
> > children?
> > 
> > Instead of accessing children, could you access hyperlink_offset_to_index_
> here?
> > 
> 
> The documentation states that clients are supposed to first call
> get_hyperlinkIndex to get the hyperlink index corresponding to a provided
> offset. That index is then passed into this method.
> 
> I guess I could just return the offset as the index in get_hyperlinkIndex and
> check for it's existence in the map here.

Clients can also call nHyperlinks and iterate over them in order from 0 to
nHyperlinks - 1, right? My concern is that your implementation breaks this -
nHyperlinks returns the number of object children (non-text), but get_hyperlink
returns a hyperlink based on its child index including all children.

http://codereview.chromium.org/8416034/diff/8001/chrome/browser/accessibility...
File chrome/browser/accessibility/browser_accessibility_win_unittest.cc (right):

http://codereview.chromium.org/8416034/diff/8001/chrome/browser/accessibility...
chrome/browser/accessibility/browser_accessibility_win_unittest.cc:503:
EXPECT_EQ(E_FAIL, root_obj->get_hyperlink(0, hyperlink.Receive()));
I think this should return success - the docs say that get_hyperlink is a
0-based index relative to the number of hyperlinks. To put it another way, it's
an index, not an arbitrary ID.

http://codereview.chromium.org/8416034/diff/8001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.h (right):

http://codereview.chromium.org/8416034/diff/8001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.h:43: public
IAccessibleHyperlink,
Add IAccessibleAction here?

http://codereview.chromium.org/8416034/diff/8001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.h:73: // Represents a
non-static text node in IAccessibleHypertext.
Add a tiny bit more detail so someone new to this understands what's going on -
say something like: this character is embedded in the response to
IAccessibleText's get_text interface, indicating the position where a child
object appears.

http://codereview.chromium.org/8416034/diff/8001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.h:490: CONTENT_EXPORT
STDMETHODIMP get_nHyperlinks(long* hyperlinkCount);
Nit: Convert all of the method parameters to hacker style, i.e.
lowercase_with_underscores

[David Tseng, 2011-11-01 19:38:03]

http://codereview.chromium.org/8416034/diff/8001/chrome/browser/accessibility...
File chrome/browser/accessibility/browser_accessibility_win_unittest.cc (right):

http://codereview.chromium.org/8416034/diff/8001/chrome/browser/accessibility...
chrome/browser/accessibility/browser_accessibility_win_unittest.cc:503:
EXPECT_EQ(E_FAIL, root_obj->get_hyperlink(0, hyperlink.Receive()));
On 2011/11/01 05:08:34, Dominic Mazzoni wrote:
> I think this should return success - the docs say that get_hyperlink is a
> 0-based index relative to the number of hyperlinks. To put it another way,
it's
> an index, not an arbitrary ID.

Done.

http://codereview.chromium.org/8416034/diff/8001/content/browser/accessibilit...
File content/browser/accessibility/browser_accessibility_win.h (right):

http://codereview.chromium.org/8416034/diff/8001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.h:43: public
IAccessibleHyperlink,
On 2011/11/01 05:08:34, Dominic Mazzoni wrote:
> Add IAccessibleAction here?

IAccessibleHyperlink : IAccessibleAction

http://codereview.chromium.org/8416034/diff/8001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.h:73: // Represents a
non-static text node in IAccessibleHypertext.
On 2011/11/01 05:08:34, Dominic Mazzoni wrote:
> Add a tiny bit more detail so someone new to this understands what's going on
-
> say something like: this character is embedded in the response to
> IAccessibleText's get_text interface, indicating the position where a child
> object appears.

Done.

http://codereview.chromium.org/8416034/diff/8001/content/browser/accessibilit...
content/browser/accessibility/browser_accessibility_win.h:490: CONTENT_EXPORT
STDMETHODIMP get_nHyperlinks(long* hyperlinkCount);
On 2011/11/01 05:08:34, Dominic Mazzoni wrote:
> Nit: Convert all of the method parameters to hacker style, i.e.
> lowercase_with_underscores

Done.

[dmazzoni, 2011-11-01 21:32:37]

lgtm

http://codereview.chromium.org/8416034/diff/13001/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_win.h (right):

http://codereview.chromium.org/8416034/diff/13001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.h:792: // Collection of
non-static text child indecies used for gathering hyperlinks.
indecies -> indices

To be more clear, I'd say "the child index corresponding to each hyperlink".

[David Tseng, 2011-11-01 21:38:33]

>
http://codereview.chromium.org/8416034/diff/13001/content/browser/accessibili...
> File content/browser/accessibility/browser_accessibility_win.h (right):
>
>
http://codereview.chromium.org/8416034/diff/13001/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_win.h:792: //
> Collection of non-static text child indecies used for gathering
> hyperlinks.
> indecies -> indices
>
> To be more clear, I'd say "the child index corresponding to each
> hyperlink".
>
Done.
> http://codereview.chromium.org/8416034/
>

[commit-bot: I haz the power, 2011-11-01 22:55:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/8416034/18001

[commit-bot: I haz the power, 2011-11-02 00:41:06]

Change committed as 108211