[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