Summary: | AX: aria-checked not recognized on image map radio buttons | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bdakin | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
chris fleizach
2010-07-11 23:59:30 PDT
Created attachment 61195 [details]
patch
Created attachment 61196 [details]
patch
Created attachment 61336 [details]
Patch
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. (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 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
|