I'll put mochitests later, it should allow to use us construction like: <content aria-labelledby="internal"> <xul:label anonid="internal"/> </content> It helps when binding element is complex.

This could possibly help simplify calendar code a bit. For example, we have items in our month view that look like ==================================================== A = Alarm icon |[T] 09:15 AM New Task [A]|C| T = Task icon ==================================================== C = Category box The xbl is quite complex due to extra features like the gradient and that it should be possible to edit the task title via single-click editing. I don't know how good this is for localizablity, but aside from that this bug will help, since instead of manually filling the aria-label, we can use the anonymous members to fill the aria label.

When we check if attribute placed on bound element is referred to anonymous content then take into account attributes defined on xbl:content only, otherwise consider it as it isn't referred to anonymous content inside of bound element. What do you think?

Comment on attachment 385694 [details] [diff] [review] patch I keep getting interrupted so I'll comment as I go. > nsIContent* > nsCoreUtils::FindNeighbourPointingToNode(nsIContent *aForNode, > nsIAtom **aRelationAttrs, > PRUint32 aAttrNum, > nsIAtom *aTagName, > PRUint32 aAncestorLevelsToSearch) > { >+ nsIDocument* doc = aForNode->GetCurrentDoc(); We assume valid input right? (aForNode) > if (aForNode == binding) { >- // When we reach the binding parent, make sure to check >- // all of its anonymous child subtrees >- nsCOMPtr<nsIDocument> doc = aForNode->GetCurrentDoc(); >+ // When we reach the binding parent, check attributes on XBL content >+ // element because attributes placed on it are referred to anonymous >+ // content I think "referred to" is not quite right here. Do you mean "referring to" or "reffered to by"? (in at least 4 places)

(In reply to comment #2) > Created an attachment (id=385694) [details] > patch > > When we check if attribute placed on bound element is referred to anonymous > content then take into account attributes defined on xbl:content only, > otherwise consider it as it isn't referred to anonymous content inside of bound > element. > > What do you think? Makes sense to me.

Comment on attachment 385694 [details] [diff] [review] patch >+} >+ >+PRBool >+nsCoreUtils::IsContentPointingToID(const nsCString& aIdWithSpaces, >+ for (PRUint32 i = 0; i < aAttrNum; i++) { >+ nsAutoString idList; >+ if (aContent->GetAttr(kNameSpaceID_None, aRelationAttrs[i], idList)) { >+ >+ nsIDocument *document = aContent->GetCurrentDoc(); >+ nsIContent *prototypeContent = document->GetPrototypeContent(aContent); You could move these initializers before the loop right? >+ } else if (aContent->IsInAnonymousSubtree()) { (note to self: left off here)

(In reply to comment #3) > (From update of attachment 385694 [details] [diff] [review]) > >+ nsIDocument* doc = aForNode->GetCurrentDoc(); > > We assume valid input right? (aForNode) Right. We didn't check it before, so I didn't change this now. > >+ // When we reach the binding parent, check attributes on XBL content > >+ // element because attributes placed on it are referred to anonymous > >+ // content > > I think "referred to" is not quite right here. Do you mean "referring to" or > "reffered to by"? (in at least 4 places) I though "referred to", I can't apply "referred to by" here, probably because of lack of my english knowledge. Could you help here please? (In reply to comment #5) > (From update of attachment 385694 [details] [diff] [review]) > >+ nsIDocument *document = aContent->GetCurrentDoc(); > >+ nsIContent *prototypeContent = document->GetPrototypeContent(aContent); > > You could move these initializers before the loop right? sure, I will

Olli, your opinion on the approach, please?

(In reply to comment #2) > Created an attachment (id=385694) [details] > patch > > When we check if attribute placed on bound element is referred to anonymous > content then take into account attributes defined on xbl:content only, > otherwise consider it as it isn't referred to anonymous content inside of bound > element. > > What do you think? What if someone removes and adds the attribute on bound element? And if this approach is taken, it must be documented somewhere that aria attributes on <content> have very special meaning. Would it be possible to add some new (non-standard) attribute? -moz-aria-labelledbyanonymous. I know that is a hack too, but at least it would be used only with XBL1 which is Mozilla specific anyway.

(In reply to comment #8) > What if someone removes and adds the attribute on bound element? It doesn't matter. I check if it's value is different from prototype's one. If different then aria-labelledby doesn't refer to anonymous content of the bound element. The weak place of this approach if someone will define the aria-labelledby on bound element and it shouldn't be referred to anon content of bound element for sure. Here we will fail. > And if this approach is taken, it must be documented somewhere that > aria attributes on <content> have very special meaning. Sure, it must be documented. > Would it be possible to add some new (non-standard) attribute? > -moz-aria-labelledbyanonymous. > I know that is a hack too, but at least it would be used only with XBL1 which > is Mozilla specific anyway. New attribute doesn't have a minus of taken approach. It's not ambiguous. But we should watch for one more attribute changes and etc.

(In reply to comment #9) > > Would it be possible to add some new (non-standard) attribute? > > -moz-aria-labelledbyanonymous. Maybe even in xbl namespace > But > we should watch for one more attribute changes and etc. Or you define that it is not "live". You just check the initial value.

(In reply to comment #10) > (In reply to comment #9) > > > Would it be possible to add some new (non-standard) attribute? > > > -moz-aria-labelledbyanonymous. > Maybe even in xbl namespace > > > But > > we should watch for one more attribute changes and etc. > Or you define that it is not "live". You just check the initial value. David, how does it sound for you? This should cover our needs.

Comment on attachment 385694 [details] [diff] [review] patch Sounds OK. I'd like to see the new patch please.

(In reply to comment #8) > Would it be possible to add some new (non-standard) attribute? > -moz-aria-labelledbyanonymous. > I know that is a hack too, but at least it would be used only with XBL1 which > is Mozilla specific anyway. Olli, should I clone this attribute on the bound element?

Perhaps. That would be at least easier. Then you could always just check the attribute on the element. No need to do anything with XBL implementation.

Comment on attachment 385694 [details] [diff] [review] patch Cancelling review request since this never got anywhere after the r-.

the feature is not restricted to aria-labelledby, i.e. any ARIA relation should work. And it seems people think that these attributes should be working this way (see bug 661293 for example) so introduction of new attribute(s) is not what people expect. On the other hand we should track attribute changes because I foresee that people will change the attribute value dynamically inside the binding. My suggestion is: 1) check elements referred by ARIA relation in explicit content 2) if 1) fails then check anonymous content That us makes confident that if the author overrides ARIA relation then we respect this. If the author runs into unwanted collisions then he needs to address it (change the id), that's unfortunate but anyway the author should test accessibility.

Comment on attachment 598871 [details] [diff] [review] patch >+ // Get elements in DOM tree by ID attribute if this is an explicit content. >+ // In case of bound element check its anonymous subtree. >+ if (!mContent->IsInAnonymousSubtree()) { >+ dom::Element* refElm = mContent->OwnerDoc()->GetElementById(aID); >+ if (refElm || !mContent->OwnerDoc()->BindingManager()->GetBinding(mContent)) >+ return refElm; > } > >- return mDocument->GetElementById(aID); >+ // If content is in anonymous subtree or an element having anonymous subtree >+ // then use "anonid" attribute to get elements in anonymous subtree. >+ nsCOMPtr<nsIDOMElement> refDOMElm; >+ nsCOMPtr<nsIDOMDocumentXBL> xblDocument = >+ do_QueryInterface(mContent->OwnerDoc()); >+ >+ // Check inside the binding the element is contained in. >+ nsIContent* bindingParent = mContent->GetBindingParent(); >+ if (bindingParent) { >+ nsCOMPtr<nsIDOMElement> bindingParentElm = do_QueryInterface(bindingParent); >+ xblDocument->GetAnonymousElementByAttribute(bindingParentElm, >+ NS_LITERAL_STRING("anonid"), >+ aID, >+ getter_AddRefs(refDOMElm)); >+ nsCOMPtr<dom::Element> refElm = do_QueryInterface(refDOMElm); >+ if (refElm) >+ return refElm; >+ } >+ >+ // Check inside the binding of the element. >+ if (mContent->OwnerDoc()->BindingManager()->GetBinding(mContent)) { >+ nsCOMPtr<nsIDOMElement> elm = do_QueryInterface(mContent); >+ xblDocument->GetAnonymousElementByAttribute(elm, >+ NS_LITERAL_STRING("anonid"), >+ aID, >+ getter_AddRefs(refDOMElm)); >+ nsCOMPtr<dom::Element> refElm = do_QueryInterface(refDOMElm); >+ return refElm; >+ } >+ >+ return nsnull; > } > > nsAccessible* > IDRefsIterator::Next() > { > nsIContent* nextElm = NextElem(); > return nextElm ? GetAccService()->GetAccessible(nextElm, nsnull) : nsnull; > } >diff --git a/accessible/src/base/AccIterator.h b/accessible/src/base/AccIterator.h >--- a/accessible/src/base/AccIterator.h >+++ b/accessible/src/base/AccIterator.h >@@ -287,20 +287,17 @@ public: > > private: > IDRefsIterator(); > IDRefsIterator(const IDRefsIterator&); > IDRefsIterator operator = (const IDRefsIterator&); > > nsString mIDs; > nsAString::index_type mCurrIdx; >- >- nsIDocument* mDocument; >- nsCOMPtr<nsIDOMDocumentXBL> mXBLDocument; >- nsCOMPtr<nsIDOMElement> mBindingParent; >+ nsIContent* mContent; > }; you might want to put the index after the pointer incase we want to put other small things in the class for better pakcing on 64 bit platforms, but for short lived things like this it hardly matters.

Olli, ping. Could you please take a look at usage of XBL-related method?

Comment on attachment 598871 [details] [diff] [review] patch >+++ b/accessible/src/base/AccIterator.h >@@ -287,20 +287,17 @@ public: > > private: > IDRefsIterator(); > IDRefsIterator(const IDRefsIterator&); > IDRefsIterator operator = (const IDRefsIterator&); > > nsString mIDs; > nsAString::index_type mCurrIdx; >- >- nsIDocument* mDocument; >- nsCOMPtr<nsIDOMDocumentXBL> mXBLDocument; >- nsCOMPtr<nsIDOMElement> mBindingParent; >+ nsIContent* mContent; I hope something ensures that this does never ever point to a deleted object. I think this is ok. I'm not very familiar with all the a11y APIs

(In reply to Olli Pettay [:smaug] from comment #20) > >+ nsIContent* mContent; > I hope something ensures that this does never ever point to a deleted object. the object is created on stack always, we shouldn't do anything that could destroy nsIContent