395699 - relations not working when pointing to a <span>

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Scott Haeger]

Live region areas are not providing any relations.  I would expect to see the following relations on these test cases.

Test cases:
labelledby: http://accessibleajax.clcworld.net/simple/labelledby.htm
describedby: http://accessibleajax.clcworld.net/simple/describedby.htm

I also remember seeing labelledby relations for Real v Nicks, but there is no aria markup.  Is the XHTML attribute 'header' responsible for setting the relation?

Referring bug is #392061

Comment 1

[Scott Haeger]

For http://accessibleajax.clcworld.net/simple/controls.htm I am seeing the controllerfor relation but not the controlledby relation.

Comment 2

[Aaron Leventhal]

<span>s don't have accessible objects, so there is nothing to point to. 

Workaround:
Use a role on the <span>, or use a tag that always gets an accessible, e.g. <div>

Long term solution:
Fix bug 381599

We nee to have Charles Chen fix his examples using the workarounds for now.

Comment 3

[Aaron Leventhal]

Ok, I have a fix for this in bug 371156.

Comment 4

[Aaron Leventhal]

Fixed by checkin to bug 371156.

Comment 5

[Aaron Leventhal]

Had to back that one out.

Comment 6

[Aaron Leventhal]

Instead of exposing *every* object with an ID, we should fix this by making sure we expose an object with an ID only if it has some relation pointing to it.

Comment 7

[Evan Yan]

Attachment: create accessible when it has a relation

Also made nsAccUtils::FindNeighbourPointingToNode() methods accept multiple relation attributes, so that we can improve performance.

Comment 8

[alexander :surkov (:asurkov)]

Evan, what do you think about to have two methods FindNeighbourPointingToNode, one of them with existing syntax, another one should take in array and its length.

Like:

FindNeighbourPointingToNode(nsIAtom *aAtom)
{
  FindNeighbourPointingToNode(&aAtom, 1)
}

Comment 9

[Evan Yan]

Attachment: overload FindNeighbourPointingToNode

Comment 10

[Evan Yan]

Attachment: also check aria_activedescendant

Comment 11

[Aaron Leventhal]

This is potentially a huge performance hit, since for node in the tree we are walking most of the tree several times!

Entire tree * entire tree * n relations    ... And remember each time you GetAttr() it has to go through the attributes for the node one at a time -- that's O(n). 

We should not be doing any work unless the current node has an ID, otherwise it's impossible for it to be pointed to.

The activedescendant check looks good. It will make sure that even if the current item is not the active one it still gets an accessible.

I would not bother with this one:
+                              nsAccessibilityAtoms::control, // for xul
That's only used to associate a label with a XUL form control, and all XUL form controls already have accessibles.

Comment 12

[Evan Yan]

(In reply to comment #11)
> Entire tree * entire tree * n relations
> 
So I overloaded FindNeighbourPointingToNode to handle all relations at one time.

> We should not be doing any work unless the current node has an ID, otherwise
> it's impossible for it to be pointed to.
> 
There is check for id in FindNeighbourPointingToNode.

> I would not bother with this one:
> +                              nsAccessibilityAtoms::control, // for xul
> That's only used to associate a label with a XUL form control, and all XUL form
> controls already have accessibles.
> 
Could a "control" relation be associated with something without accessible?

In your comment #7 of bug 371156, I thought you mean "control" should be checked, did I get you wrong?

Comment 13

[Aaron Leventhal]

(In reply to comment #12)
> (In reply to comment #11)
> Could a "control" relation be associated with something without accessible?
> 
> In your comment #7 of bug 371156, I thought you mean "control" should be
> checked, did I get you wrong?
You're right, I said that, but right now I don't believe we want or need this "control" check for XUL. It's for pointing at form controls which already have accessible objects.

As far as the ID check, you also have the check in HasRelatedContent(), but further down. I suggest just moving that up.
  nsAutoString id;
  if (aContent && nsAccUtils::GetID(aContent, id)) {


	

Comment 14

[Aaron Leventhal]

Comment on attachment 295053 [details] [diff] [review]
also check aria_activedescendant

Re-requesting review.

Comment 15

[Aaron Leventhal]

> As far as the ID check, you also have the check in HasRelatedContent(), but
> further down. I suggest just moving that up.
>   nsAutoString id;
>  if (aContent && nsAccUtils::GetID(aContent, id)) 

I still want to see that check moved to the top of the method. There's no reason to build the array of relations or make any extra calls if there is no ID.

Comment 16

[Evan Yan]

(In reply to comment #15)
> > As far as the ID check, you also have the check in HasRelatedContent(), but
> > further down. I suggest just moving that up.
> >   nsAutoString id;
> >  if (aContent && nsAccUtils::GetID(aContent, id)) 
> 
> I still want to see that check moved to the top of the method. There's no
> reason to build the array of relations or make any extra calls if there is no
> ID.
> 
Yes, I will update it.

Do you need a new patch for review?

Comment 17

[Evan Yan]

Attachment: addression Aaron's comment

Comment 18

[Aaron Leventhal]

Comment on attachment 297128 [details] [diff] [review]
addression Aaron's comment

Thanks for those changes. I want Surkov to review the rest, which I didn't look at.

Comment 19

[alexander :surkov (:asurkov)]

Ok. The patch looks ok but I have a question about HasRelatedContent. THere you check activedescendant property on parent and if is there is then content will be accessible. If it's correct then we will get for example in XUL that every xul:box with id is accessible. How much is this correct?

Comment 20

[Aaron Leventhal]

There would be an accessible for any XUL box with an id, that also has an ancestor that has an activedescendant property.

Basically that's the best we can do. We need at least some generic accessible since the activedescendant attribute could point to that xul:box and thus we would need to fire a focus event for it.

Comment 21

[alexander :surkov (:asurkov)]

Comment on attachment 297128 [details] [diff] [review]
addression Aaron's comment

looks ok, r=me

Comment 22

[Evan Yan]

Checking in nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <--  nsAccessibilityService.cpp
new revision: 1.260; previous revision: 1.259
done
Checking in nsAccessibilityUtils.h;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityUtils.h,v  <--  nsAccessibilityUtils.h
new revision: 1.19; previous revision: 1.18
done
Checking in nsAccessibilityUtils.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityUtils.cpp,v  <--  nsAccessibilityUtils.cpp
new revision: 1.24; previous revision: 1.23
done

Comment 23

[alexander :surkov (:asurkov)]

in-testsuite?