[Gtk] Implement LABEL_FOR/LABELLED_BY relationship pair for labels

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

Steps to reproduce:

1. Open the (to-be) attached test case.

2. In Accerciser, in the pane on the left, select the accessible object associated with the text "First Name:".

3. In Accerciser, in the pane on the right, choose the Interface Viewer and expand "Accessible".

Expected results:

1. The accessible would be of ROLE_LABEL.

2. In the "Relations" list, Accerciser would indicate that this accessible is a "Label for" beneath which it would list the accessible entry.

Actual results:

1. The accessible is of ROLE_TEXT.

2. The "Relations" list is empty.

In addition:

The Atk relationship LABEL_FOR has a reciprocal LABELLED_BY relationship (see http://library.gnome.org/devel/atk/unstable/AtkRelation.html#AtkRelationType). It is expected that this would be also be indicated/present for the entry being labelled.

[Joanmarie Diggs]

Created attachment 29968 [details]
aforementioned test case

[Joanmarie Diggs]

I'm doing a little bug re-arrangement. Don't mind me. :-)

Exposing labels as ATK_ROLE_LABEL is now part of bug 25901 as that's really no different from the role issue around divs.

This bug is now first and foremost about implementing the accessible relationship set for labels (and in general). In addition, we need to be sure that the thing being labelled gets its accessible name from the thing doing the labelling.

BTW, I'll take this bug. (Doesn't look like I can change the Assigned To.)

[Joanmarie Diggs]

Created attachment 41386 [details]
patch to implement the relationship pair - take 1

For some reason I'm not having any luck getting the accessible names to take (which should be the easier of the two parts). Go figure.... Anyhoo, because the two parts are discrete, I figured I'd submit this relationship pair implementation.

General/culture question: Should I hold off on asking for review until I've got all the parts ready to go?

[Joanmarie Diggs]

Ugh. It seems that controls (at least those of ROLE_ENTRY) base their accessible name on their contents (or lack thereof). My doing atk_object_set_name() on them when I set LABELLED_BY is having no impact. :-(

Therefore, requesting review on the relationship pair patch, which needs doing anyway. Advice on how best (or how at all) I can approach changing the accessible name on these creatures would be awesome. Thanks!

[Joanmarie Diggs]

Xan: Ping. :-)

Since you reviewed the Table Interface patch which I intentionally didn't ask for a review on, I figured you must have missed this one. :-)

[Xan Lopez]

(In reply to comment #5)
> Xan: Ping. :-)
> 
> Since you reviewed the Table Interface patch which I intentionally didn't ask
> for a review on, I figured you must have missed this one. :-)

I'll check this one tomorrow.

[Holger Freyther]

Technically it looks fine. The name of the new methods are a bit misleading. When reading them one would expect to see a parameter. Can you come up with a better name?

[Joanmarie Diggs]

(In reply to comment #7)
> Technically it looks fine. The name of the new methods are a bit misleading.
> When reading them one would expect to see a parameter. Can you come up with a
> better name?

Thanks for the review! 

Having slept and had some coffee, I do believe I can. :-)

Current:   labeledBy()
Proposed:  getLabelForControl()

Current:   labelFor()
Proposed:  getControlForLabel()

Assuming those are acceptable, and Xan doesn't find any other issues, I'll work up a new patch later today. (Doing my DayJob atm.)

[Holger Freyther]

(In reply to comment #8)
> (In reply to comment #7)
> > Technically it looks fine. The name of the new methods are a bit misleading.
> > When reading them one would expect to see a parameter. Can you come up with a
> > better name?
> 
> Thanks for the review! 
> 
> Having slept and had some coffee, I do believe I can. :-)
> 
> Current:   labeledBy()
> Proposed:  getLabelForControl()
> 
> Current:   labelFor()
> Proposed:  getControlForLabel()

Great, normally we omit the "get" part in methods.

[Joanmarie Diggs]

Created attachment 41596 [details]
patch to implement the relationship pair - take 2

> > Current:   labeledBy()
> > Proposed:  getLabelForControl()
> > 
> > Current:   labelFor()
> > Proposed:  getControlForLabel()
> 
> Great, normally we omit the "get" part in methods.

<slap type="forehead">Style Guidelines</slap> Thanks for the reminder. labelForControl and controlForLabel it is. :-)

[Xan Lopez]

Comment on attachment 41596 [details]
patch to implement the relationship pair - take 2

> 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.

[Xan Lopez]

OK, so, my comment is wrong and silly. The code joannie added originally to AccessibilityRenderObject.cpp can't be moved elsewhere, since it's using a static function in that file.

After talking with her on IRC she says she thinks those two functions are pretty basic for any a11y implementation, so let's try to go with her plan and have those added.

CCing [e-mail], which was in the past our a11y contact: can you look at the last patch in the attachment list and see if you are OK with the two functions added to AccessibilityRenderObject?

[chris fleizach]

> CCing [e-mail], which was in the past our a11y contact: can you look
> at the last patch in the attachment list and see if you are OK with the two
> functions added to AccessibilityRenderObject?

So there's already a 
correspondingControlForLabelElement

method in AXRenderObject.cpp. I don't think there's a counterpart going the other direction that's not static however. it seems you should be able to remove/use that method.

Otherwise, i'm OK having a pair to find label/control control/label pairs

[Joanmarie Diggs]

Created attachment 41689 [details]
implment relationship pair and set the name

(In reply to comment #13)
> > CCing [e-mail], which was in the past our a11y contact: can you look
> > at the last patch in the attachment list and see if you are OK with the two
> > functions added to AccessibilityRenderObject?
> 
> So there's already a 
> correspondingControlForLabelElement

Yikes. I totally missed that somehow. My apologies.

This patch uses it.
 
> Otherwise, i'm OK having a pair to find label/control control/label pairs

Awesome. Thanks. labelForCorrespondingControlElement added.

This also solves the issue of setting the name, which I mentioned having problems with.

Xan, please review. Thanks!

[Joanmarie Diggs]

Created attachment 41690 [details]
implment relationship pair and set the name - corrected

* Xan recommended correspondingLabelForControlElement instead. Done.
* We both caught a rogue Tab char and left-over include from debugging. Removed.

[Xan Lopez]

Comment on attachment 41690 [details]
implment relationship pair and set the name - corrected

Looks good to me!

[WebKit Commit Bot]

Comment on attachment 41690 [details]
implment relationship pair and set the name - corrected

Clearing flags on attachment: 41690

Committed r49958: <http://trac.webkit.org/changeset/49958>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.