There are a several dialog boxes that contain panels with labels. These labels should have the LABEL_FOR relationship pointing to the panel. Similarly, the panel should have the LABELLED_BY relationship pointing to the label(s). The ones I have found so far are: 1. New Account wizard (Thunderbird) 2. New Address Book card (Thunderbird) 3. The Import Wizard from the Address Book (Thunderbird) 4. The Page Setup dialog (Thunderbird, Firefox)

Aaron, the general issue here is we set control attr if the parent (groupbox) has an id. See: http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/groupbox.xml#52 But there're a lot of groupbox don't have an id. See: http://lxr.mozilla.org/seamonkey/source/mailnews/base/prefs/resources/content/pref-mailnews.xul Should we tell UI dev, if <groupbox> doesn't have an id, it could not be accessible? Is there a doc for this?

No, it was a dumb implementation from me. We should fix it so that an ID is not needed. I think it would be okay to change nsXULTextAccessible::GetAccessibleRelated() and nsXULGroupboxAccessible::GetAccessibleRelated() so that they automatically associate parent groupboxes with child labels (and descriptions).

Yes, I think it's not good to require ID attribute on groupbox to link groupbox and caption. So either we should search caption inside explicit children of groupbox (and vice versa) or require interface for groupbox to find a caption and vice versa. I like more second approach because it won't make a pain to watch if something is changed in groupbox architecture.

relates to these codes in accessible/src/base/nsAccessible.cpp I tried to associate parent groupbox with child labels, but actually it looks like that the label_for relation was associated with the label caption rather than groupbox panel. Is it a wrong way to do like this below? case nsIAccessibleRelation::RELATION_LABEL_FOR: 2578 { 2579 if (content->Tag() == nsAccessibilityAtoms::label) { 2580 nsIAtom *relatedIDAttr = content->IsNodeOfType(nsINode::eHTML) ? 2581 nsAccessibilityAtoms::_for : nsAccessibilityAtoms::control; 2582 content->GetAttr(kNameSpaceID_None, relatedIDAttr, relatedID); 2583 } 2584 if (relatedID.IsEmpty()) { 2585 const PRUint32 kAncestorLevelsToSearch = 3; 2586 relatedNode = FindNeighbourPointingToThis(nsAccessibilityAtoms::labelledby, 2587 kNameSpaceID_WAIProperties, 2588 kAncestorLevelsToSearch); 2589 if ( !relatedNode ) { 2590 // Only in certain case we need to associate the child labels with parent capiton and parent groupbox automatically 2591 // Firstly, look for the label parent, if it is caption, then look for caption parent --> groupbox 2592 nsIContent *ContentParent = content->GetParent(); 2593 nsIContent *parent = ( ContentParent && ContentParent->Tag() == nsAccessibilityAtoms::caption )? ContentParent->GetParent() : ContentPare nt; 2594 if ( parent && parent->Tag() == nsAccessibilityAtoms::groupbox ) { 2595 relatedNode = do_QueryInterface(content); 2596 } 2597 } 2598 } 2599 break; 2600 }

It seems you want to return "parent" not "content". 2595 relatedNode = do_QueryInterface(content);

sorry! correct line 2595: relatedNode = do_QueryInterface(parent)

sorry! correct line 2595: relatedNode = do_QueryInterface(parent)

I would like to see the code inside groupbox/caption accessible or something like this because we shouldn't burden nsAccessible with information about specific elements.

surkov, I agree.

Currently, nsHTMLCaptionAccessible is only used for caption in html table, we can also use it for caption in group.

It seems that override nsXULTextAccessible::GetAccessibleRelated() is enough. like below: NS_IMETHODIMP nsXULTextAccessible::GetAccessibleRelated(PRUint32 aRelationType, nsIAccessible **aRelated) { NS_ENSURE_ARG_POINTER(aRelated); *aRelated = nsnull; if (!mDOMNode) { return NS_ERROR_FAILURE; } nsresult rv = nsAccessible::GetAccessibleRelated(aRelationType, aRelated); if (NS_FAILED(rv) || *aRelated) { return rv; } if (aRelationType == nsIAccessibleRelation::RELATION_LABEL_FOR) { return GetParent(aRelated); } return NS_OK; } right?

(In reply to comment #12) > It seems that override nsXULTextAccessible::GetAccessibleRelated() is enough. > like below: What testcase do you want to cover? Notice, now xul:caption has no accessible but I guess xul:caption should be a label for groupbox. Right?

Tiger, are you going to submit a patch for this?

Taking.

Ginn, bug 371048 is the same bug but for descriptions. Can you fix it at the same time?

Comment on attachment 281802 [details] [diff] [review] patch r+ but personall I think you don't need the nsIContent check. All you need is to check the parent accessible's role. That means you can get rid of the new atom as well. I realize this could lead to a situation where something has ARIA role="group" and the label gets labelledby, but the group does not have label for. I don't see that is a good enough reason for the extra check.

I think if the groupbox have children of description text, usually the description text is not LABEL_FOR the groupbox. e.g. 194 <groupbox id="securityBox"> 195 <caption id="securityBoxCaption" label="&securityHeader;"/> 196 <description id="general-security-identity" class="header"/> 197 <description id="general-security-privacy" class="header"/> 198 <hbox align="right"> 199 <button id="security-view-more" label="&generalSecurityMore;" 200 accesskey="&generalSecurityMore.accesskey;" 201 oncommand="onClickMore();"/> 202 </hbox> 203 </groupbox>

Ginn, the description text is description_for the groupbox.

Comment on attachment 281802 [details] [diff] [review] patch This contains a chaing in toolkit/content/widgets/groupbox.xml. Shouldn't this have a reviewer from toolkit? Just want to make sure.

Comment on attachment 281802 [details] [diff] [review] patch Technicallyt we need an r= for removal of code from toolkit.

(In reply to comment #20) > Ginn, the description text is description_for the groupbox. > Yes, but we're talking about label_for here. So I need to check for caption.

Comment on attachment 281802 [details] [diff] [review] patch mpa=mano for the widget change.