Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(9)

Issue 7745035: Add a big grab bag of missing web accessibility functionality... (Closed)

Created:
9 years, 4 months ago by dmazzoni
Modified:
9 years, 3 months ago
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, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, ctguil+watch_chromium.org, zork+watch_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 80

Patch Set 2 : '' #

Total comments: 4

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Patch Set 7 : '' #

Total comments: 24

Patch Set 8 : '' #

Patch Set 9 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1474 lines, -327 lines) Patch
M chrome/browser/accessibility/accessibility_win_browsertest.cc View 1 2 3 4 5 6 7 5 chunks +6 lines, -9 lines 0 comments Download
M chrome/browser/accessibility/browser_accessibility_manager_unittest.cc View 1 2 3 4 5 6 7 4 chunks +4 lines, -8 lines 0 comments Download
M chrome/browser/accessibility/browser_accessibility_win_unittest.cc View 1 2 3 4 5 6 7 2 chunks +2 lines, -6 lines 0 comments Download
M content/browser/accessibility/browser_accessibility.h View 1 2 3 4 5 6 7 7 chunks +51 lines, -11 lines 0 comments Download
M content/browser/accessibility/browser_accessibility.cc View 1 2 3 4 5 6 7 6 chunks +66 lines, -12 lines 0 comments Download
M content/browser/accessibility/browser_accessibility_manager.h View 1 2 3 4 5 6 7 4 chunks +13 lines, -12 lines 0 comments Download
M content/browser/accessibility/browser_accessibility_manager.cc View 1 2 3 4 5 6 7 9 chunks +60 lines, -79 lines 0 comments Download
M content/browser/accessibility/browser_accessibility_manager_mac.mm View 1 2 3 4 5 6 7 8 1 chunk +52 lines, -14 lines 0 comments Download
M content/browser/accessibility/browser_accessibility_manager_win.cc View 1 2 3 4 5 6 7 1 chunk +30 lines, -14 lines 0 comments Download
M content/browser/accessibility/browser_accessibility_win.h View 1 2 3 4 5 6 7 9 chunks +77 lines, -11 lines 0 comments Download
M content/browser/accessibility/browser_accessibility_win.cc View 1 2 3 4 5 6 7 47 chunks +464 lines, -98 lines 0 comments Download
M content/browser/renderer_host/browser_render_process_host.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_view_host.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -5 lines 0 comments Download
M content/common/content_switches.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M content/common/content_switches.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M content/common/view_messages.h View 1 2 3 4 5 6 7 5 chunks +57 lines, -11 lines 0 comments Download
M content/renderer/render_view.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/render_view.cc View 1 2 3 4 5 6 7 6 chunks +48 lines, -17 lines 0 comments Download
M webkit/glue/webaccessibility.h View 1 2 3 4 5 6 7 7 chunks +56 lines, -5 lines 0 comments Download
M webkit/glue/webaccessibility.cc View 1 2 3 4 5 6 7 10 chunks +477 lines, -15 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
dmazzoni
dtseng: primary reviewer ctguil: additional a11y review if you have time jam: OWNERS for content/common ...
9 years, 4 months ago (2011-08-25 21:45:19 UTC) #1
David Tseng
First pass. http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/browser_accessibility.cc File content/browser/accessibility/browser_accessibility.cc (right): http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/browser_accessibility.cc#newcode185 content/browser/accessibility/browser_accessibility.cc:185: this); I think you want to align ...
9 years, 4 months ago (2011-08-26 16:14:37 UTC) #2
jam
content\common and content\renderer lgtm
9 years, 4 months ago (2011-08-26 16:54:31 UTC) #3
tony
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#newcode38 webkit/glue/webaccessibility.cc:38: return b? "true": "false"; Nit: Space between b and ...
9 years, 4 months ago (2011-08-27 00:05:46 UTC) #4
dmazzoni
Addressed all comments. I also shortened the enum constants in content/common/view_messages.h so that they fit ...
9 years, 3 months ago (2011-08-29 18:08:50 UTC) #5
tony
webkit/glue LGTM http://codereview.chromium.org/7745035/diff/10001/webkit/glue/webaccessibility.cc File webkit/glue/webaccessibility.cc (right): http://codereview.chromium.org/7745035/diff/10001/webkit/glue/webaccessibility.cc#newcode37 webkit/glue/webaccessibility.cc:37: static std::string IntVectorToString(const std::vector<int>& items) { Nit: ...
9 years, 3 months ago (2011-08-29 18:46:21 UTC) #6
David Tseng
http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/browser_accessibility_win.cc File content/browser/accessibility/browser_accessibility_win.cc (left): http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/browser_accessibility_win.cc#oldcode41 content/browser/accessibility/browser_accessibility_win.cc:41: } nit: first_time_alert_? http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/browser_accessibility_win.cc File content/browser/accessibility/browser_accessibility_win.cc (right): http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/browser_accessibility_win.cc#newcode429 content/browser/accessibility/browser_accessibility_win.cc:429: ...
9 years, 3 months ago (2011-08-29 19:05:34 UTC) #7
dmazzoni
http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/browser_accessibility_win.cc File content/browser/accessibility/browser_accessibility_win.cc (left): http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/browser_accessibility_win.cc#oldcode41 content/browser/accessibility/browser_accessibility_win.cc:41: } On 2011/08/29 19:05:34, David Tseng wrote: > nit: ...
9 years, 3 months ago (2011-08-29 19:38:35 UTC) #8
David Tseng
LGTM http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/browser_accessibility_win.cc File content/browser/accessibility/browser_accessibility_win.cc (right): http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/browser_accessibility_win.cc#newcode2438 content/browser/accessibility/browser_accessibility_win.cc:2438: } else { On 2011/08/29 19:38:35, Dominic Mazzoni ...
9 years, 3 months ago (2011-08-29 23:57:09 UTC) #9
David Tseng
> > > http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/browser_accessibility_win.cc > File content/browser/accessibility/browser_accessibility_win.cc (right): > > http://codereview.chromium.org/7745035/diff/1/content/browser/accessibility/browser_accessibility_win.cc#newcode2438 > content/browser/accessibility/browser_accessibility_win.cc:2438: } else ...
9 years, 3 months ago (2011-08-30 00:55:04 UTC) #10
dmazzoni
On Mon, Aug 29, 2011 at 4:57 PM, <dtseng@chromium.org> wrote: > Looks like someone recently ...
9 years, 3 months ago (2011-08-30 18:59:16 UTC) #11
David Tseng
> > We should look into why this isn't working without setting the read only ...
9 years, 3 months ago (2011-08-30 19:34:10 UTC) #12
Chris Guillory
LGTM. http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibility/browser_accessibility.h File content/browser/accessibility/browser_accessibility.h (right): http://codereview.chromium.org/7745035/diff/26001/content/browser/accessibility/browser_accessibility.h#newcode25 content/browser/accessibility/browser_accessibility.h:25: typedef std::map<WebAccessibility::BoolAttribute, bool> BoolAttrMap; Optional: Attr -> Attribute ...
9 years, 3 months ago (2011-08-31 02:14:53 UTC) #13
dmazzoni
9 years, 3 months ago (2011-08-31 22:02:53 UTC) #14
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.

Powered by Google App Engine
This is Rietveld 408576698