Bug 25530

Summary:

[Gtk] Implement LABEL_FOR/LABELLED_BY relationship pair for labels

Product:

WebKit

Reporter:

Joanmarie Diggs <jdiggs>

Component:

Accessibility

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apinheiro, cfleizach, commit-queue, walker.willie, xan.lopez, zecke

Priority:

Keywords:

Gtk

Version:

528+ (Nightly build)

Hardware:

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

Joanmarie Diggs

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

Attachments
aforementioned test case (186 bytes, text/html) 2009-05-03 16:12 PDT, Joanmarie Diggs	no flags	Details
patch to implement the relationship pair - take 1 (6.22 KB, patch) 2009-10-18 14:11 PDT, Joanmarie Diggs	no flags	Details Formatted Diff Diff
patch to implement the relationship pair - take 2 (6.20 KB, patch) 2009-10-21 12:02 PDT, Joanmarie Diggs	xan.lopez: review-	Details Formatted Diff Diff
implment relationship pair and set the name (7.33 KB, patch) 2009-10-22 15:18 PDT, Joanmarie Diggs	no flags	Details Formatted Diff Diff
implment relationship pair and set the name - corrected (7.18 KB, patch) 2009-10-22 15:52 PDT, Joanmarie Diggs	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Joanmarie Diggs

Comment 1 2009-05-03 16:12:18 PDT

Created attachment 29968 [details] aforementioned test case

Joanmarie Diggs

Comment 2 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.)

Joanmarie Diggs

Comment 3 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?

Joanmarie Diggs

Comment 4 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!

Joanmarie Diggs

Comment 5 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. :-)

Xan Lopez

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

Holger Freyther

Comment 7 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?

Joanmarie Diggs

Comment 8 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.)

Holger Freyther

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

Joanmarie Diggs

Comment 10 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. :-)

Xan Lopez

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

Xan Lopez

Comment 12 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?

chris fleizach

Comment 13 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

Joanmarie Diggs

Comment 14 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!

Joanmarie Diggs

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

Xan Lopez

Comment 16 2009-10-22 15:55:22 PDT

Comment on attachment 41690 [details] implment relationship pair and set the name - corrected Looks good to me!

WebKit Commit Bot

Comment 17 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>

WebKit Commit Bot

Comment 18 2009-10-22 16:08:08 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.