362365 - Use anonid for relations/labels within world of anonymous content

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

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.

Comment 1

[alexander :surkov (:asurkov)]

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

Comment 2

[Aaron Leventhal]

Attachment: Generalize the getting of an id from an element

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.

Comment 3

[alexander :surkov (:asurkov)]

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?

Comment 4

[Aaron Leventhal]

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

Comment 5

[alexander :surkov (:asurkov)]

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

Comment 6

[Aaron Leventhal]

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

Comment 7

[alexander :surkov (:asurkov)]

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

Comment 8

[Aaron Leventhal]

Good catch! Lazy assumption on my part :)

Comment 9

[Aaron Leventhal]

Attachment: In searching for match, stop going up ancestor chain if content->GetBindingParent() != nsnull

Comment 10

[Aaron Leventhal]

Or is GetBindingParent() the wrong test?

Not sure if a grandchild in the binding has a "binding parent".

Comment 11

[Aaron Leventhal]

Comment on attachment 247573 [details] [diff] [review]
In searching for match, stop going up ancestor chain if content->GetBindingParent() != nsnull

Still not correct.

Comment 12

[Aaron Leventhal]

Mano suggest we special case the anonid case and do 
getBindingParent(this).getAnonymousElementByAttribute("anonid", "foo")

Comment 13

[alexander :surkov (:asurkov)]

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

Comment 14

[Asaf Romano (gone)]

(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 ;)

Comment 15

[Asaf Romano (gone)]

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.

Comment 16

[alexander :surkov (:asurkov)]

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

Comment 17

[alexander :surkov (:asurkov)]

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.

Comment 18

[Aaron Leventhal]

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?

Comment 19

[Asaf Romano (gone)]

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.

Comment 20

[alexander :surkov (:asurkov)]

(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".

Comment 21

[alexander :surkov (:asurkov)]

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

Comment 22

[Aaron Leventhal]

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.

Comment 23

[alexander :surkov (:asurkov)]

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?

Comment 24

[Aaron Leventhal]

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?

Comment 25

[alexander :surkov (:asurkov)]

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

Comment 26

[Aaron Leventhal]

Okay, I think you can do away with the code that makes #2 happen.

Comment 27

[alexander :surkov (:asurkov)]

Attachment: patch

I didn't test it yet

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 29

[alexander :surkov (:asurkov)]

Attachment: test.xml

Comment 30

[alexander :surkov (:asurkov)]

Attachment: test.css

Comment 31

[alexander :surkov (:asurkov)]

Attachment: test.xul

The actual testcase. All texboxes in testcase should have name.

Comment 32

[Aaron Leventhal]

Attachment: 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()

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

Comment 33

[Aaron Leventhal]

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.

Comment 34

[alexander :surkov (:asurkov)]

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

Comment 35

[Aaron Leventhal]

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.

Comment 36

[alexander :surkov (:asurkov)]

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

Comment 37

[Aaron Leventhal]

Sounds good to me.

Comment 38

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 39

[alexander :surkov (:asurkov)]

Comment on attachment 255790 [details] [diff] [review]
patch3

sorry, wrong patch, I'll put another one.

Comment 40

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 41

[alexander :surkov (:asurkov)]

Attachment: patch5

Comment 42

[Johnny Stenback (:jst)]

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

sr=jst

Comment 43

[alexander :surkov (:asurkov)]

do I need additional r+ (for example, from content guy) for checking in?

Comment 44

[Aaron Leventhal]

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.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #45)
> and
> keep all the changes in accessibility/ ? 
> 
er, accessible/ ofc. :)

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

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

So r- unless someone tells me why nsIDocument::GetAnonymousNodesFor is really needed.

Comment 48

[alexander :surkov (:asurkov)]

Attachment: patch6

I think I have not strong reason to introduce getAnonymousChildFor() for nsIDocument.

Comment 49

[alexander :surkov (:asurkov)]

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.

Comment 50

[alexander :surkov (:asurkov)]

checked in