Closed Bug 362365 Opened 18 years ago Closed 17 years ago

Use anonid for relations/labels within world of anonymous content

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(4 files, 8 obsolete files)

A number of mechanisms in mozilla/accessible use the ID attribute to find elements being pointed to. However, when ARIA relations or <label control="[id]"> is used from within XBL, the anonid attribute should be used instead.
(In reply to comment #0)
> A number of mechanisms in mozilla/accessible use the ID attribute to find
> elements being pointed to. However, when ARIA relations or <label
> control="[id]"> is used from within XBL, the anonid attribute should be used
> instead.
> 

I'm totally agree. Few days ago I filed analogue bug for xforms (bug 361501). We need common mechanism like getElementById().
I've tested this with non-anonymous content, but I haven't tried any anonid cases yet. Surkov, could you try that out for me? You mentioned you had a case where you need this.
Attachment #247072 - Flags: review?(surkov.alexander)
Blocks: 361501
Code that searches label for xul elements is trickly. I guess xul has two rules:
1) get element with @control attribute value of which equals value of @id attribute of given node.
2) search label element in direct children if given node has anonymous content.

What is all magic of GetXULLabelContent/GetContentPointingTo is for?
> 1) get element with @control attribute value of which equals value of @id
> attribute of given node.
Or @anonid.

> 2) search label element in direct children if given node has anonymous content.

Also actually
0) If ::labelledby property exists, use the element pointed to by that.
The labelledby property is handled by GetHTMLName, GetXULName and GetAccessibleRelated.

> What is all magic of GetXULLabelContent/GetContentPointingTo is for?
GetXULLabelContent first tries to do #2, it looks for label element in the direct children by calling GetContentPointingTo with aForAttr == nsnsnull

If that fails it gets an appropriate node which holds an ID for the current element. That could be the binding parent if this is an anonymous element. Otherwise it's the current element itself. 

Then we have an id or anonid, and we look for something pointing to that. Rather than search the entire tree, we assume that the label is usually close in proximity. So first we look at the parent's children. If that fails we look at the grandparent's descendents etc. We also optimize so that we never check the same subtree twice, but passing in the previously area checked via aExcludeContent to GetContentPointintTo().

GetContentPointingTo() is simply recursive and is looking for an element with the appropriate attribute pointing to aId. It's abstracted so that it can be reused to get the inverse relations LABEL_FOR/LABELLED_BY/DESCRIPTION_FOR/DESCRIBED_BY in GetAccessibleRelated(), no matter how the label or description was set.

Let me know where you think I should add appropriate comments.
(In reply to comment #4)

> Then we have an id or anonid, and we look for something pointing to that.
> Rather than search the entire tree, we assume that the label is usually close
> in proximity. So first we look at the parent's children. If that fails we look
> at the grandparent's descendents etc. We also optimize so that we never check
> the same subtree twice, but passing in the previously area checked via
> aExcludeContent to GetContentPointintTo().

If you got value of anonid then I guess you should search label element inside anonymous content only. But you don't make difference between searching for @id attribute and @anonid attribute. Should search procedure for @anonid case be changed?
> If you got value of anonid then I guess you should search label element inside
> anonymous content only. But you don't make difference between searching for @id
> attribute and @anonid attribute. Should search procedure for @anonid case be
> changed?

Can GetParent() go from anon content to non-anon content? I didn't think so.

If we are using an anonid then we are starting from inside anonymous content. Since GetContentPointingTo() just uses GetParent(), and stops when it reaches null, it will never search outside of the anonymous content in the anonid case.

(In reply to comment #6)
> > If you got value of anonid then I guess you should search label element inside
> > anonymous content only. But you don't make difference between searching for @id
> > attribute and @anonid attribute. Should search procedure for @anonid case be
> > changed?
> 
> Can GetParent() go from anon content to non-anon content? I didn't think so.

Yes, it can.
Good catch! Lazy assumption on my part :)
Attachment #247072 - Attachment is obsolete: true
Attachment #247573 - Flags: review?
Attachment #247072 - Flags: review?(surkov.alexander)
Attachment #247573 - Flags: review? → review?(surkov.alexander)
Or is GetBindingParent() the wrong test?

Not sure if a grandchild in the binding has a "binding parent".
Comment on attachment 247573 [details] [diff] [review]
In searching for match, stop going up ancestor chain if content->GetBindingParent() != nsnull

Still not correct.
Attachment #247573 - Attachment is obsolete: true
Attachment #247573 - Flags: review?(surkov.alexander)
Mano suggest we special case the anonid case and do 
getBindingParent(this).getAnonymousElementByAttribute("anonid", "foo")
(In reply to comment #12)
> Mano suggest we special case the anonid case and do 
> getBindingParent(this).getAnonymousElementByAttribute("anonid", "foo")
> 

Let consider the following example:

<topcontrol>
  <xbl:content>
    <xul:label control="anonControlId"/>
    <somecontrol>
      <xbl:content>
        <!-- explicit children, inserted by xbl:children -->
        <xul:button anonid="anonControlId"/>
      </xbl:content>
    </somecontrol>
  </xbl:content>
</topcontrol>

If you would try getBindingParent() for xul:button then you'll get <somecontrol/> element (not <topcontrol/> element, please correct me if I'm wrong) and you will not find xul:label element by anonid. Therefore I guess you should search label through whole bindings chain.
(In reply to comment #13)
> (In reply to comment #12)
> > Mano suggest we special case the anonid case and do 
> > getBindingParent(this).getAnonymousElementByAttribute("anonid", "foo")
> > 
> 
> Let consider the following example:
> 
> <topcontrol>
>   <xbl:content>
>     <xul:label control="anonControlId"/>
>     <somecontrol>
>       <xbl:content>
>         <!-- explicit children, inserted by xbl:children -->
>         <xul:button anonid="anonControlId"/>
>       </xbl:content>
>     </somecontrol>
>   </xbl:content>
> </topcontrol>
> 
> If you would try getBindingParent() for xul:button then you'll get
> <somecontrol/> element (not <topcontrol/> element, please correct me if I'm
> wrong).

Not AFIACT <samecontrol/> is an anonymous element in that context.

Do note that if you actually mean samecontrol is bound to the same binding
as topcontrol then this example is not valid ;)

Er, I read you example wrong, ignore comment 14.

I do not think we should look for anonymous elements inside child bindings, doing so would make anonid a non-unique identifier.
(In reply to comment #15)
> Er, I read you example wrong, ignore comment 14.
> 
> I do not think we should look for anonymous elements inside child bindings,
> doing so would make anonid a non-unique identifier.
> 

Yes, non-unique identifier is bad thing. That's reason why I don't like to search through whole bindings chain. But in my example if <somecontrol/> is not bound element then getBindingParent() will work but if <somecontrol/> is bound element then it will not. But in any way <xul:button/> is explicit child of <somecontrol/> element. There's something bogus in that. Can we handle explicit nodes of <somecontrol/> to make accessibility code working for both cases?
Assignee: aaronleventhal → surkov.alexander
nsIContent* nsAccessible::GetXULLabelContent(nsIContent *aForNode, nsIAtom *aLabelType)
{
  nsAutoString controlID;
  nsIContent *labelContent = GetContentPointingTo(&controlID, aForNode, nsnull, nsnull,
                                                  kNameSpaceID_None, aLabelType);

I wonder does XUL assume usecases for this? I mean can xul:label or xul:description be children of element and be used to for accessibility properties?

  // If we're in anonymous content, determine whether we should use
  // the binding parent based on where the id for this control is
  aForNode->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::id, controlID);
  if (controlID.IsEmpty()) {
    // If no control ID and we're anonymous content
    // get ID from parent that inserted us.
    aForNode = aForNode->GetBindingParent();
    if (aForNode) {
      aForNode->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::id, controlID);

I broke my head but I can't see any usecase for this.
I don't understand the size of your questions. Are you asking why we're doing the anonymous check for XUL at all?

Well, unfortunately I don't remember what XUL situation brought this up anymore. There was a specific bug, IIRC. But in any case it would be easy to develop a XUL widget that used a <label control="[id]"> in the <content> section.

Mark, do you recall what bug made us file this?
I needed this for the findbar a while ago (I actually still do), so the textbox (annoymous node) would have the find label (annoymous node too) as its control.
(In reply to comment #18)
> I don't understand the size of your questions. Are you asking why we're doing
> the anonymous check for XUL at all?

Not exactly I guess. I'll show my comments by examples below.

(In reply to comment #17)
> nsIContent* nsAccessible::GetXULLabelContent(nsIContent *aForNode, nsIAtom
> *aLabelType)
> {
>   nsAutoString controlID;
>   nsIContent *labelContent = GetContentPointingTo(&controlID, aForNode, nsnull,
> nsnull,
>                                                   kNameSpaceID_None,
> aLabelType);
> 
> I wonder does XUL assume usecases for this? I mean can xul:label or
> xul:description be children of element and be used to for accessibility
> properties?

This logic has been added in bug 198118. If I get right then it allows to handle the following cases:

<xul:someelement>
  <xul:label value="It's label"/>
</xul:someelement>

or 

<xul:someelement>
  <xul:description value="It's description"/>
</xul:someelement>

Here xul:someelement should have nsIAccessible::name="it's label" and nsIAccessible::description="it's description". Is it used somewhere and is it vaid testcases for XUL?

>   // If we're in anonymous content, determine whether we should use
>   // the binding parent based on where the id for this control is
>   aForNode->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::id, controlID);
>   if (controlID.IsEmpty()) {
>     // If no control ID and we're anonymous content
>     // get ID from parent that inserted us.
>     aForNode = aForNode->GetBindingParent();
>     if (aForNode) {
>       aForNode->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::id,
> controlID);
> 
> I broke my head but I can't see any usecase for this.
> 

<xul:label control="mycontrol" value="it's label"/>
<xul:myelement id="mycontrol">
  <xbl:content>
    <xul:toolbarbutton/>
  </xbl:content>
</xul:myelement>

Here xul:toolbarbutton should have nsIAccessible::name="it's label".
Status: NEW → ASSIGNED
(In reply to comment #19)
> I needed this for the findbar a while ago (I actually still do), so the textbox
> (annoymous node) would have the find label (annoymous node too) as its control.
> 

Asaf, can you put short example for your case?
For the first question, I think we might be able to remove that code.

It might be for thing like:
<checkbox label="check me"/>
where the <label> element because an anonymous child.

However, I think we actually don't want to try to associate a separate node for that anyway, since it won't have it's own accessible object. So we should file a separate bug and remove that code.
For the second question, it looks like trikly thing. For example, if xul:myelement from example above will be accessible and it will contain few accessible toolbarbutton then all accessible objects will have one and the same name. It doesn't look right.

Does fx/toolkit has (or assume to have) usecases for this?
Oh, I see what you mean for question #2.

I think the use case might be for the address bar -- the autocomplete has a XUL textbox which uses an HTML input. Do we need it for that?
(In reply to comment #24)
> Oh, I see what you mean for question #2.
> 
> I think the use case might be for the address bar -- the autocomplete has a XUL
> textbox which uses an HTML input. Do we need it for that?
> 

I guess no because HTML input is not accessible and autocomplete textbox is not contained by XBL widget.
Okay, I think you can do away with the code that makes #2 happen.
Attached patch patch (obsolete) — Splinter Review
I didn't test it yet
Blocks: xula11y
No longer blocks: keya11y
Attached patch patch2 (obsolete) — Splinter Review
Attachment #253498 - Attachment is obsolete: true
Attachment #253626 - Flags: review?(aaronleventhal)
Attached file test.xml
Attached file test.css
Attached file test.xul
The actual testcase. All texboxes in testcase should have name.
I hope you don't mind Alexander, but I think the new methods names are much better now:
1. GetRelatedContent() -> FindAncestorPointingToNode()
2. GetInverseRelatedNode() -> FindAncestorPointingToThis()
3. GetContentPointingTo() -> FindDescendantPointingToID()

One problem: Tools -> Options -> Startup -> Home Page textbox isn't seeing it's label
Surkov, back over to you to figure out why that Home Page textbox label is no longer hooked up.

It regressed before this patch. I suspect bug 367145 may be involved.
(In reply to comment #32)
> Created an attachment (id=253775) [details]
> Alexander's changes plus some extra comments and a few renamed methods. Also
> remove unneeded declaration for GetXULLabelContent()
> 
> I hope you don't mind Alexander, but I think the new methods names are much
> better now:
> 1. GetRelatedContent() -> FindAncestorPointingToNode()
> 2. GetInverseRelatedNode() -> FindAncestorPointingToThis()
> 3. GetContentPointingTo() -> FindDescendantPointingToID()

Looks ok, but I'm not sure about "ancestor" because the method searched not only inside parent nodes, probably neighborhood or something like this is more appropriate? Also, I'd like if neil looks at the patch to check XBL-related issue. Do patch requires any r since we are both its authors and we are agree with the approach?

> One problem: Tools -> Options -> Startup -> Home Page textbox isn't seeing it's
> label
> 

I'll look at the problem.
Attachment #253626 - Flags: review?(aaronleventhal)
Hi Surkov, you're right about the name ancestor. Maybe you can think of a better name.

We don't need another r=, just because we both worked on it. That's essentially what the r= process is, but instead of just comments I think it's okay to resubmit with explained changes.
(In reply to comment #35)
> Hi Surkov, you're right about the name ancestor. Maybe you can think of a
> better name.

Probably, FindNeighbourPointingToXXX()? though I'm not sure about how it is english.
Blocks: 346895
Sounds good to me.
Attached patch patch3 (obsolete) — Splinter Review
Attachment #253626 - Attachment is obsolete: true
Attachment #253775 - Attachment is obsolete: true
Attachment #255790 - Flags: superreview?(neil)
Comment on attachment 255790 [details] [diff] [review]
patch3

sorry, wrong patch, I'll put another one.
Attachment #255790 - Flags: superreview?(neil)
Attached patch patch4 (obsolete) — Splinter Review
Attachment #255790 - Attachment is obsolete: true
Attachment #255893 - Flags: superreview?(jst)
Attached patch patch5 (obsolete) — Splinter Review
Attachment #255893 - Attachment is obsolete: true
Attachment #256034 - Flags: superreview?(jst)
Attachment #255893 - Flags: superreview?(jst)
Attachment #256034 - Attachment is patch: true
Attachment #256034 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 256034 [details] [diff] [review]
patch5

sr=jst
Attachment #256034 - Flags: superreview?(jst) → superreview+
do I need additional r+ (for example, from content guy) for checking in?
Comment on attachment 256034 [details] [diff] [review]
patch5

Yes, in fact Sicking was mentioning something about GetXBLChildNodesFor() and the other day on IRC. He asked how some possible changes might affect accessibility, but I never heard more.
Attachment #256034 - Flags: review?(jonas)
Attachment #256034 - Flags: review?(jonas) → review?(Olli.Pettay)
Comment on attachment 256034 [details] [diff] [review]
patch5


>   /**
>+   * See GetAnonymousNodesFor on nsBindingManager
>+   */
>+  virtual nsresult GetAnonymousNodesFor(nsIContent* aContent,
>+                                        nsIDOMNodeList** aResult) = 0;
>+
>+  /**

Is this really needed? Why not just use nsIDOMDocumentXBL::GetAnonymousNodes and
keep all the changes in accessibility/ ?
(In reply to comment #45)
> and
> keep all the changes in accessibility/ ? 
> 
er, accessible/ ofc. :)

Comment on attachment 256034 [details] [diff] [review]
patch5

So r- unless someone tells me why nsIDocument::GetAnonymousNodesFor is really needed.
Attachment #256034 - Flags: review?(Olli.Pettay) → review-
Attached patch patch6Splinter Review
I think I have not strong reason to introduce getAnonymousChildFor() for nsIDocument.
Attachment #256034 - Attachment is obsolete: true
Attachment #258814 - Flags: review?(Olli.Pettay)
Olli, now there is no reason for your review :), but if you will look at part that deals with XBL then it would be great.
Attachment #258814 - Flags: review?(Olli.Pettay) → review+
checked in
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 380458
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: