Issue 13479003: Modify AccessibilityTreeFormatter to build up an internal representation of the tree

Issue 13479003: Modify AccessibilityTreeFormatter to build up an internal representation of the tree (Closed)

Created:
7 years, 8 months ago by aboxhall

Modified:
7 years, 8 months ago

Reviewers:
dmazzoni

CC:
chromium-reviews, hashimoto+watch_chromium.org, aboxhall+watch_chromium.org, yoshiki+watch_chromium.org, jam, yuzo+watch_chromium.org, davidbarr+watch_chromium.org, joi+watch-content_chromium.org, sail+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, ctguil+watch_chromium.org, zork+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Modify AccessibilityTreeFormatter to build up an internal representation of the tree in the form of DictionaryValue et al. This may be used at a later stage to create a JSON representation of the tree. Also modify the Mac platform implementation to use the NSAccessibility attribute names rather than method names. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194775 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195443

Patch Set 1 #

Patch Set 2 : Rebaseline tests #

Patch Set 3 : Code cleanup #

Total comments: 18

Patch Set 4 : Address review comments #

Patch Set 5 : First pass at Windows code #

Patch Set 6 : Rebase #

Patch Set 7 : Reinstate AddProperties method name #

Patch Set 8 : Trying to fix compile errors #

Patch Set 9 : s/SetHeight/SetInteger/ #

Patch Set 10 : Implementing ToString on Windows #

Patch Set 11 : Implementing ToString on Windows #

Patch Set 12 : Attempt to fix windows compile and implement ToString as a loop #

Patch Set 13 : More compile fixing #

Patch Set 14 : More compile errors #

Patch Set 15 : Partial fix for compile #

Patch Set 16 : Fix for illegal this pointer #

Patch Set 17 : string_value -> &string_value #

Patch Set 18 : Use portable wprintf formats #

Patch Set 19 : Those pesky string types #

Total comments: 4

Patch Set 20 : Fix coordinate attribute formatting and add a NOTREACHED for the default case; temporarily enable A… #

Patch Set 21 : Re-disable AccessibilityIframeCoordinates #

Patch Set 22 : Implement ToString and AddProperties for non-Windows/Mac platforms #

Patch Set 23 : Rebase #

Patch Set 24 : Fix syntax errors #

Total comments: 2

Patch Set 25 : Lazy initialize ALL_ATTRIBUTES #

Total comments: 2

Patch Set 26 : Use CR_DEFINE_STATIC_LOCAL per Dominic's suggestion #

Patch Set 27 : Rebase #

Created: 7 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+669 lines, -435 lines)			Patch
M	content/browser/accessibility/accessibility_tree_formatter.h	View	1 2 3 4 5 6 7 8 9 10 11 12	3 chunks	+51 lines, -8 lines	0 comments	Download
M	content/browser/accessibility/accessibility_tree_formatter.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23	4 chunks	+63 lines, -27 lines	0 comments	Download
M	content/browser/accessibility/accessibility_tree_formatter_mac.mm	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25	3 chunks	+113 lines, -62 lines	0 comments	Download
M	content/browser/accessibility/accessibility_tree_formatter_win.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22	2 chunks	+197 lines, -74 lines	0 comments	Download
M	content/test/data/accessibility/a-expected-mac.txt	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/a-with-img.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/a-with-img-expected-mac.txt	View	1	1 chunk	+14 lines, -14 lines	0 comments	Download
M	content/test/data/accessibility/address.html	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/address-expected-mac.txt	View	1 2	1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/aria-application.html	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/aria-application-expected-mac.txt	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/aria-combobox-expected-mac.txt	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/aria-invalid.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/aria-invalid-expected-mac.txt	View	1	1 chunk	+12 lines, -12 lines	0 comments	Download
M	content/test/data/accessibility/aria-level.html	View		1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/aria-level-expected-mac.txt	View	1	1 chunk	+14 lines, -14 lines	0 comments	Download
M	content/test/data/accessibility/aria-menu.html	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/aria-menu-expected-mac.txt	View	1	1 chunk	+8 lines, -8 lines	0 comments	Download
M	content/test/data/accessibility/aria-menuitemradio.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/aria-menuitemradio-expected-mac.txt	View	1 2	1 chunk	+6 lines, -6 lines	0 comments	Download
M	content/test/data/accessibility/aria-pressed-expected-mac.txt	View	1	1 chunk	+4 lines, -4 lines	0 comments	Download
M	content/test/data/accessibility/aria-progressbar.html	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	content/test/data/accessibility/aria-progressbar-expected-mac.txt	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/aria-valuemax.html	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/aria-valuemax-expected-mac.txt	View	1	1 chunk	+4 lines, -4 lines	0 comments	Download
M	content/test/data/accessibility/aria-valuemin.html	View		1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/aria-valuemin-expected-mac.txt	View	1	1 chunk	+4 lines, -4 lines	0 comments	Download
M	content/test/data/accessibility/article.html	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/article-expected-mac.txt	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/bdo.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/bdo-expected-mac.txt	View	1 2	1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/br-expected-mac.txt	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/button-name-calc.html	View		1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/button-name-calc-expected-mac.txt	View	1	1 chunk	+6 lines, -6 lines	0 comments	Download
M	content/test/data/accessibility/canvas-expected-mac.txt	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/checkbox-name-calc.html	View		1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/checkbox-name-calc-expected-mac.txt	View	1	1 chunk	+6 lines, -6 lines	0 comments	Download
M	content/test/data/accessibility/div-expected-mac.txt	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/dl.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/dl-expected-mac.txt	View	1	1 chunk	+7 lines, -7 lines	0 comments	Download
M	content/test/data/accessibility/em-expected-mac.txt	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/footer.html	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/footer-expected-mac.txt	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/form-expected-mac.txt	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/heading-expected-mac.txt	View	1	1 chunk	+24 lines, -24 lines	0 comments	Download
M	content/test/data/accessibility/iframe-coordinates.html	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
D	content/test/data/accessibility/iframe-coordinates-expected-mac.txt	View	1	1 chunk	+0 lines, -19 lines	0 comments	Download
M	content/test/data/accessibility/input-button.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/input-button-expected-mac.txt	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/input-color.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/input-color-expected-mac.txt	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/input-range-expected-mac.txt	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/label-expected-mac.txt	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/list-markers.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/list-markers-expected-mac.txt	View	1	1 chunk	+13 lines, -13 lines	0 comments	Download
M	content/test/data/accessibility/p-expected-mac.txt	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/select.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/select-expected-mac.txt	View	1	1 chunk	+17 lines, -17 lines	0 comments	Download
M	content/test/data/accessibility/span.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/span-expected-mac.txt	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/spinbutton.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/spinbutton-expected-mac.txt	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/svg.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/svg-expected-mac.txt	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/tab-expected-mac.txt	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/togglebutton-expected-mac.txt	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	content/test/data/accessibility/toolbar.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/toolbar-expected-mac.txt	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	content/test/data/accessibility/ul.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/test/data/accessibility/ul-expected-mac.txt	View	1	1 chunk	+7 lines, -7 lines	0 comments	Download
M	content/test/data/accessibility/wbr-expected-mac.txt	View	1 2	1 chunk	+3 lines, -3 lines	0 comments	Download

Messages

Total messages: 25 (0 generated)

Expand Messages | Collapse Messages

aboxhall

Dominic: I haven't done the necessary Windows changes yet, but what do you think of ...

7 years, 8 months ago (2013-04-05 04:38:03 UTC) #1

dmazzoni

lgtm https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibility/accessibility_tree_formatter.cc File content/browser/accessibility/accessibility_tree_formatter.cc (right): https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibility/accessibility_tree_formatter.cc#newcode65 content/browser/accessibility/accessibility_tree_formatter.cc:65: dict->Set("children", children); Put dict field names in a ...

7 years, 8 months ago (2013-04-08 05:46:35 UTC) #2

aboxhall

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibility/accessibility_tree_formatter.cc File content/browser/accessibility/accessibility_tree_formatter.cc (right): https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibility/accessibility_tree_formatter.cc#newcode65 content/browser/accessibility/accessibility_tree_formatter.cc:65: dict->Set("children", children); On 2013/04/08 05:46:35, Dominic Mazzoni wrote: > ...

7 years, 8 months ago (2013-04-10 03:09:34 UTC) #3

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibil...
File content/browser/accessibility/accessibility_tree_formatter.cc (right):

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibil...
content/browser/accessibility/accessibility_tree_formatter.cc:65:
dict->Set("children", children);
On 2013/04/08 05:46:35, Dominic Mazzoni wrote:
> Put dict field names in a constant at the top of this file, inside the
anonymous
> namespace, something like:
> 
> namespace {
> const int kIndentSpaces = 4;
> ...
> const char[] kChildrenDictAttr = "children";
> }

Done (there was actually only this one).

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibil...
content/browser/accessibility/accessibility_tree_formatter.cc:76: string16 line
= ToString(dict, string16(depth * kIndentSpaces, ' '));
On 2013/04/08 05:46:35, Dominic Mazzoni wrote:
> Thanks - I didn't realize you could construct a string this way!

Hehe - I was thinking "surely there's a more efficient way" so I looked up the
STL docs. It's neat!

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibil...
content/browser/accessibility/accessibility_tree_formatter.cc:82:
dict->GetList("children", &children);
On 2013/04/08 05:46:35, Dominic Mazzoni wrote:
> Make sure this uses the constant!

Done.

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibil...
content/browser/accessibility/accessibility_tree_formatter.cc:82:
dict->GetList("children", &children);
On 2013/04/08 05:46:35, Dominic Mazzoni wrote:
> Make sure this uses the constant!

Done.

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibil...
File content/browser/accessibility/accessibility_tree_formatter.h (right):

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibil...
content/browser/accessibility/accessibility_tree_formatter.h:32: // Populates
the given DictionaryValue with the accessibility tree.
On 2013/04/08 05:46:35, Dominic Mazzoni wrote:
> Can you document the JSON schema here, or point to where it's documented or
> defined?
> 
> A minimally complete and self-explanatory example JSON output would be more
than
> enough, I don't need paragraphs.

Done.

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibil...
content/browser/accessibility/accessibility_tree_formatter.h:40: void
AddProperties(BrowserAccessibility* node, DictionaryValue* dict);
On 2013/04/08 05:46:35, Dominic Mazzoni wrote:
> Try making the first argument const to better distinguish it from the second
> output argument.

Unfortunately ToBrowserAccessibilityCocoa() (and presumably
ToBrowserAccessibilityWin()) is non-const. I guess I could sneakily const_cast
it for that one method call, though.

> I think this can be under "protected" - it doesn't need to be public, right?

Done.

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibil...
File content/browser/accessibility/accessibility_tree_formatter_mac.mm (right):

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibil...
content/browser/accessibility/accessibility_tree_formatter_mac.mm:53: string16
FormatString(std::string prefix,
On 2013/04/08 05:46:35, Dominic Mazzoni wrote:
> Use a const ref for string arguments, like: const std::string&
> 
> ...please make sure you do that throughout.

I ended up deleting this method, it was silly anyway.

I went through and const-reffed anything I could in other methods.

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibil...
content/browser/accessibility/accessibility_tree_formatter_mac.mm:67: [NSString
stringWithFormat:@"%s=(%d, %d)", name.c_str(), x, y];
On 2013/04/08 05:46:35, Dominic Mazzoni wrote:
> Indent 4 spaces

Done.

https://codereview.chromium.org/13479003/diff/5001/content/browser/accessibil...
content/browser/accessibility/accessibility_tree_formatter_mac.mm:151: if
(dict->GetString(SysNSStringToUTF8(NSAccessibilitySubroleAttribute),
On 2013/04/08 05:46:35, Dominic Mazzoni wrote:
> Put SysNSStringToUTF8(NSAccessibilitySubroleAttribute) in a local variable to
> make this more readable (especially since it's used twice).

Done.

> Why is "subrole" a special case? Can't it be handled as part of ALL_ATTRIBUTES
> below?

Only because as originally written, it was formatted without enclosing single
quotes around the attribute value. Since I'm rebasing all the tests anyway, I
can make it behave the same in this change if that's better.

dmazzoni

LGTM On 2013/04/10 03:09:34, aboxhall wrote: > > Why is "subrole" a special case? Can't ...

7 years, 8 months ago (2013-04-10 03:11:12 UTC) #4

aboxhall

On 2013/04/10 03:11:12, Dominic Mazzoni wrote: > LGTM > > On 2013/04/10 03:09:34, aboxhall wrote: ...

7 years, 8 months ago (2013-04-10 03:30:25 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/13479003/38001

7 years, 8 months ago (2013-04-12 00:04:53 UTC) #6

aboxhall

Ok, looks like the Windows changes are finally working. Still LG?

7 years, 8 months ago (2013-04-12 08:34:46 UTC) #7

aboxhall

On 2013/04/12 08:34:46, aboxhall wrote: > Ok, looks like the Windows changes are finally working. ...

7 years, 8 months ago (2013-04-13 00:53:35 UTC) #8

dmazzoni

LGTM for Windows changes, too https://codereview.chromium.org/13479003/diff/64001/content/browser/accessibility/accessibility_tree_formatter_win.cc File content/browser/accessibility/accessibility_tree_formatter_win.cc (right): https://codereview.chromium.org/13479003/diff/64001/content/browser/accessibility/accessibility_tree_formatter_win.cc#newcode260 content/browser/accessibility/accessibility_tree_formatter_win.cc:260: // Currently all dictionary ...

7 years, 8 months ago (2013-04-15 04:47:48 UTC) #9

aboxhall

https://codereview.chromium.org/13479003/diff/64001/content/browser/accessibility/accessibility_tree_formatter_win.cc File content/browser/accessibility/accessibility_tree_formatter_win.cc (right): https://codereview.chromium.org/13479003/diff/64001/content/browser/accessibility/accessibility_tree_formatter_win.cc#newcode260 content/browser/accessibility/accessibility_tree_formatter_win.cc:260: // Currently all dictionary values are coordinates. On 2013/04/15 ...

7 years, 8 months ago (2013-04-15 07:00:09 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/13479003/76001

7 years, 8 months ago (2013-04-15 07:00:10 UTC) #11

commit-bot: I haz the power

Sorry for I got bad news for ya. Compile failed with a clobber build on ...

7 years, 8 months ago (2013-04-15 07:17:48 UTC) #12

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/13479003/92001

7 years, 8 months ago (2013-04-18 01:09:33 UTC) #13

dmazzoni

https://chromiumcodereview.appspot.com/13479003/diff/92001/content/browser/accessibility/accessibility_tree_formatter_mac.mm File content/browser/accessibility/accessibility_tree_formatter_mac.mm (right): https://chromiumcodereview.appspot.com/13479003/diff/92001/content/browser/accessibility/accessibility_tree_formatter_mac.mm#newcode26 content/browser/accessibility/accessibility_tree_formatter_mac.mm:26: NSArray* ALL_ATTRIBUTES = [NSArray arrayWithObjects: This is the issue: ...

7 years, 8 months ago (2013-04-18 17:08:34 UTC) #15

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/13479003/107001

7 years, 8 months ago (2013-04-18 22:26:16 UTC) #16

aboxhall

dmazzoni: PTAL? Only accessibility_tree_formatter_mac.mm has changed.

7 years, 8 months ago (2013-04-18 22:26:49 UTC) #17

dmazzoni

lgtm with nit https://codereview.chromium.org/13479003/diff/107001/content/browser/accessibility/accessibility_tree_formatter_mac.mm File content/browser/accessibility/accessibility_tree_formatter_mac.mm (right): https://codereview.chromium.org/13479003/diff/107001/content/browser/accessibility/accessibility_tree_formatter_mac.mm#newcode26 content/browser/accessibility/accessibility_tree_formatter_mac.mm:26: NSArray* ALL_ATTRIBUTES = nil; I think ...

7 years, 8 months ago (2013-04-18 23:05:14 UTC) #18

aboxhall

https://codereview.chromium.org/13479003/diff/107001/content/browser/accessibility/accessibility_tree_formatter_mac.mm File content/browser/accessibility/accessibility_tree_formatter_mac.mm (right): https://codereview.chromium.org/13479003/diff/107001/content/browser/accessibility/accessibility_tree_formatter_mac.mm#newcode26 content/browser/accessibility/accessibility_tree_formatter_mac.mm:26: NSArray* ALL_ATTRIBUTES = nil; On 2013/04/18 23:05:14, Dominic Mazzoni ...

7 years, 8 months ago (2013-04-19 18:09:50 UTC) #19

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/13479003/118001

7 years, 8 months ago (2013-04-19 18:10:13 UTC) #20

commit-bot: I haz the power

Step "update" is always a major failure. Look at the try server FAQ for more ...

7 years, 8 months ago (2013-04-19 18:12:09 UTC) #21

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/13479003/20072

7 years, 8 months ago (2013-04-19 23:09:25 UTC) #22

commit-bot: I haz the power

Step "update" is always a major failure. Look at the try server FAQ for more ...

7 years, 8 months ago (2013-04-20 00:36:06 UTC) #23

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/13479003/20072

7 years, 8 months ago (2013-04-22 02:44:49 UTC) #24

Message was sent while issue was closed.

Change committed as 195443

Expand Messages | Collapse Messages