Bug 25530

Summary: [Gtk] Implement LABEL_FOR/LABELLED_BY relationship pair for labels
Product: WebKit Reporter: Joanmarie Diggs <jdiggs>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apinheiro, cfleizach, commit-queue, walker.willie, xan.lopez, zecke
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 25531    
Attachments:
Description Flags
aforementioned test case
none
patch to implement the relationship pair - take 1
none
patch to implement the relationship pair - take 2
xan.lopez: review-
implment relationship pair and set the name
none
implment relationship pair and set the name - corrected none

Description Joanmarie Diggs 2009-05-03 16:11:45 PDT
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.
Comment 1 Joanmarie Diggs 2009-05-03 16:12:18 PDT
Created attachment 29968 [details]
aforementioned test case
Comment 2 Joanmarie Diggs 2009-10-17 21:31:44 PDT
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.)
Comment 3 Joanmarie Diggs 2009-10-18 14:11:48 PDT
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?
Comment 4 Joanmarie Diggs 2009-10-19 07:20:17 PDT
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!
Comment 5 Joanmarie Diggs 2009-10-20 13:55:34 PDT
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. :-)
Comment 6 Xan Lopez 2009-10-20 14:08:19 PDT
(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.
Comment 7 Holger Freyther 2009-10-21 06:27:16 PDT
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?
Comment 8 Joanmarie Diggs 2009-10-21 06:53:23 PDT
(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.)
Comment 9 Holger Freyther 2009-10-21 07:03:04 PDT
(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.
Comment 10 Joanmarie Diggs 2009-10-21 12:02:03 PDT
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. :-)
Comment 11 Xan Lopez 2009-10-21 14:02:39 PDT
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.
Comment 12 Xan Lopez 2009-10-21 15:48:49 PDT
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 cfleizach@apple.com, 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?
Comment 13 chris fleizach 2009-10-21 16:24:50 PDT
> CCing cfleizach@apple.com, 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
Comment 14 Joanmarie Diggs 2009-10-22 15:18:31 PDT
Created attachment 41689 [details]
implment relationship pair and set the name

(In reply to comment #13)
> > CCing cfleizach@apple.com, 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!
Comment 15 Joanmarie Diggs 2009-10-22 15:52:28 PDT
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.
Comment 16 Xan Lopez 2009-10-22 15:55:22 PDT
Comment on attachment 41690 [details]
implment relationship pair and set the name - corrected

Looks good to me!
Comment 17 WebKit Commit Bot 2009-10-22 16:08:03 PDT
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>
Comment 18 WebKit Commit Bot 2009-10-22 16:08:08 PDT
All reviewed patches have been landed.  Closing bug.