[webkit-reviews] review granted: [Bug 42055] AX: aria-checked not recognized on image map radio buttons : [Attachment 61336] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jul 13 09:50:30 PDT 2010


Darin Adler <darin at apple.com> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 42055: AX: aria-checked not recognized on image map radio buttons
https://bugs.webkit.org/show_bug.cgi?id=42055

Attachment 61336: Patch
https://bugs.webkit.org/attachment.cgi?id=61336&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +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.


More information about the webkit-reviews mailing list