Bug 96323 – AX: Refactor most AccessibilityRenderObject code into AccessibilityNodeObject

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Dominic Mazzoni]

Whenever possible, methods in AccessibilityRenderObject should be implemented in AccessibilityNodeObject instead, so they work for elements in canvas fallback content.

[Dominic Mazzoni]

Created attachment 163267 [details]
Patch

[chris fleizach]

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

[Dominic Mazzoni]

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

[Dominic Mazzoni]

Created attachment 163541 [details]
Patch

[Dominic Mazzoni]

Created attachment 163629 [details]
Patch

[chris fleizach]

Comment on attachment 163629 [details]
Patch

thanks

[WebKit Review Bot]

Comment on attachment 163629 [details]
Patch

Clearing flags on attachment: 163629

Committed r128318: <http://trac.webkit.org/changeset/128318>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

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=''

[Dominic Mazzoni]

Re-landing this change here: https://bugs.webkit.org/show_bug.cgi?id=96565