Last Comment Bug 395699 - relations not working when pointing to a <span>
: relations not working when pointing to a <span>
Status: VERIFIED FIXED
orca:high
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Linux
: -- major (vote)
: ---
Assigned To: Evan Yan
:
Mentors:
Depends on: 381599
Blocks: aria orca fox3access
  Show dependency treegraph
 
Reported: 2007-09-10 13:05 PDT by Scott Haeger
Modified: 2008-04-28 23:36 PDT (History)
3 users (show)
surkov.alexander: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
create accessible when it has a relation (21.58 KB, patch)
2007-12-29 07:13 PST, Evan Yan
no flags Details | Diff | Splinter Review
overload FindNeighbourPointingToNode (14.46 KB, patch)
2008-01-01 19:44 PST, Evan Yan
no flags Details | Diff | Splinter Review
also check aria_activedescendant (14.91 KB, patch)
2008-01-01 20:26 PST, Evan Yan
no flags Details | Diff | Splinter Review
addression Aaron's comment (14.82 KB, patch)
2008-01-15 00:13 PST, Evan Yan
surkov.alexander: review+
mtschrep: approval1.9+
Details | Diff | Splinter Review

Description Scott Haeger 2007-09-10 13:05:25 PDT
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 2007-09-11 06:41:04 PDT
For http://accessibleajax.clcworld.net/simple/controls.htm I am seeing the controllerfor relation but not the controlledby relation.
Comment 2 Aaron Leventhal 2007-09-15 12:55:30 PDT
<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 2007-09-15 14:38:30 PDT
Ok, I have a fix for this in bug 371156.
Comment 4 Aaron Leventhal 2007-09-18 14:32:10 PDT
Fixed by checkin to bug 371156.
Comment 5 Aaron Leventhal 2007-09-24 11:40:50 PDT
Had to back that one out.
Comment 6 Aaron Leventhal 2007-12-06 11:10:20 PST
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 2007-12-29 07:13:42 PST
Created attachment 294835 [details] [diff] [review]
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 2007-12-31 04:43:15 PST
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 2008-01-01 19:44:08 PST
Created attachment 295051 [details] [diff] [review]
overload FindNeighbourPointingToNode
Comment 10 Evan Yan 2008-01-01 20:26:14 PST
Created attachment 295053 [details] [diff] [review]
also check aria_activedescendant
Comment 11 Aaron Leventhal 2008-01-07 11:44:43 PST
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 2008-01-07 18:58:46 PST
(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 2008-01-08 08:58:12 PST
(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 2008-01-08 08:59:18 PST
Comment on attachment 295053 [details] [diff] [review]
also check aria_activedescendant

Re-requesting review.
Comment 15 Aaron Leventhal 2008-01-14 08:31:45 PST
> 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 2008-01-14 19:26:03 PST
(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 2008-01-15 00:13:52 PST
Created attachment 297128 [details] [diff] [review]
addression Aaron's comment
Comment 18 Aaron Leventhal 2008-01-16 14:33:31 PST
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 2008-01-17 22:09:44 PST
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 2008-01-18 18:46:00 PST
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 2008-01-20 00:47:43 PST
Comment on attachment 297128 [details] [diff] [review]
addression Aaron's comment

looks ok, r=me
Comment 22 Evan Yan 2008-01-21 19:18:12 PST
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 2008-04-28 23:36:44 PDT
in-testsuite?

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