Add a big grab bag of missing web accessibility functionality...

Add a big grab bag of missing web accessibility
functionality on Windows. (Much of this will benefit
Mac in a future changelist.) Improvements include
dozens of corrected roles and states for various
elements, improved support for tables with rowspan
and colspan, range control support, and live region
support.

Also adds a new command-line flag to turn on
logging of accessibility events, to help making
this type of bug fixing much easier in the future.

BUG=89181, 89185, 89187, 89188, 89202, 89205, 89210, 89212, 89213, 89223
TEST=Manual testing with JAWS, NVDA, AViewer, and accProbe.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99161

[dmazzoni, 2011-08-25 21:45:19]

dtseng: primary reviewer
ctguil: additional a11y review if you have time
jam: OWNERS for content/common and content/renderer
tony: OWNERS for webkit/glue

There are several different things in one changelist because I was trying to
batch-fix related issues and the fixes ended up overlapping a lot.  If there are
any changes that look at all risky or confusing, I'm happy to move them to a
separate changelist.

[David Tseng, 2011-08-26 16:14:37]

First pass.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility.cc (right):

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility.cc:185: this);
I think you want to align this with
"ViewHostMsg_AccessibilityNotification_Type::
"

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility.cc:260: bool
BrowserAccessibility::IsEditableText() {
const method?

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility.h (right):

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility.h:149: float_attributes()
const {
Indent? (and below).

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_manager.cc:110: if (iter ==
renderer_id_to_child_id_map_.end()) {
nit: Do we need the braces?

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_manager.cc:136:
NOTIFICATION_TYPE_LIVE_REGION_CHANGED:
nit: sort alphabetically.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_manager.cc:152: // For all
other notifications, children are not included.
Either remove the DCHECK above or explicitly list the other notifications that
exclude descendants (without which I think clang will generate a warning).

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_manager.cc:295: for (int i =
0; i < static_cast<int>(old_tree_nodes.size()); i++) {
nit: braces necessary?

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_manager.cc:359: if
(send_show_events) {
Any chance we could move this up to UpdateNode?

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_manager.cc:363: instance);
nit: align with "ViewHostMsg_AccessibilityNotification_Type::
"

http://codereview.chromium.org/7745035/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7745035/diff/1/content/common/view_messages.h#...
content/common/view_messages.h:108: NOTIFICATION_TYPE_ALERT,
nit: sort alphabetically.

[jam, 2011-08-26 16:54:31]

content\common and content\renderer lgtm

[tony, 2011-08-27 00:05:46]

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc
File webkit/glue/webaccessibility.cc (right):

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:38: return b? "true": "false";
Nit: Space between b and ?

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:497: if (iattrs.find(ATTR_DOC_SCROLLX) !=
iattrs.end())
It would be faster to iterate over the map and switch over the key value.  Then
you don't have to run find on the map for each possible value.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:519: if (sattrs.find(ATTR_DOC_URL) !=
sattrs.end())
Same as above.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:594: if (children.size() > 0)
Nit: !children.empty()

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:597: if (indirect_child_ids.size() > 0) {
Nit: !empty()

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:599: for (size_t i = 0; i <
indirect_child_ids.size(); i++) {
Nit: ++i

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:606: if (line_breaks.size() > 0) {
Please make a helper function for printing a vector of ints.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:615: if (cell_ids.size() > 0) {
Nit: !empty()

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:617: for (size_t i = 0; i < cell_ids.size();
i++) {
Nit: ++i

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:626: indent++;
Nit: ++indent

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:629: indent--;
Nit: --indent

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:643: if (src.valueDescription().length())
Nit: !.empty()

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.h
File webkit/glue/webaccessibility.h (right):

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.h#...
webkit/glue/webaccessibility.h:156: STATE_VISITED
Nit: Is it possible to have a compile assert that there are at most 32 States?

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.h#...
webkit/glue/webaccessibility.h:283: std::vector<int32> unique_cell_ids;
Can we just make this a set?

[dmazzoni, 2011-08-29 18:08:50]

Addressed all comments.

I also shortened the enum constants in content/common/view_messages.h so that
they fit on one line most of the time, it really improves readability. John, can
you have another quick look?  I'm happy to consider other names as long as the
full scoped name fits in a single line in a switch statement.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility.cc (right):

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility.cc:185: this);
On 2011/08/26 16:14:37, David Tseng wrote:
> I think you want to align this with
> "ViewHostMsg_AccessibilityNotification_Type::
> "

You're right, thanks!

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility.cc:260: bool
BrowserAccessibility::IsEditableText() {
On 2011/08/26 16:14:37, David Tseng wrote:
> const method?

Done. I made some other methods const as well.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility.h (right):

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility.h:149: float_attributes()
const {
On 2011/08/26 16:14:37, David Tseng wrote:
> Indent? (and below).

I think this may have been correct, but as a better fix, I defined some typedefs
for these maps so that these all fit on one line.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_manager.cc:110: if (iter ==
renderer_id_to_child_id_map_.end()) {
On 2011/08/26 16:14:37, David Tseng wrote:
> nit: Do we need the braces?

Done.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_manager.cc:136:
NOTIFICATION_TYPE_LIVE_REGION_CHANGED:
On 2011/08/26 16:14:37, David Tseng wrote:
> nit: sort alphabetically.

Done.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_manager.cc:152: // For all
other notifications, children are not included.
On 2011/08/26 16:14:37, David Tseng wrote:
> Either remove the DCHECK above or explicitly list the other notifications that
> exclude descendants (without which I think clang will generate a warning).

Done.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_manager.cc:295: for (int i =
0; i < static_cast<int>(old_tree_nodes.size()); i++) {
On 2011/08/26 16:14:37, David Tseng wrote:
> nit: braces necessary?

Done.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_manager.cc:359: if
(send_show_events) {
On 2011/08/26 16:14:37, David Tseng wrote:
> Any chance we could move this up to UpdateNode?

It'd be pretty hard, we need to update children_can_send_show_events as part of
the recursive call.

I added a comment to try to explain it better.

Essentially what we need to do is send a SHOW event for the root of a subtree
that just appeared for the first time, but not for any of its descendants.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_manager.cc:363: instance);
On 2011/08/26 16:14:37, David Tseng wrote:
> nit: align with "ViewHostMsg_AccessibilityNotification_Type::
> "

Done.

http://codereview.chromium.org/7745035/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7745035/diff/1/content/common/view_messages.h#...
content/common/view_messages.h:108: NOTIFICATION_TYPE_ALERT,
On 2011/08/26 16:14:37, David Tseng wrote:
> nit: sort alphabetically.

Done.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc
File webkit/glue/webaccessibility.cc (right):

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:38: return b? "true": "false";
On 2011/08/27 00:05:47, tony wrote:
> Nit: Space between b and ?

Done.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:497: if (iattrs.find(ATTR_DOC_SCROLLX) !=
iattrs.end())
On 2011/08/27 00:05:47, tony wrote:
> It would be faster to iterate over the map and switch over the key value. 
Then
> you don't have to run find on the map for each possible value.

Done.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:519: if (sattrs.find(ATTR_DOC_URL) !=
sattrs.end())
On 2011/08/27 00:05:47, tony wrote:
> Same as above.

Done.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:594: if (children.size() > 0)
On 2011/08/27 00:05:47, tony wrote:
> Nit: !children.empty()

Done.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:597: if (indirect_child_ids.size() > 0) {
On 2011/08/27 00:05:47, tony wrote:
> Nit: !empty()

Done.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:599: for (size_t i = 0; i <
indirect_child_ids.size(); i++) {
On 2011/08/27 00:05:47, tony wrote:
> Nit: ++i

Fixed throughout.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:606: if (line_breaks.size() > 0) {
On 2011/08/27 00:05:47, tony wrote:
> Please make a helper function for printing a vector of ints.

Done.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:615: if (cell_ids.size() > 0) {
On 2011/08/27 00:05:47, tony wrote:
> Nit: !empty()

Done.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:617: for (size_t i = 0; i < cell_ids.size();
i++) {
On 2011/08/27 00:05:47, tony wrote:
> Nit: ++i

Done.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:626: indent++;
On 2011/08/27 00:05:47, tony wrote:
> Nit: ++indent

Done.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:629: indent--;
On 2011/08/27 00:05:47, tony wrote:
> Nit: --indent

Done.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:643: if (src.valueDescription().length())
On 2011/08/27 00:05:47, tony wrote:
> Nit: !.empty()

Actually this one is a WebString, which doesn't have empty().

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.h
File webkit/glue/webaccessibility.h (right):

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.h#...
webkit/glue/webaccessibility.h:156: STATE_VISITED
On 2011/08/27 00:05:47, tony wrote:
> Nit: Is it possible to have a compile assert that there are at most 32 States?

Done.

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.h#...
webkit/glue/webaccessibility.h:283: std::vector<int32> unique_cell_ids;
On 2011/08/27 00:05:47, tony wrote:
> Can we just make this a set?

No, the order matters and the uniqueness only needs to be enforced when it's
created. I added a comment to make this more clear.

[tony, 2011-08-29 18:46:21]

webkit/glue LGTM

http://codereview.chromium.org/7745035/diff/10001/webkit/glue/webaccessibilit...
File webkit/glue/webaccessibility.cc (right):

http://codereview.chromium.org/7745035/diff/10001/webkit/glue/webaccessibilit...
webkit/glue/webaccessibility.cc:37: static std::string IntVectorToString(const
std::vector<int>& items) {
Nit: Chrome style prefers anonymous namespaces to static.

http://codereview.chromium.org/7745035/diff/10001/webkit/glue/webaccessibilit...
webkit/glue/webaccessibility.cc:333: std::string
WebAccessibility::DebugString(bool recursive) {
I think this is OK, but you might want to put DebugString and it's callers
behind an #ifndef NDEBUG to ensure that it's not compiled into release builds.

[David Tseng, 2011-08-29 19:05:34]

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility_win.cc (left):

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:41: }
nit: first_time_alert_?

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:429: for (unsigned
int i = 0; i < ia2_attributes_.size(); i++) {
nit: ++i

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:616: for (size_t i =
0; i < unique_cell_ids_.size(); i++) {
nit: ++i

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:1078:
*n_selected_cells = 0;
TODO? cells is an out param?

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:1091: *n_columns = 0;
Same here; TODO.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:1104: *n_rows = 0;
Same here.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:1700: return
E_NOTIMPL;
nit: TODO?

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2152: while (table &&
table->role() != WebAccessibility::ROLE_TABLE)
Should this ever happen? Will there ever be a cell whose parent is not a table?

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2157: for (size_t i =
0; i < unique_cell_ids.size(); i++) {
nit: ++i

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2157: for (size_t i =
0; i < unique_cell_ids.size(); i++) {
Also, if you change |unique_cell_ids| to a set, you can do


unique_cell_ids.find(renderer_id());
if (it != unique_cell_ids.end())
  index = it - unique_cell_ids.begin();

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2187: void
BrowserAccessibilityWin::SendNodeUpdateEvents() {
nit: new line above?

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2193: this);
nit: re-align -- 4 spaces).

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2199: if
(!previous_text_.empty() && !text.empty()) {
Is this necessary? What happens when the |previous_text| is cleared in the
|text|? Should we fire a hide event if !previous_text.empty() and text.empty()

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2438: } else {
;Shouldn't we check for this state explicitly?

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2478: ia_state_|=
STATE_SYSTEM_READONLY;
Should we be setting this state? Some screen readers will actually say
"read-only" when this is set (i.e. ok button readonly).

http://codereview.chromium.org/7745035/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7745035/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:1652: LOG(INFO) << "Accessibility update: \n"
Might be useful to also output
|param.notification_type| and
|routing_id_|

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc
File webkit/glue/webaccessibility.cc (right):

http://codereview.chromium.org/7745035/diff/1/webkit/glue/webaccessibility.cc...
webkit/glue/webaccessibility.cc:814: if (unique_cell_id_set.find(cell_id) ==
unique_cell_id_set.end()) {
NIT: IS THIS NECESSSARY?

[dmazzoni, 2011-08-29 19:38:35]

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility_win.cc (left):

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:41: }
On 2011/08/29 19:05:34, David Tseng wrote:
> nit: first_time_alert_?

It's more generally useful, I think I'll be using it for other events soon, so
I'd rather just call it first_time_.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:429: for (unsigned
int i = 0; i < ia2_attributes_.size(); i++) {
On 2011/08/29 19:05:34, David Tseng wrote:
> nit: ++i

Done.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:616: for (size_t i =
0; i < unique_cell_ids_.size(); i++) {
On 2011/08/29 19:05:34, David Tseng wrote:
> nit: ++i

Done.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:1078:
*n_selected_cells = 0;
On 2011/08/29 19:05:34, David Tseng wrote:
> TODO? cells is an out param?

Done.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:1091: *n_columns = 0;
On 2011/08/29 19:05:34, David Tseng wrote:
> Same here; TODO.

Done.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:1104: *n_rows = 0;
On 2011/08/29 19:05:34, David Tseng wrote:
> Same here.

Done.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:1700: return
E_NOTIMPL;
On 2011/08/29 19:05:34, David Tseng wrote:
> nit: TODO?

Done.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2152: while (table &&
table->role() != WebAccessibility::ROLE_TABLE)
On 2011/08/29 19:05:34, David Tseng wrote:
> Should this ever happen? Will there ever be a cell whose parent is not a
table?

The parent should be a row, and the row's parent should be a table. I'll add an
assert.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2157: for (size_t i =
0; i < unique_cell_ids.size(); i++) {
On 2011/08/29 19:05:34, David Tseng wrote:
> nit: ++i

Done.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2157: for (size_t i =
0; i < unique_cell_ids.size(); i++) {
On 2011/08/29 19:05:34, David Tseng wrote:
> Also, if you change |unique_cell_ids| to a set, you can do
> 
> 
> unique_cell_ids.find(renderer_id());
> if (it != unique_cell_ids.end())
>   index = it - unique_cell_ids.begin();

Yeah, but unfortunately the order matters. I documented this better in
webaccessibility.h.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2199: if
(!previous_text_.empty() && !text.empty()) {
On 2011/08/29 19:05:34, David Tseng wrote:
> Is this necessary? What happens when the |previous_text| is cleared in the
> |text|? Should we fire a hide event if !previous_text.empty() and text.empty()

This is the only way I can get JAWS to announce live region changes when the
text changes. I think that IA2_EVENT_TEXT_CHANGED is the right way to do this,
but JAWS doesn't seem to support it.

FWIW, Firefox seems to send both.

I'll add a TODO to look into HIDE events, but I found that Firefox+NVDA/JAWS
doesn't work correctly with live regions that are supposed to announce removals,
at least as far as I understand the spec, so I want to investigate that further
before I implement it possibly incorrectly.

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2438: } else {
On 2011/08/29 19:05:34, David Tseng wrote:
> ;Shouldn't we check for this state explicitly?

Isn't everything either horizontal or vertical? Is there a reason to allow
something to be neither or both?

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2478: ia_state_|=
STATE_SYSTEM_READONLY;
On 2011/08/29 19:05:34, David Tseng wrote:
> Should we be setting this state? Some screen readers will actually say
> "read-only" when this is set (i.e. ok button readonly).

So actually I was having the opposite problem: I tried to take away READONLY
from everything except controls explicitly marked as uneditable, but that made
Chrome totally unusable. Apparently JAWS doesn't like things like a LIST or
TABLE to be not marked as READONLY. So, I tried to mark everything that doesn't
have a value to change as READONLY.

http://codereview.chromium.org/7745035/diff/10001/webkit/glue/webaccessibilit...
File webkit/glue/webaccessibility.cc (right):

http://codereview.chromium.org/7745035/diff/10001/webkit/glue/webaccessibilit...
webkit/glue/webaccessibility.cc:37: static std::string IntVectorToString(const
std::vector<int>& items) {
On 2011/08/29 18:46:21, tony wrote:
> Nit: Chrome style prefers anonymous namespaces to static.

Done.

http://codereview.chromium.org/7745035/diff/10001/webkit/glue/webaccessibilit...
webkit/glue/webaccessibility.cc:333: std::string
WebAccessibility::DebugString(bool recursive) {
On 2011/08/29 18:46:21, tony wrote:
> I think this is OK, but you might want to put DebugString and it's callers
> behind an #ifndef NDEBUG to ensure that it's not compiled into release builds.

Sure, done.

[David Tseng, 2011-08-29 23:57:09]

LGTM

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
File content/browser/accessibility/browser_accessibility_win.cc (right):

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2438: } else {
On 2011/08/29 19:38:35, Dominic Mazzoni wrote:

Looks like someone recently added these properties to the WebAccessibilityObject
api? I guess you would know better here. Why wasn't a "isHorizontal" api also
added? I could dream up scenarios where a role isn't either horizontal or
vertical or is both, but I'm not sure what webkit says :).

http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
content/browser/accessibility/browser_accessibility_win.cc:2478: ia_state_|=
STATE_SYSTEM_READONLY;
On 2011/08/29 19:38:35, Dominic Mazzoni wrote:

We should look into why this isn't working without setting the read only state
(see crbug.com/89185 as a negative consequence of doing this). Jaws appends
"read only" to every role type when tabbing in forms mode.

[David Tseng, 2011-08-30 00:55:04]

>
>
>
http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
> File content/browser/accessibility/browser_accessibility_win.cc (right):
>
>
http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/b...
> content/browser/accessibility/browser_accessibility_win.cc:2438: } else
> {
> On 2011/08/29 19:38:35, Dominic Mazzoni wrote:
>
> Looks like someone recently added these properties to the
> WebAccessibilityObject api? I guess you would know better here. Why
> wasn't a "isHorizontal" api also added? I could dream up scenarios where
> a role isn't either horizontal or vertical or is both, but I'm not sure
> what webkit says :).
>
http://www.opensource.apple.com/source/WebCore/WebCore-7534.48.3/accessibilit...

Looks like orientation is only ever horizontal or vertical with
horizontal getting precedence in case of a tie., so this looks good

[dmazzoni, 2011-08-30 18:59:16]

On Mon, Aug 29, 2011 at 4:57 PM, <dtseng@chromium.org> wrote:

> Looks like someone recently added these properties to the
> WebAccessibilityObject api?


Yes, I submitted a WebKit changelist recently to expose a lot more things in
WebAccessibilityObject, including this.

We should look into why this isn't working without setting the read only
> state (see crbug.com/89185 as a negative consequence of doing this).
> Jaws appends "read only" to every role type when tabbing in forms mode.
>

FWIW, Firefox marks most non-controls as read-only too, so I think that's
the established convention. For now all controls that can have their value
changed are not read only.

- Dominic

[David Tseng, 2011-08-30 19:34:10]

>
> We should look into why this isn't working without setting the read only
>> state (see crbug.com/89185 as a negative consequence of doing this).
>> Jaws appends "read only" to every role type when tabbing in forms mode.
>>
>
> FWIW, Firefox marks most non-controls as read-only too, so I think that's
> the established convention. For now all controls that can have their value
> changed are not read only.
>
>

Looked at this with AccEvent and the FF5 focus events are fired on
IAccessible's whose acc state isn't read-only. Perhaps FF removes this
state before firing focus events?

FF5
OBJ_FOCUS	 Name="Google Search" Role=push button State=focused,default,focusable

Chrome Canary
OBJ_FOCUS	 Name="Google Search" Role=push button State=focused,read
only,focusable
Anyhow, we're tracking the bug still and the issue shouldn't block this cl.

[Chris Guillory, 2011-08-31 02:14:53]

LGTM.

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility.h (right):

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.h:25: typedef
std::map<WebAccessibility::BoolAttribute, bool> BoolAttrMap;
Optional: Attr -> Attribute

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager.cc:149: // All other
notifications
Optional: Mention in comment that children are not included.

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

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:726:
manager_->GetFromRendererID(cell_id));
toBrowserAccessibilityWin()

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:908:
BrowserAccessibilityWin* cell = static_cast<BrowserAccessibilityWin*>(
toBrowserAccessibilityWin()

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:1162: DCHECK(false);
NOTREACHED()

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:1260: DCHECK(false);
NOTREACHED()

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:1385: DCHECK(false);
NOTREACHED()

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:1389: *table =
static_cast<IAccessibleTable*>(
toBrowserAccessibilityWin(). IAccessibleTable* cast is needed?

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:1622: new_text->end =
static_cast<long>(text.size());
Verification: string16 keeps a local for the text size and doesn't compute it
correct?

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:2184: DCHECK(false);
NOTREACHED()

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:2218: //
TODO(dmazzoni): look into HIDE events, too.
Nit: look -> Look

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:2336: for (int j = 0;
j < static_cast<int>(line_breaks_.size()); ++j) {
Verification: This is a no effect change on released builds correct? Any reason
to do this?

http://codereview.chromium.org/7745035/diff/26001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/7745035/diff/26001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.cc:1270: if
(param.notification_type == ViewHostMsg_AccEvent::LOAD_COMPLETE) {
We can combine these two if statements now.

http://codereview.chromium.org/7745035/diff/26001/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7745035/diff/26001/content/renderer/render_vie...
content/renderer/render_view.cc:1643: WebAccessibilityObject obj =
accessibility_->getObjectById(
Doesn't this fit without wrapping?

http://codereview.chromium.org/7745035/diff/26001/content/renderer/render_vie...
content/renderer/render_view.cc:1650:
pending_accessibility_notifications_[i].type,
Doesn't this fit without wrapping?

[dmazzoni, 2011-08-31 22:02:53]

Addressed all comments, and also made some changes to
browser_accessibility_cocoa.mm to make it compile. I tried to map a few of the
events while I was at it, couldn't hurt.

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager.cc:149: // All other
notifications
On 2011/08/31 02:14:53, Chris Guillory wrote:
> Optional: Mention in comment that children are not included.

Done.

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

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:726:
manager_->GetFromRendererID(cell_id));
On 2011/08/31 02:14:53, Chris Guillory wrote:
> toBrowserAccessibilityWin()

Done.

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:1260: DCHECK(false);
On 2011/08/31 02:14:53, Chris Guillory wrote:
> NOTREACHED()

Done.

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:1389: *table =
static_cast<IAccessibleTable*>(
On 2011/08/31 02:14:53, Chris Guillory wrote:
> toBrowserAccessibilityWin(). IAccessibleTable* cast is needed?

Switched to use toBrowserAccessibilityWin(), but the cast to IAccessibleTable is
needed because there are multiple inheritance paths to IUnknown.  :(

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:1622: new_text->end =
static_cast<long>(text.size());
On 2011/08/31 02:14:53, Chris Guillory wrote:
> Verification: string16 keeps a local for the text size and doesn't compute it
> correct?

Correct, this is O(1)

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:2218: //
TODO(dmazzoni): look into HIDE events, too.
On 2011/08/31 02:14:53, Chris Guillory wrote:
> Nit: look -> Look

Done.

http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_win.cc:2336: for (int j = 0;
j < static_cast<int>(line_breaks_.size()); ++j) {
On 2011/08/31 02:14:53, Chris Guillory wrote:
> Verification: This is a no effect change on released builds correct? Any
reason
> to do this?

Our style guide slightly prefers ++j, but yes, it should make no difference for
an int. I'm just making this file consistent in style.

http://codereview.chromium.org/7745035/diff/26001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/7745035/diff/26001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.cc:1270: if
(param.notification_type == ViewHostMsg_AccEvent::LOAD_COMPLETE) {
On 2011/08/31 02:14:53, Chris Guillory wrote:
> We can combine these two if statements now.

Done.

http://codereview.chromium.org/7745035/diff/26001/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7745035/diff/26001/content/renderer/render_vie...
content/renderer/render_view.cc:1643: WebAccessibilityObject obj =
accessibility_->getObjectById(
On 2011/08/31 02:14:53, Chris Guillory wrote:
> Doesn't this fit without wrapping?

You're right, looks like my Windows editor was set to 79, not 80.