Last Comment Bug 421242 - allow relations in anonymous content for binding parent
: allow relations in anonymous content for binding parent
Status: RESOLVED FIXED
: calendar-integration
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: namea11y rela11y 661293
  Show dependency treegraph
 
Reported: 2008-03-05 23:37 PST by alexander :surkov
Modified: 2012-03-14 14:45 PDT (History)
6 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (5.90 KB, patch)
2008-03-05 23:37 PST, alexander :surkov
no flags Details | Diff | Review
patch (31.12 KB, patch)
2009-06-28 23:26 PDT, alexander :surkov
bugs: review-
Details | Diff | Review
patch (11.76 KB, patch)
2012-02-20 07:33 PST, alexander :surkov
tbsaunde+mozbugs: review+
bugs: feedback+
Details | Diff | Review

Description alexander :surkov 2008-03-05 23:37:26 PST
Created attachment 307656 [details] [diff] [review]
wip

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.
Comment 1 Philipp Kewisch [:Fallen] 2009-06-24 00:20:11 PDT
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.
Comment 2 alexander :surkov 2009-06-28 23:26:06 PDT
Created attachment 385694 [details] [diff] [review]
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?
Comment 3 David Bolter [:davidb] 2009-06-29 09:31:37 PDT
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)
Comment 4 David Bolter [:davidb] 2009-06-29 09:34:17 PDT
(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 5 David Bolter [:davidb] 2009-07-02 08:50:58 PDT
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)
Comment 6 alexander :surkov 2009-07-02 19:45:14 PDT
(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
Comment 7 alexander :surkov 2009-07-02 20:18:02 PDT
Olli, your opinion on the approach, please?
Comment 8 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-07-03 00:47:06 PDT
(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.
Comment 9 alexander :surkov 2009-07-03 00:54:41 PDT
(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.
Comment 10 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-07-06 06:31:48 PDT
(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.
Comment 11 alexander :surkov 2009-07-07 19:35:50 PDT
(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 12 David Bolter [:davidb] 2009-07-17 09:48:41 PDT
Comment on attachment 385694 [details] [diff] [review]
patch

Sounds OK. I'd like to see the new patch please.
Comment 13 alexander :surkov 2009-07-17 19:53:18 PDT
(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?
Comment 14 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2009-07-31 14:40:28 PDT
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 15 Marco Zehe (:MarcoZ) 2012-01-22 22:28:59 PST
Comment on attachment 385694 [details] [diff] [review]
patch

Cancelling review request since this never got anywhere after the r-.
Comment 16 alexander :surkov 2012-02-19 22:22:42 PST
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 17 alexander :surkov 2012-02-20 07:33:10 PST
Created attachment 598871 [details] [diff] [review]
patch
Comment 18 Trevor Saunders (:tbsaunde) 2012-02-27 21:11:33 PST
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.
Comment 19 alexander :surkov 2012-03-04 23:56:05 PST
Olli, ping. Could you please take a look at usage of XBL-related method?
Comment 20 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-03-12 15:48:52 PDT
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
Comment 21 alexander :surkov 2012-03-13 04:38:49 PDT
(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
Comment 23 Ed Morley [:emorley] 2012-03-14 14:40:36 PDT
https://hg.mozilla.org/mozilla-central/rev/6ee509ada03b

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