[webkit-reviews] review denied: [Bug 25530] [Gtk] Implement LABEL_FOR/LABELLED_BY relationship pair for labels : [Attachment 41596] patch to implement the relationship pair - take 2

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has denied Joanmarie Diggs
<joanmarie.diggs at gmail.com>'s request for review:
Bug 25530: [Gtk] Implement LABEL_FOR/LABELLED_BY relationship pair for labels
https://bugs.webkit.org/show_bug.cgi?id=25530

Attachment 41596: patch to implement the relationship pair - take 2
https://bugs.webkit.org/attachment.cgi?id=41596&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
> diff --git a/WebCore/accessibility/AccessibilityRenderObject.cpp
b/WebCore/accessibility/AccessibilityRenderObject.cpp
> index 834e931..9c1122b 100644
> --- a/WebCore/accessibility/AccessibilityRenderObject.cpp
> +++ b/WebCore/accessibility/AccessibilityRenderObject.cpp
> @@ -930,6 +930,35 @@ static HTMLLabelElement* labelForElement(Element*
element)
>      
>      return 0;
>  }
> +
> +AccessibilityObject* AccessibilityRenderObject::labelForControl() const
> +{
> +    if (!m_renderer)
> +	   return 0;
> +
> +    Node* node = m_renderer->node();
> +    if (node && node->isHTMLElement()) {
> +	   HTMLLabelElement* label =
labelForElement(static_cast<Element*>(node));
> +	   if (label)
> +	       return firstAccessibleObjectFromNode(static_cast<Node*>(label));

> +    }
> +
> +    return 0;
> +}
> +
> +AccessibilityObject* AccessibilityRenderObject::controlForLabel() const
> +{
> +    if (!m_renderer)
> +	   return 0;
> +
> +    Node* node = m_renderer->node();
> +    if (node && node->hasTagName(labelTag)) {
> +	   HTMLLabelElement* label = static_cast<HTMLLabelElement*>(node);
> +	   return
firstAccessibleObjectFromNode(static_cast<Node*>(label->correspondingControl())
);
> +    }
> +
> +    return 0;
> +}

I think I prefer to have this as private functions in our code, since they seem
somewhat tied to ATK functionality. We can later ask the other ports using a11y
if they think this would be useful for them, and we can put it in the
crossplatform code.

  
> +static void setAtkRelationSetFromCoreObject(AccessibilityObject* coreObject,
AtkRelationSet* relationSet)
> +{
> +    AccessibilityRenderObject* accObject =
static_cast<AccessibilityRenderObject*>(coreObject);
> +    if (accObject->isControl()) {
> +	   AccessibilityObject* label = accObject->labelForControl();
> +	   if (label) {
> +	       AtkObject* atkLabel = label->wrapper();
> +	       atk_relation_set_add_relation_by_type(relationSet,
ATK_RELATION_LABELLED_BY, atkLabel);
> +	   }
> +    }
> +    else {

The 'else {' goes in the same line than the '}'.

Looks good to me otherwise, marking r- while we change those two details.