388185 - LABELLED_BY, LABEL_FOR relations should be set for labels in panels

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

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)

Comment 1

[Ginn Chen]

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?

Comment 2

[Aaron Leventhal]

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

Comment 3

[alexander :surkov (:asurkov)]

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.

Comment 4

[tiger.zhang]

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     }

Comment 5

[Ginn Chen]

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

Comment 6

[tiger.zhang]

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

Comment 7

[tiger.zhang]

Attachment: patch!

Comment 8

[tiger.zhang]

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

Comment 9

[alexander :surkov (:asurkov)]

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.

Comment 10

[Ginn Chen]

surkov, I agree.

Comment 11

[Evan Yan]

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

Comment 12

[tiger.zhang]

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?

Comment 13

[alexander :surkov (:asurkov)]

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

Comment 14

[Aaron Leventhal]

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

Comment 15

[Ginn Chen]

Taking.

Comment 16

[Aaron Leventhal]

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

Comment 17

[Ginn Chen]

Attachment: patch

Comment 18

[Aaron Leventhal]

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.

Comment 19

[Ginn Chen]

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>

Comment 20

[Aaron Leventhal]

Ginn, the description text is description_for the groupbox.

Comment 21

[Damon Sicore (:damons)]

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 22

[Aaron Leventhal]

Comment on attachment 281802 [details] [diff] [review]
patch

Technicallyt we need an r= for removal of code from toolkit.

Comment 23

[Ginn Chen]

(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 24

[Asaf Romano (gone)]

Comment on attachment 281802 [details] [diff] [review]
patch

mpa=mano for the widget change.

Comment 25

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381