|
|
Created:
3 years, 9 months ago by dmazzoni Modified:
3 years, 8 months ago 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. |
DescriptionInitial 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 #Dependent Patchsets: Messages
Total messages: 72 (38 generated)
dmazzoni@chromium.org changed reviewers: + aboxhall@chromium.org
Alice, please take an initial look. I'll send out the Intent to Implement once the latest spec changes are submitted.
https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/accessibility/aom.html (right): https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/accessibility/aom.html:19: test(function(t) { I imagine we'll want to split these out into separate files at some point once we get more in depth, but no point doing that for this change. https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/accessibility/aom.html:21: assert_true(button.nodeType == 1); // ELEMENT Why not just use Node.ELEMENT_NODE ? Also, could use assert_equals/not_equals for all of these (it uses ===). https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/accessibility/aom.html:28: assert_true(staticText.nodeType == 3); // TEXT Node.TEXT_NODE https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/accessibility/aom.html:105: </script> Do we want a test asserting the precedence order of AOM properties vs ARIA attributes? https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right): https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXObject.cpp:397: const AtomicString& AXObject::getAomPropertyOrAriaAttribute( maybe just getAuthorDefinedProperty? https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXObject.cpp:400: if (aomNode) { As discussed, swap this and the following block. https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/AccessibleNode.h (right): https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AccessibleNode.h:18: enum class AomStringProperty { I think the standard style is to capitalise all of AOM; cf. HTMLElement, DOMRect etc. https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/ElementAccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/ElementAccessibleNode.cpp:21: if (settings && !settings->getAccessibilityEnabled()) Wait, why do we need to do this? Can we decouple creating this object from switching on accessibility?
Changed the order of precedence and addressed other feedback. The main open question is who should own AccessibleNode. How could we have an Element own an AccessibleNode in a clean way? https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/accessibility/aom.html (right): https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/accessibility/aom.html:19: test(function(t) { On 2017/03/16 05:49:33, aboxhall wrote: > I imagine we'll want to split these out into separate files at some point once > we get more in depth, but no point doing that for this change. Agreed. For now it's simpler to have them in one place. https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/accessibility/aom.html:21: assert_true(button.nodeType == 1); // ELEMENT On 2017/03/16 05:49:33, aboxhall wrote: > Why not just use Node.ELEMENT_NODE ? > Also, could use assert_equals/not_equals for all of these (it uses ===). Done. https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/accessibility/aom.html:28: assert_true(staticText.nodeType == 3); // TEXT On 2017/03/16 05:49:33, aboxhall wrote: > Node.TEXT_NODE Done. https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/accessibility/aom.html:105: </script> On 2017/03/16 05:49:33, aboxhall wrote: > Do we want a test asserting the precedence order of AOM properties vs ARIA > attributes? The tests were kind of doing that implicitly. I added a new explicit test for that. https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right): https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXObject.cpp:397: const AtomicString& AXObject::getAomPropertyOrAriaAttribute( On 2017/03/16 05:49:34, aboxhall wrote: > maybe just getAuthorDefinedProperty? Hmmm, I wouldn't intuitively know how to interpret that. I'm up for something more concise, but I think that at least during the transition phase we should make this verbose enough that anyone reading the code realizes we're checking two places for an attribute/property. Renamed to getAriaAttributeOrAomProperty for now to reflect the correct precedence. https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXObject.cpp:400: if (aomNode) { On 2017/03/16 05:49:34, aboxhall wrote: > As discussed, swap this and the following block. Done. https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/AccessibleNode.h (right): https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AccessibleNode.h:18: enum class AomStringProperty { On 2017/03/16 05:49:34, aboxhall wrote: > I think the standard style is to capitalise all of AOM; cf. HTMLElement, DOMRect > etc. Changed throughout https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/ElementAccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/ElementAccessibleNode.cpp:21: if (settings && !settings->getAccessibilityEnabled()) On 2017/03/16 05:49:34, aboxhall wrote: > Wait, why do we need to do this? Can we decouple creating this object from > switching on accessibility? Hmmm, good point. Right now the AXObject for the Element owns the AccessibleNode. That's convenient because it's the one that needs to access it. Maybe it'd be better for the Element to own it, but it's not clear to me how we could do that technically without keeping accessibility code more decoupled. There are two issues: * We don't want to add another reference to every Element, we'd want to put it in RareData - but we want this code to live in modules/ not in core/ * Alternatively, we could keep track of a map from Element to AccessibleNode like we do for the rest of AX code, but then we'd need a callback when an Element is deleted / removed from the tree, and currently we do that if Accessibility is enabled. Do we want another separate callback system just for this independently of accessibility being enabled? This might be a good question for a Blink owner
Changed the order of precedence and addressed other feedback. The main open question is who should own AccessibleNode. How could we have an Element own an AccessibleNode in a clean way? More below.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/03/16 20:43:17, dmazzoni wrote: > Changed the order of precedence and addressed other > feedback. > > The main open question is who should own AccessibleNode. > How could we have an Element own an AccessibleNode in > a clean way? > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/accessibility/aom.html (right): > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/accessibility/aom.html:19: test(function(t) { > On 2017/03/16 05:49:33, aboxhall wrote: > > I imagine we'll want to split these out into separate files at some point once > > we get more in depth, but no point doing that for this change. > > Agreed. For now it's simpler to have them in one place. > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/accessibility/aom.html:21: > assert_true(button.nodeType == 1); // ELEMENT > On 2017/03/16 05:49:33, aboxhall wrote: > > Why not just use Node.ELEMENT_NODE ? > > Also, could use assert_equals/not_equals for all of these (it uses ===). > > Done. > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/accessibility/aom.html:28: > assert_true(staticText.nodeType == 3); // TEXT > On 2017/03/16 05:49:33, aboxhall wrote: > > Node.TEXT_NODE > > Done. > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/accessibility/aom.html:105: </script> > On 2017/03/16 05:49:33, aboxhall wrote: > > Do we want a test asserting the precedence order of AOM properties vs ARIA > > attributes? > > The tests were kind of doing that implicitly. > > I added a new explicit test for that. > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right): > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/accessibility/AXObject.cpp:397: const > AtomicString& AXObject::getAomPropertyOrAriaAttribute( > On 2017/03/16 05:49:34, aboxhall wrote: > > maybe just getAuthorDefinedProperty? > > Hmmm, I wouldn't intuitively know how to interpret that. > I'm up for something more concise, but I think that at least > during the transition phase we should make this verbose enough > that anyone reading the code realizes we're checking two places > for an attribute/property. > > Renamed to getAriaAttributeOrAomProperty for now to reflect > the correct precedence. > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/accessibility/AXObject.cpp:400: if (aomNode) { > On 2017/03/16 05:49:34, aboxhall wrote: > > As discussed, swap this and the following block. > > Done. > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/accessibility/AccessibleNode.h (right): > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/accessibility/AccessibleNode.h:18: enum class > AomStringProperty { > On 2017/03/16 05:49:34, aboxhall wrote: > > I think the standard style is to capitalise all of AOM; cf. HTMLElement, > DOMRect > > etc. > > Changed throughout > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/accessibility/ElementAccessibleNode.cpp > (right): > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/accessibility/ElementAccessibleNode.cpp:21: if > (settings && !settings->getAccessibilityEnabled()) > On 2017/03/16 05:49:34, aboxhall wrote: > > Wait, why do we need to do this? Can we decouple creating this object from > > switching on accessibility? > > Hmmm, good point. > > Right now the AXObject for the Element owns the AccessibleNode. > That's convenient because it's the one that needs to access it. > > Maybe it'd be better for the Element to own it, but it's not clear to me > how we could do that technically without keeping accessibility code more > decoupled. There are two issues: > > * We don't want to add another reference to every Element, we'd want to put > it in RareData - but we want this code to live in modules/ not in core/ > * Alternatively, we could keep track of a map from Element to AccessibleNode > like we do for the rest of AX code, but then we'd need a callback when an > Element is deleted / removed from the tree, and currently we do that if > Accessibility is enabled. Do we want another separate callback system just > for this independently of accessibility being enabled? > > This might be a good question for a Blink owner I'm chatting to Elliott about this (making as much use as possible of his visiting Sydney!) - will let you know where we end up. However, my feeling is that we should think of this as being analogous to an ARIA attribute - despite that feeding into the accessibility tree (and only the accessibility tree, really), it belongs to the DOM and doesn't require spinning up an AXObjectCache to track. Also, whether an AXObjectCache is ever spun up - whether it's even *possible* to spin one up - you should be able to set things on Element.accessibleNode (even if they never get read by anything).
On 2017/03/16 23:42:59, aboxhall wrote: > On 2017/03/16 20:43:17, dmazzoni wrote: > > Changed the order of precedence and addressed other > > feedback. > > > > The main open question is who should own AccessibleNode. > > How could we have an Element own an AccessibleNode in > > a clean way? > > > > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... > > File third_party/WebKit/LayoutTests/accessibility/aom.html (right): > > > > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/accessibility/aom.html:19: test(function(t) { > > On 2017/03/16 05:49:33, aboxhall wrote: > > > I imagine we'll want to split these out into separate files at some point > once > > > we get more in depth, but no point doing that for this change. > > > > Agreed. For now it's simpler to have them in one place. > > > > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/accessibility/aom.html:21: > > assert_true(button.nodeType == 1); // ELEMENT > > On 2017/03/16 05:49:33, aboxhall wrote: > > > Why not just use Node.ELEMENT_NODE ? > > > Also, could use assert_equals/not_equals for all of these (it uses ===). > > > > Done. > > > > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/accessibility/aom.html:28: > > assert_true(staticText.nodeType == 3); // TEXT > > On 2017/03/16 05:49:33, aboxhall wrote: > > > Node.TEXT_NODE > > > > Done. > > > > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/accessibility/aom.html:105: </script> > > On 2017/03/16 05:49:33, aboxhall wrote: > > > Do we want a test asserting the precedence order of AOM properties vs ARIA > > > attributes? > > > > The tests were kind of doing that implicitly. > > > > I added a new explicit test for that. > > > > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right): > > > > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/accessibility/AXObject.cpp:397: const > > AtomicString& AXObject::getAomPropertyOrAriaAttribute( > > On 2017/03/16 05:49:34, aboxhall wrote: > > > maybe just getAuthorDefinedProperty? > > > > Hmmm, I wouldn't intuitively know how to interpret that. > > I'm up for something more concise, but I think that at least > > during the transition phase we should make this verbose enough > > that anyone reading the code realizes we're checking two places > > for an attribute/property. > > > > Renamed to getAriaAttributeOrAomProperty for now to reflect > > the correct precedence. > > > > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/accessibility/AXObject.cpp:400: if (aomNode) > { > > On 2017/03/16 05:49:34, aboxhall wrote: > > > As discussed, swap this and the following block. > > > > Done. > > > > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/accessibility/AccessibleNode.h (right): > > > > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/accessibility/AccessibleNode.h:18: enum > class > > AomStringProperty { > > On 2017/03/16 05:49:34, aboxhall wrote: > > > I think the standard style is to capitalise all of AOM; cf. HTMLElement, > > DOMRect > > > etc. > > > > Changed throughout > > > > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/accessibility/ElementAccessibleNode.cpp > > (right): > > > > > https://codereview.chromium.org/2750533006/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/accessibility/ElementAccessibleNode.cpp:21: > if > > (settings && !settings->getAccessibilityEnabled()) > > On 2017/03/16 05:49:34, aboxhall wrote: > > > Wait, why do we need to do this? Can we decouple creating this object from > > > switching on accessibility? > > > > Hmmm, good point. > > > > Right now the AXObject for the Element owns the AccessibleNode. > > That's convenient because it's the one that needs to access it. > > > > Maybe it'd be better for the Element to own it, but it's not clear to me > > how we could do that technically without keeping accessibility code more > > decoupled. There are two issues: > > > > * We don't want to add another reference to every Element, we'd want to put > > it in RareData - but we want this code to live in modules/ not in core/ > > * Alternatively, we could keep track of a map from Element to AccessibleNode > > like we do for the rest of AX code, but then we'd need a callback when an > > Element is deleted / removed from the tree, and currently we do that if > > Accessibility is enabled. Do we want another separate callback system just > > for this independently of accessibility being enabled? > > > > This might be a good question for a Blink owner > > I'm chatting to Elliott about this (making as much use as possible of his > visiting Sydney!) - will let you know where we end up. > > However, my feeling is that we should think of this as being analogous to an > ARIA attribute - despite that feeding into the accessibility tree (and only the > accessibility tree, really), it belongs to the DOM and doesn't require spinning > up an AXObjectCache to track. Also, whether an AXObjectCache is ever spun up - > whether it's even *possible* to spin one up - you should be able to set things > on Element.accessibleNode (even if they never get read by anything). Forgot to lead back to the conclusion: Therefore I don't think this belongs in modules/accessibility - it's part of the DOM.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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 equivalent AOM property and then falls back on ARIA. Once this change lands it should be relatively straightforward to implement the rest of the properties of AccessibleNode and complete Phase 1. BUG=680345 ========== to ========== 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 ==========
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmazzoni@chromium.org changed reviewers: + esprehn@chromium.org
+esprehn Moved to core/dom, ready for another look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/Sour... File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:55: cache->handleAttributeChanged(HTMLNames::roleAttr, m_element); Hm, this is a bit icky - maybe add a TODO here for handling this more cleanly?
On 2017/03/20 04:22:52, aboxhall wrote: > LGTM, but what do you think about my proposal here: > https://github.com/WICG/aom/issues/60#issuecomment-287607857 ? A pleasant implementation side effect of this would be that we can also use AccessibleNode for everything internally, and not have to have the getAOMPropertyOrARIAAttribute method. > https://codereview.chromium.org/2750533006/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): > > https://codereview.chromium.org/2750533006/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/AccessibleNode.cpp:55: > cache->handleAttributeChanged(HTMLNames::roleAttr, m_element); > Hm, this is a bit icky - maybe add a TODO here for handling this more cleanly?
https://codereview.chromium.org/2750533006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/60001/third_party/WebKit/Sour... 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, this is a bit icky - maybe add a TODO here for handling this more cleanly? Done.
On 2017/03/20 05:05:00, aboxhall wrote: > On 2017/03/20 04:22:52, aboxhall wrote: > > LGTM, but what do you think about my proposal here: > > https://github.com/WICG/aom/issues/60#issuecomment-287607857 ? > > A pleasant implementation side effect of this would be that we can also use > AccessibleNode for everything internally, and not have to have the > getAOMPropertyOrARIAAttribute method. For the record: the proposal is about what exactly to reflect between ARIA attributes and AOM properties. Definitely open to this idea, still discussing it on the GitHub issue. It might make sense to land this change either way, but we should resolve that before moving forwards and finishing the rest of Phase 1.
On 2017/03/21 21:46:42, dmazzoni wrote: > On 2017/03/20 05:05:00, aboxhall wrote: > > On 2017/03/20 04:22:52, aboxhall wrote: > > > LGTM, but what do you think about my proposal here: > > > https://github.com/WICG/aom/issues/60#issuecomment-287607857 ? > > > > A pleasant implementation side effect of this would be that we can also use > > AccessibleNode for everything internally, and not have to have the > > getAOMPropertyOrARIAAttribute method. > > For the record: the proposal is about what exactly to reflect between > ARIA attributes and AOM properties. Definitely open to this idea, > still discussing it on the GitHub issue. > > It might make sense to land this change either way, but we should > resolve that before moving forwards and finishing the rest of Phase 1. Agreed, still LGTM.
https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/AXUtils.h (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AXUtils.h:2: * Copyright (C) 2017 Google Inc. All rights reserved. Short copyright in all new files. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AXUtils.h:40: class CORE_EXPORT AXUtils { We don't add generic Utils classes, this needs to be more specific to what it does. I'm also suspect of the virtual thing going on here, is this just because of the modules/ vs core/ split? https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:25: const AtomicString& result = m_stringProperties.at(property); I'd suggest using a vector instead, you probably won't have that many properties per thing, and a HashMap is a lot of overhead. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:31: if (!AXUtils::getInstance()->isValidRoleAttribute(result)) This feels like over abstraction going through a singleton and a virtual call, what's this for? https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:45: String AccessibleNode::role() const { return AtomicString https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:49: void AccessibleNode::setRole(const String& role) { Take an AtomicString here instead https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:61: String AccessibleNode::label() const { ditto https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:65: void AccessibleNode::setLabel(const String& label) { ditto https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/AccessibleNode.h (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.h:18: enum class AOMStringProperty { What is an AOMStringProperty? Can you ad comments to all the classes? https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.h:19: None, We've been prefixing with k for new ones. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.h:24: struct AOMStringPropertyHashTraits : WTF::GenericHashTraits<AOMStringProperty> { Can you explain why you need a custom hash config? These are pretty unusual and often are not needed. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ElementRareData.cpp (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementRareData.cpp:106: m_accessibleNode); // TODO: needed? yes you need it https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementRareData.h:163: ScriptWrappableVisitor::writeBarrier(this, m_accessibleNode); Yeah I have no idea what this is, haraken@ should probably comment. I think you do though, because we don't use TraceWrapperMember for the stuff in ElementRareData to avoid having tons of duplicate ptrs in here and bloating the size. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:622: getAriaAttributeOrAOMProperty(AOMStringProperty::Role); I'm skeptical of having a separate enum for every data type like this, but I guess it works. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXObject.cpp:390: map.set(AOMStringProperty::Role, roleAttr); Can we just use roleAttr as the key into the AOM map instead? You're doing a lot of mapping from X -> Y. If the AccessibleObject just stored a Vector or HashMap of AtomicString and you used roleAttr as the key that'd be much simpler. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXObject.cpp:391: map.set(AOMStringProperty::Label, aria_labelAttr); calling set() repeatedly inlines the hash function at every call site. You want a static array of pairs and to loop over it calling .set() instead if you want this map to get bigger. Since your whole thing is just a big table of enum -> AtomicString going through a hash table is super expensive though you could just use the enum index and do vector[enum] => AtomicString instead and avoid any hashing. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXObject.cpp:419: if (aomNode) We often combines these like: if (auto* node = getAccessibleNode()) return node->getProperty(property); https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXUtilsImpl.cpp (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXUtilsImpl.cpp:39: return AXObject::ariaRoleToWebCoreRole(role) != UnknownRole; ariaRoleToWebCoreRole is actually static, can we move it into core/ instead and use it directly and then modules/ can just use the stuff in core. Then we don't need this indirection.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmazzoni@chromium.org changed reviewers: + haraken@chromium.org
Ready for a fresh look. Addressed feedback, but some of it is now moot because we're not validating for now, which simplifies things somewhat. +haraken to check if ScriptWrappableVisitor::writeBarrier is correct. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/AXUtils.h (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AXUtils.h:40: class CORE_EXPORT AXUtils { On 2017/03/22 03:40:31, esprehn wrote: > We don't add generic Utils classes, this needs to be more specific to what it > does. Sure. Maybe AXPropertyValidator or something like that. The latest consensus is that we will not validate in phase 1 anymore, so I'm just removing this for now. > I'm also suspect of the virtual thing going on here, is this just because > of the modules/ vs core/ split? Yes, this is how AXObjectCache is initialized too, so the impl can live in modules/ https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:25: const AtomicString& result = m_stringProperties.at(property); On 2017/03/22 03:40:31, esprehn wrote: > I'd suggest using a vector instead, you probably won't have that many properties > per thing, and a HashMap is a lot of overhead. The reason I did this was memory. There are 48 properties, corresponding to 48 ARIA attributes, in phase 1, and probably a dozen more by the end of the spec. I didn't want every AccessibleNode to have space for 48+ values that would be mostly unused. Perhaps std::map would be less overhead but more sparse? In Chromium accessibility code we often use vector<pair<enum, value>>, that's another option. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:31: if (!AXUtils::getInstance()->isValidRoleAttribute(result)) On 2017/03/22 03:40:31, esprehn wrote: > This feels like over abstraction going through a singleton and a virtual call, > what's this for? Removed, we're not doing validation for now. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:45: String AccessibleNode::role() const { On 2017/03/22 03:40:31, esprehn wrote: > return AtomicString Done throughout. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/AccessibleNode.h (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.h:18: enum class AOMStringProperty { On 2017/03/22 03:40:31, esprehn wrote: > What is an AOMStringProperty? Can you ad comments to all the classes? Done. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.h:19: None, On 2017/03/22 03:40:32, esprehn wrote: > We've been prefixing with k for new ones. Done. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/AccessibleNode.h:24: struct AOMStringPropertyHashTraits : WTF::GenericHashTraits<AOMStringProperty> { On 2017/03/22 03:40:32, esprehn wrote: > Can you explain why you need a custom hash config? These are pretty unusual and > often are not needed. No longer needed as I switched to an array, as suggested. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ElementRareData.cpp (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementRareData.cpp:106: m_accessibleNode); // TODO: needed? On 2017/03/22 03:40:32, esprehn wrote: > yes you need it Acknowledged. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:622: getAriaAttributeOrAOMProperty(AOMStringProperty::Role); On 2017/03/22 03:40:32, esprehn wrote: > I'm skeptical of having a separate enum for every data type like this, but I > guess it works. We use this pattern elsewhere in accessibility code both to cut down on boilerplate code and to allow sparse representations. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right): https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXObject.cpp:390: map.set(AOMStringProperty::Role, roleAttr); On 2017/03/22 03:40:32, esprehn wrote: > Can we just use roleAttr as the key into the AOM map instead? You're doing a lot > of mapping from X -> Y. If the AccessibleObject just stored a Vector or HashMap > of AtomicString and you used roleAttr as the key that'd be much simpler. I got rid of this HashMap, instead now we can just switch in AccessibleNode. https://codereview.chromium.org/2750533006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXObject.cpp:419: if (aomNode) On 2017/03/22 03:40:32, esprehn wrote: > We often combines these like: > > if (auto* node = getAccessibleNode()) > return node->getProperty(property); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
wrapper tracing LGTM https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AccessibleNode.h (right): https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.h:35: AccessibleNode(Element*); Add explicit. https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementRareData.h:163: ScriptWrappableVisitor::writeBarrier(this, m_accessibleNode); If you need to keep alive AccessibleNode's wrapper as long as the corresponding Element's wrapper is alive, you need this. Otherwise, you don't need it. I guess you need it because the fact that the IDL file has Element.accessibleNode would imply that there's a strong reference from Element's wrapper to AccessibleNode's wrapper.
LGTM with comments. I'm not 100% sold on the no validation thing yet (per #70 on the repo) but I think it makes sense to address that separately in any case. https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/accessibility/aom.html (right): https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/accessibility/aom.html:158: assert_equals(axButton.name, "AOM"); Add a case checking that setting the attribute to something new (and valid) has no effect at this point (it's not "last one wins" regardless of whether AOM properties are null). https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right): https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/accessibility/AXObject.cpp:382: const AtomicString& AXObject::getAOMPropertyOrARIAAttribute( Does this method name still need to refer to ARIA attributes?
LGTM with comments. I'm not 100% sold on the no validation thing yet (per #70 on the repo) but I think it makes sense to address that separately in any case.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/accessibility/aom.html (right): https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/accessibility/aom.html:158: assert_equals(axButton.name, "AOM"); On 2017/03/23 04:34:23, aboxhall wrote: > Add a case checking that setting the attribute to something new (and valid) has > no effect at this point (it's not "last one wins" regardless of whether AOM > properties are null). Good idea, done. https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AccessibleNode.h (right): https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.h:35: AccessibleNode(Element*); On 2017/03/23 04:21:19, haraken wrote: > > Add explicit. Done. https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementRareData.h:163: ScriptWrappableVisitor::writeBarrier(this, m_accessibleNode); On 2017/03/23 04:21:19, haraken wrote: > > If you need to keep alive AccessibleNode's wrapper as long as the corresponding > Element's wrapper is alive, you need this. Otherwise, you don't need it. > > I guess you need it because the fact that the IDL file has > Element.accessibleNode would imply that there's a strong reference from > Element's wrapper to AccessibleNode's wrapper. That's correct. Thanks! https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right): https://codereview.chromium.org/2750533006/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/accessibility/AXObject.cpp:382: const AtomicString& AXObject::getAOMPropertyOrARIAAttribute( On 2017/03/23 04:34:23, aboxhall wrote: > Does this method name still need to refer to ARIA attributes? I prefer it during our transition period, so anyone else reading our code understands what's going on. Once we've completely migrated, I was thinking of adding a presubmit check to forbid use of getAttribute() with an ARIA attribute.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
esprehn@chromium.org changed reviewers: + haraken@chromium.org by - haraken@chromium.org
https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:28: if (accessibleNode->m_stringPropertyDirty[propertyIndex]) value = accessibleNode->m_stringProperties[index]; if (!value.isNull()) return value; https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:49: if (!m_element) How does the node still exist but the accessibleNode not exist? I don't think that should happen if the lifetime is correct. https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:54: m_stringPropertyDirty[kRoleIndex] = true; remove https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:69: m_stringPropertyDirty[kLabelIndex] = true; ditto https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AccessibleNode.h (right): https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.h:51: AtomicString m_stringProperties[kAOMStringPropertyCount]; fwiw I was actually suggesting using a Vector of pairs, since that avoids making your object huge when only a few properties are set, and the linear lookup cost is probably not that bad for this https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.h:52: bool m_stringPropertyDirty[kAOMStringPropertyCount] = {false}; I think you want a std::bitset ? I don't think you need this though, use the null value of AtomicString to signal if the value is set or not. assigning null from the AOM should be equivalent to doing a reset.
https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AccessibleNode.h (right): https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.h:52: bool m_stringPropertyDirty[kAOMStringPropertyCount] = {false}; On 2017/03/24 01:59:39, esprehn wrote: > I think you want a std::bitset ? > > I don't think you need this though, use the null value of AtomicString to signal > if the value is set or not. assigning null from the AOM should be equivalent to > doing a reset. We want this to work equivalently to value, where once you have set the property the attribute is ignored forever.
On 2017/03/24 at 02:17:51, aboxhall wrote: > https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/AccessibleNode.h (right): > > https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/AccessibleNode.h:52: bool m_stringPropertyDirty[kAOMStringPropertyCount] = {false}; > On 2017/03/24 01:59:39, esprehn wrote: > > I think you want a std::bitset ? > > > > I don't think you need this though, use the null value of AtomicString to signal > > if the value is set or not. assigning null from the AOM should be equivalent to > > doing a reset. > > We want this to work equivalently to value, where once you have set the property the attribute is ignored forever. That's a very strange pattern to copy, it means if you accidentally set the value you're stuck forever. I don't think you want that. In general <input> is not a good thing to copy concepts from.
dmazzoni@chromium.org changed reviewers: + haraken@chromium.org - haraken@chromium.org by
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:49: if (!m_element) On 2017/03/24 01:59:39, esprehn wrote: > How does the node still exist but the accessibleNode not exist? I don't think > that should happen if the lifetime is correct. Did you mean that backwards? The case we're guarding against here is that the element is deleted, but there's still a reference to the AccessibleNode. I added a test for this that explicitly calls gc(). Without null-checking m_element it crashes. https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AccessibleNode.h (right): https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.h:51: AtomicString m_stringProperties[kAOMStringPropertyCount]; On 2017/03/24 01:59:39, esprehn wrote: > fwiw I was actually suggesting using a Vector of pairs, since that avoids making > your object huge when only a few properties are set, and the linear lookup cost > is probably not that bad for this Oh good, I like that better. Do you see anyone else using this pattern? Maybe we should have a VectorMap class that stores a vector of pairs but presents a map-like interface. https://codereview.chromium.org/2750533006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.h:52: bool m_stringPropertyDirty[kAOMStringPropertyCount] = {false}; On 2017/03/24 02:17:51, aboxhall wrote: > On 2017/03/24 01:59:39, esprehn wrote: > > I think you want a std::bitset ? > > > > I don't think you need this though, use the null value of AtomicString to > signal > > if the value is set or not. assigning null from the AOM should be equivalent > to > > doing a reset. > > We want this to work equivalently to value, where once you have set the property > the attribute is ignored forever. Now that I'm using a vector of pairs, we can simply use presence of the property in the vector to indicate whether it's ever been set or not. No explicit bit field needed. As Alice said, the current plan is to make it work like value. That seems to be the rough consensus now so I want to implement it that way, but it's definitely not set in stone - feel free to make an argument for other behavior on the issue.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping. How is this patchset looking?
The AOM objects need to keep strong references to the Element, otherwise you're exposing GC timing to the page which we don't do. The AccessibilityObject and the Element its connected to must live the exact same amount of time. https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/accessibility/aom.html (right): https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/accessibility/aom.html:14: function gc() Is this not in the testharness.js somewhere? Can you file a bug? https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/accessibility/aom.html:207: assert_equals(aomButton.label, null); This is leaking gc behavior, the AOMObject needs to keep the Element alive. https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:24: DCHECK_LT(property, AOMStringProperty::kCount); Remove this and just static_assert somewhere? https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementRareData.h:162: // Note to reviewers: do I need this? Remove this comment.
I think you can just use Member<Element> in there? Why did you decide to use Weak* today?
> The AOM objects need to keep strong references to the Element, > otherwise you're exposing GC timing to the page which we don't do. > The AccessibilityObject and the Element its connected to must live the > exact same amount of time. Thanks, I didn't think about exposing gc timing, that makes sense. I was used to using weak references elsewhere in accessibility code but this is different because it's exposed as a platform API. Fixed. https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/accessibility/aom.html (right): https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/accessibility/aom.html:14: function gc() On 2017/03/28 21:39:58, esprehn wrote: > Is this not in the testharness.js somewhere? Can you file a bug? Ah, found something in resources/gc.js. I filed http://crbug.com/706285 to clean up existing layout tests that redefine it. https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/accessibility/aom.html:207: assert_equals(aomButton.label, null); On 2017/03/28 21:39:58, esprehn wrote: > This is leaking gc behavior, the AOMObject needs to keep the Element alive. Done. https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right): https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/AccessibleNode.cpp:24: DCHECK_LT(property, AOMStringProperty::kCount); On 2017/03/28 21:39:58, esprehn wrote: > Remove this and just static_assert somewhere? Good point, actually those were really only needed when I needed to treat the num as an int. Removed. https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/2750533006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementRareData.h:162: // Note to reviewers: do I need this? On 2017/03/28 21:39:58, esprehn wrote: > Remove this comment. Done.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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/bui...)
lgtm, thanks for being so patient! This is how I feel about this patch: http://i.imgur.com/s4nh07b.gif
Great, thanks for your excellent guidance!
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, aboxhall@chromium.org Link to the patchset: https://codereview.chromium.org/2750533006/#ps160001 (title: "Make m_element a Member, not a WeakMember")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/bui...)
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490847044625960, "parent_rev": "fac29ec2345d1f010a70ea8a462010a6f803e4e2", "commit_rev": "59b48ec0a88a1716bd4b39b2ac2d3021458749f4"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/59b48ec0a88a1716bd4b39b2ac2d... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/59b48ec0a88a1716bd4b39b2ac2d...
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. |