Bug 42055 – AX: aria-checked not recognized on image map radio buttons

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

aria-checked not recognized on image map radio buttons

[chris fleizach]

Created attachment 61195 [details]
patch

[chris fleizach]

Created attachment 61196 [details]
patch

[chris fleizach]

Created attachment 61336 [details]
Patch

[Darin Adler]

Comment on attachment 61336 [details]
Patch

> +void AccessibilityObject::retrieveIntValue(int& result) const

The code from this function should be in the AccessibilityObject::intValue function; a separate function is not helpful here. AccessibilityRenderObject::intValue could just "return AccessibilityObject::intValue()" at the end and get the same result we get the way it's written in the patch. Both functions return 0 if there is no int value, so there's no need for a "don't touch the int" or a boolean result to indicate that there is no intValue. The more roundabout way that makes use the magic number -1 gives the same end result and is unneeded.

> +    // If this is an ARIA checkbox or radio, check the aria-checked attribute rather than node()->checked()
> +    if (isCheckboxOrRadio()) {
> +        const AtomicString& checked = getAttribute(node(), aria_checkedAttr);
> +        if (equalIgnoringCase(checked, "true"))
> +            result = 1;
> +        else if (equalIgnoringCase(checked, "false"))
> +            result = 0;
> +    }

The way I'd write this is by saying that if it's a "real" checkbox or radio we'll never get here, so this simply has the logic correct for when it's an ARIA one.

    if (isCheckboxOrRadio())
        return equalIgnoringCase(getAttribute(node(), aria_checkedAttr), "true");

Now that the node() virtual function is in the AccessibilityObhject class, I think you should do a cleanup pass of static member functions that take Node* and consider whether you can replace them with member functions that do not have a Node* argument. You will need to check that callers do not have some need to pass a different Node*. This would be good for getAttribute and might also apply to one or more of these:

    anchorElementForNode
    firstAccessibleObjectFromNode
    language
    listMarkerTextForNodeAndPosition
    renderListItemContainerForNode

> -    Node* node() const 
> +    virtual Node* node() const 
>      { 
>          return m_renderer ? m_renderer->node() : 0; 
>      };

There's a stray semicolon here. Also, having this function body in the header doesn't do any good. Typically it's not useful to have virtual functions also inlined.

I’m going to say r=me because this patch is OK as-is, but I think it’s straightforward to improve it given my comments above.

Also not sure why the Win EWS failed.

[chris fleizach]

(In reply to comment #4)
> (From update of attachment 61336 [details])
> > +void AccessibilityObject::retrieveIntValue(int& result) const
> 
> The code from this function should be in the AccessibilityObject::intValue function; a separate function is not helpful here. AccessibilityRenderObject::intValue could just "return AccessibilityObject::intValue()" at the end and get the same result we get the way it's written in the patch. Both functions return 0 if there is no int value, so there's no need for a "don't touch the int" or a boolean result to indicate that there is no intValue. The more roundabout way that makes use the magic number -1 gives the same end result and is unneeded.
> 

Thanks for this good feedback

> Now that the node() virtual function is in the AccessibilityObhject class, I think you should do a cleanup pass of static member functions that take Node* and consider whether you can replace them with member functions that do not have a Node* argument. You will need to check that callers do not have some need to pass a different Node*. This would be good for getAttribute and might also apply to one or more of these:
> 
>     anchorElementForNode
>     firstAccessibleObjectFromNode
>     language
>     listMarkerTextForNodeAndPosition
>     renderListItemContainerForNode
> 

I was able to change a lot of instances of getAttribute(Node*) to the member function that uses node() implicitly, but there were still some cases where the Node* variant was needed, so I have two variants (one with node, one without)

In the list you mentioned, I was only able to change language(). The other ones are pretty tightly tied to having arbitrary nodes passed in

[chris fleizach]

Created attachment 61479 [details]
patch to test against other bots

adding patch after changes from darin's comments so that it can run against the bots

[chris fleizach]

http://trac.webkit.org/changeset/63358