Summary: | AX: Refactor most AccessibilityRenderObject code into AccessibilityNodeObject | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dominic Mazzoni <dmazzoni> | ||||||||
Component: | Accessibility | Assignee: | Dominic Mazzoni <dmazzoni> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cfleizach, jamesr, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 96547 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Dominic Mazzoni
2012-09-10 14:54:47 PDT
Created attachment 163267 [details]
Patch
Comment on attachment 163267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163267&action=review looks good overall. > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:268 > + return selectElement->multiple() ? ListBoxRole : PopUpButtonRole; this change worries me. why do we need to change to a ListBoxRole when it has been a ListRole? > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:319 > + return false; what's the scenario for this one? it's a node object, but there's no node and there's no render object. would this be the case where a node object has been deleted and this hasn't been cleaned up yet? > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:326 > + case CheckBoxRole: we should probably also have ToggleButton in this switch > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:494 > + return role == MenuRole i think we should do a switch statement here instead. > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:542 > + return static_cast<Element*>(node)->isEnabledFormControl(); use toElement() > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:641 > + if (n && (n->isElementNode() && static_cast<Element*>(n)->isFormControlElement())) use toElement() > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:688 > + return m_ariaRole == ProgressIndicatorRole we should probably do a switch statement here > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:757 > + return ((node->isElementNode() && static_cast<Element*>(node)->isFormControlElement()) use toElement() > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:806 > + return object; this line is too far indented > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:843 > + if (node->hasTagName(inputTag)) { bad indentation > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:893 > + for (Element* element = static_cast<Element*>(node); element; element = element->parentElement()) { use toElement() > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1004 > + return ariaLabeledBy; bad indentation > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1016 > + while (sibling) { seems like this should be a for loop > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1040 > + return axObjectCache()->getOrCreate(menu); when you pass in a nil element to getOrCreate, i believe it just returns 0, so in that sense we don't need to check for NULL here > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1158 > + while (parent) { looks like this should be a for loop > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1187 > + AccessibilityRole role = roleValue(); we should probably only check roleValue() after we determine that node() is not nil > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1192 > + Node* node = this->node(); it looks like this check is identical to the one 3 lines above Comment on attachment 163267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163267&action=review >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:268 >> + return selectElement->multiple() ? ListBoxRole : PopUpButtonRole; > > this change worries me. why do we need to change to a ListBoxRole when it has been a ListRole? This was my mistake in the previous patch. ListBoxRole is correct for a <select multiple>. I didn't catch this before because on Mac, both a dynamic list (like <select multiple) and a static list (like a <ul>) map to NSAccessibilityListRole - so it was still working correctly on Mac, but not elsewhere. >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:319 >> + return false; > > what's the scenario for this one? it's a node object, but there's no node and there's no render object. would this be the case where a node object has been deleted and this hasn't been cleaned up yet? The logic is slightly different: * If it's a node object, and there's no node, then we should just return. This only happens if it's detached. * If it's a render object, then it might legitimately have no node (e.g. a web area, scroll area, or css-generated text) - so we shouldn't even check for a node. I added a comment. >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:326 >> + case CheckBoxRole: > > we should probably also have ToggleButton in this switch Done >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:494 >> + return role == MenuRole > > i think we should do a switch statement here instead. Done. >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:542 >> + return static_cast<Element*>(node)->isEnabledFormControl(); > > use toElement() Changed throughout. >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:688 >> + return m_ariaRole == ProgressIndicatorRole > > we should probably do a switch statement here Done >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:806 >> + return object; > > this line is too far indented Fixed. >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:843 >> + if (node->hasTagName(inputTag)) { > > bad indentation Fixed. >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1004 >> + return ariaLabeledBy; > > bad indentation Fixed >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1016 >> + while (sibling) { > > seems like this should be a for loop Good idea, done. >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1040 >> + return axObjectCache()->getOrCreate(menu); > > when you pass in a nil element to getOrCreate, i believe it just returns 0, so in that sense we don't need to check for NULL here Done. >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1158 >> + while (parent) { > > looks like this should be a for loop Done >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1187 >> + AccessibilityRole role = roleValue(); > > we should probably only check roleValue() after we determine that node() is not nil Got rid of the variable role anyway - wasn't needed. >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1192 >> + Node* node = this->node(); > > it looks like this check is identical to the one 3 lines above Removed Created attachment 163541 [details]
Patch
Created attachment 163629 [details]
Patch
Comment on attachment 163629 [details]
Patch
thanks
Comment on attachment 163629 [details] Patch Clearing flags on attachment: 163629 Committed r128318: <http://trac.webkit.org/changeset/128318> All reviewed patches have been landed. Closing bug. This broke two content_unittests in chrome mac: DumpAccessibilityTreeTest.AccessibilityToggleButton: Testing: /Volumes/data/b/build/slave/Mac10_6_Tests/build/src/content/test/data/accessibility/togglebutton.html ../../content/browser/accessibility/dump_accessibility_tree_browsertest.cc:186: Failure Value of: is_different Actual: true Expected: false * Line Expected - ---- -------- 1 AXWebArea subrole=(null) title='' value='' 2 AXButton subrole=(null) title='Regular button' value='' 3 AXButton subrole=(null) title='' value='' * 4 AXStaticText subrole=(null) title='' value='Toggle button' 5 <-- End-of-file --> Actual ------ AXWebArea subrole=(null) title='' value='' AXButton subrole=(null) title='Regular button' value='' AXButton subrole=(null) title='' value='' DumpAccessibilityTreeTest.AccessibilityCanvas: Testing: /Volumes/data/b/build/slave/Mac10_6_Tests/build/src/content/test/data/accessibility/canvas.html ../../content/browser/accessibility/dump_accessibility_tree_browsertest.cc:186: Failure Value of: is_different Actual: true Expected: false * Line Expected - ---- -------- 1 AXWebArea subrole=(null) title='' value='' 2 AXGroup subrole=(null) title='' value='' 3 AXImage subrole=(null) title='' value='' 4 AXStaticText subrole=(null) title='' value='' 5 AXGroup subrole=(null) title='' value='' * 6 AXLink subrole=(null) title='' value='' 7 AXStaticText subrole=(null) title='' value='' 8 <-- End-of-file --> Actual ------ AXWebArea subrole=(null) title='' value='' AXGroup subrole=(null) title='' value='' AXImage subrole=(null) title='' value='' AXStaticText subrole=(null) title='' value='' AXGroup subrole=(null) title='' value='' AXLink subrole=(null) title='Interactive fallback' value='' AXStaticText subrole=(null) title='' value='' Re-landing this change here: https://bugs.webkit.org/show_bug.cgi?id=96565 |