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

Issue 2750533006: Initial skeleton of Accessibility Object Model Phase 1 (Closed)

Created:
3 years, 9 months ago by dmazzoni
Modified:
3 years, 8 months ago
Reviewers:
haraken, esprehn, aboxhall
CC:
aboxhall+watch_chromium.org, blink-reviews, chromium-reviews, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, haraken, je_julie, kinuko+watch, nektar+watch_chromium.org, nektarios, yuzo+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Initial skeleton of Accessibility Object Model Phase 1 Explainer: https://github.com/WICG/aom/blob/master/explainer.md Spec: https://wicg.github.io/aom/spec/ This change adds an accessibleNode getter on Element, and implements the role and label properties of AccessibleNode, including a LayoutTest of the supported functionality. Properties on an AccessibleNode are stored in a map for efficiency since there are dozens of possible properties and few will need to be set on any one particular object. In existing accessibility code, places where we previously retrieve an ARIA attribute are replaced with a new wrapper that first checks the ARIA attribute and then checks the equivalent AOM property. Once this change lands it should be relatively straightforward to implement the rest of the properties of AccessibleNode and complete Phase 1. BUG=680345 Review-Url: https://codereview.chromium.org/2750533006 Cr-Commit-Position: refs/heads/master@{#460670} Committed: https://chromium.googlesource.com/chromium/src/+/59b48ec0a88a1716bd4b39b2ac2d3021458749f4

Patch Set 1 #

Total comments: 16

Patch Set 2 : Address feedback and change precedence order #

Patch Set 3 : Moved from modules/accessibility to core/dom #

Patch Set 4 : Update webexposed expectations #

Total comments: 2

Patch Set 5 : Add comment about ugly AXObjectCache interface #

Total comments: 29

Patch Set 6 : Addressed feedback, removed validation #

Total comments: 8

Patch Set 7 : Address feedback #

Total comments: 10

Patch Set 8 : Use vector of pairs, add gc test #

Total comments: 8

Patch Set 9 : Make m_element a Member, not a WeakMember #

Unified diffs Side-by-side diffs Delta from patch set Stats (+443 lines, -8 lines) Patch
A third_party/WebKit/LayoutTests/accessibility/aom.html View 1 2 3 4 5 6 7 8 1 chunk +196 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/element-instance-property-listing-expected.txt View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt View 1 2 3 4 5 6 7 8 2 chunks +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/core_idl_files.gni View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/AccessibleNode.h View 1 2 3 4 5 6 7 8 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/AccessibleNode.cpp View 1 2 3 4 5 6 7 8 1 chunk +85 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/dom/AccessibleNode.idl View 1 2 1 chunk +13 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Element.h View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Element.cpp View 1 2 3 4 5 6 7 8 1 chunk +18 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Element.idl View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/ElementRareData.h View 1 2 3 4 5 6 7 8 4 chunks +13 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/dom/ElementRareData.cpp View 1 2 3 4 5 3 chunks +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp View 1 2 3 4 5 5 chunks +11 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/modules/accessibility/AXObject.h View 1 2 3 4 5 6 7 8 2 chunks +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/accessibility/AXObject.cpp View 1 2 3 4 5 6 7 8 3 chunks +12 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h View 2 chunks +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 72 (38 generated)
dmazzoni
Alice, please take an initial look. I'll send out the Intent to Implement once the ...
3 years, 9 months ago (2017-03-15 21:57:20 UTC) #2
aboxhall
https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTests/accessibility/aom.html File third_party/WebKit/LayoutTests/accessibility/aom.html (right): https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTests/accessibility/aom.html#newcode19 third_party/WebKit/LayoutTests/accessibility/aom.html:19: test(function(t) { I imagine we'll want to split these ...
3 years, 9 months ago (2017-03-16 05:49:34 UTC) #3
dmazzoni
Changed the order of precedence and addressed other feedback. The main open question is who ...
3 years, 9 months ago (2017-03-16 20:43:17 UTC) #4
dmazzoni
Changed the order of precedence and addressed other feedback. The main open question is who ...
3 years, 9 months ago (2017-03-16 20:43:19 UTC) #5
aboxhall
On 2017/03/16 20:43:17, dmazzoni wrote: > Changed the order of precedence and addressed other > ...
3 years, 9 months ago (2017-03-16 23:42:59 UTC) #10
aboxhall
On 2017/03/16 23:42:59, aboxhall wrote: > On 2017/03/16 20:43:17, dmazzoni wrote: > > Changed the ...
3 years, 9 months ago (2017-03-16 23:43:40 UTC) #11
dmazzoni
+esprehn Moved to core/dom, ready for another look.
3 years, 9 months ago (2017-03-17 21:13:16 UTC) #18
aboxhall
LGTM, but what do you think about my proposal here: https://github.com/WICG/aom/issues/60#issuecomment-287607857 ? https://codereview.chromium.org/2750533006/diff/60001/third_party/WebKit/Source/core/dom/AccessibleNode.cpp File third_party/WebKit/Source/core/dom/AccessibleNode.cpp ...
3 years, 9 months ago (2017-03-20 04:22:52 UTC) #21
aboxhall
On 2017/03/20 04:22:52, aboxhall wrote: > LGTM, but what do you think about my proposal ...
3 years, 9 months ago (2017-03-20 05:05:00 UTC) #22
dmazzoni
https://codereview.chromium.org/2750533006/diff/60001/third_party/WebKit/Source/core/dom/AccessibleNode.cpp File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/60001/third_party/WebKit/Source/core/dom/AccessibleNode.cpp#newcode55 third_party/WebKit/Source/core/dom/AccessibleNode.cpp:55: cache->handleAttributeChanged(HTMLNames::roleAttr, m_element); On 2017/03/20 04:22:51, aboxhall wrote: > Hm, ...
3 years, 9 months ago (2017-03-21 21:45:06 UTC) #23
dmazzoni
On 2017/03/20 05:05:00, aboxhall wrote: > On 2017/03/20 04:22:52, aboxhall wrote: > > LGTM, but ...
3 years, 9 months ago (2017-03-21 21:46:42 UTC) #24
aboxhall
On 2017/03/21 21:46:42, dmazzoni wrote: > On 2017/03/20 05:05:00, aboxhall wrote: > > On 2017/03/20 ...
3 years, 9 months ago (2017-03-21 22:42:03 UTC) #25
esprehn
https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Source/core/dom/AXUtils.h File third_party/WebKit/Source/core/dom/AXUtils.h (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Source/core/dom/AXUtils.h#newcode2 third_party/WebKit/Source/core/dom/AXUtils.h:2: * Copyright (C) 2017 Google Inc. All rights reserved. ...
3 years, 9 months ago (2017-03-22 03:40:32 UTC) #26
dmazzoni
Ready for a fresh look. Addressed feedback, but some of it is now moot because ...
3 years, 9 months ago (2017-03-23 03:36:41 UTC) #30
haraken
wrapper tracing LGTM https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Source/core/dom/AccessibleNode.h File third_party/WebKit/Source/core/dom/AccessibleNode.h (right): https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Source/core/dom/AccessibleNode.h#newcode35 third_party/WebKit/Source/core/dom/AccessibleNode.h:35: AccessibleNode(Element*); Add explicit. https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Source/core/dom/ElementRareData.h File third_party/WebKit/Source/core/dom/ElementRareData.h ...
3 years, 9 months ago (2017-03-23 04:21:19 UTC) #33
aboxhall
LGTM with comments. I'm not 100% sold on the no validation thing yet (per #70 ...
3 years, 9 months ago (2017-03-23 04:34:23 UTC) #34
aboxhall
LGTM with comments. I'm not 100% sold on the no validation thing yet (per #70 ...
3 years, 9 months ago (2017-03-23 04:34:25 UTC) #35
dmazzoni
https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/LayoutTests/accessibility/aom.html File third_party/WebKit/LayoutTests/accessibility/aom.html (right): https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/LayoutTests/accessibility/aom.html#newcode158 third_party/WebKit/LayoutTests/accessibility/aom.html:158: assert_equals(axButton.name, "AOM"); On 2017/03/23 04:34:23, aboxhall wrote: > Add ...
3 years, 9 months ago (2017-03-23 17:06:49 UTC) #37
esprehn
https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Source/core/dom/AccessibleNode.cpp File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Source/core/dom/AccessibleNode.cpp#newcode28 third_party/WebKit/Source/core/dom/AccessibleNode.cpp:28: if (accessibleNode->m_stringPropertyDirty[propertyIndex]) value = accessibleNode->m_stringProperties[index]; if (!value.isNull()) return value; ...
3 years, 9 months ago (2017-03-24 01:59:40 UTC) #42
aboxhall
https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Source/core/dom/AccessibleNode.h File third_party/WebKit/Source/core/dom/AccessibleNode.h (right): https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Source/core/dom/AccessibleNode.h#newcode52 third_party/WebKit/Source/core/dom/AccessibleNode.h:52: bool m_stringPropertyDirty[kAOMStringPropertyCount] = {false}; On 2017/03/24 01:59:39, esprehn wrote: ...
3 years, 9 months ago (2017-03-24 02:17:51 UTC) #43
aboxhall
3 years, 9 months ago (2017-03-24 02:17:54 UTC) #44
esprehn
On 2017/03/24 at 02:17:51, aboxhall wrote: > https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Source/core/dom/AccessibleNode.h > File third_party/WebKit/Source/core/dom/AccessibleNode.h (right): > > https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Source/core/dom/AccessibleNode.h#newcode52 ...
3 years, 9 months ago (2017-03-24 06:42:04 UTC) #45
dmazzoni
https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Source/core/dom/AccessibleNode.cpp File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Source/core/dom/AccessibleNode.cpp#newcode49 third_party/WebKit/Source/core/dom/AccessibleNode.cpp:49: if (!m_element) On 2017/03/24 01:59:39, esprehn wrote: > How ...
3 years, 9 months ago (2017-03-24 17:51:12 UTC) #48
dmazzoni
Friendly ping. How is this patchset looking?
3 years, 8 months ago (2017-03-28 15:10:17 UTC) #52
esprehn
The AOM objects need to keep strong references to the Element, otherwise you're exposing GC ...
3 years, 8 months ago (2017-03-28 21:39:58 UTC) #53
esprehn
I think you can just use Member<Element> in there? Why did you decide to use ...
3 years, 8 months ago (2017-03-28 21:40:32 UTC) #54
dmazzoni
> The AOM objects need to keep strong references to the Element, > otherwise you're ...
3 years, 8 months ago (2017-03-29 07:00:42 UTC) #55
esprehn
lgtm, thanks for being so patient! This is how I feel about this patch: http://i.imgur.com/s4nh07b.gif
3 years, 8 months ago (2017-03-29 18:12:23 UTC) #60
dmazzoni
Great, thanks for your excellent guidance!
3 years, 8 months ago (2017-03-29 18:30:24 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2750533006/160001
3 years, 8 months ago (2017-03-29 18:31:31 UTC) #64
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/182759)
3 years, 8 months ago (2017-03-30 00:05:46 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2750533006/160001
3 years, 8 months ago (2017-03-30 04:11:18 UTC) #68
commit-bot: I haz the power
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/59b48ec0a88a1716bd4b39b2ac2d3021458749f4
3 years, 8 months ago (2017-03-30 05:15:14 UTC) #71
Will Harris
3 years, 8 months ago (2017-04-01 21:58:31 UTC) #72
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2796553002/ by wfh@chromium.org.

The reason for reverting is: major crashes on renderer see crbug.com/707218.

Powered by Google App Engine
This is Rietveld 408576698