allow relations in anonymous content for binding parent

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 2 bugs, {calendar-integration})

unspecified
mozilla14
calendar-integration
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Updated

8 years ago
Blocks: 475297
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.
Keywords: calendar-integration
(Assignee)

Comment 2

8 years ago
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?
Attachment #307656 - Attachment is obsolete: true
Attachment #385694 - Flags: review?(marco.zehe)
Attachment #385694 - Flags: review?(bolterbugz)
Attachment #385694 - Flags: review?(Olli.Pettay)
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)
(Assignee)

Comment 6

8 years ago
(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
(Assignee)

Comment 7

8 years ago
Olli, your opinion on the approach, please?

Comment 8

8 years ago
(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.
(Assignee)

Comment 9

8 years ago
(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.
(Assignee)

Comment 11

8 years ago
(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.

Updated

8 years ago
Attachment #385694 - Flags: review?(bolterbugz)
Comment on attachment 385694 [details] [diff] [review]
patch

Sounds OK. I'd like to see the new patch please.
(Assignee)

Comment 13

8 years ago
(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.

Updated

8 years ago
Attachment #385694 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Updated

7 years ago
Blocks: 459353
Comment on attachment 385694 [details] [diff] [review]
patch

Cancelling review request since this never got anywhere after the r-.
Attachment #385694 - Flags: review?(marco.zehe)
(Assignee)

Updated

5 years ago
Blocks: 661293
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Created attachment 598871 [details] [diff] [review]
patch
Attachment #385694 - Attachment is obsolete: true
Attachment #598871 - Flags: review?(trev.saunders)
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.
Attachment #598871 - Flags: review?(trev.saunders)
Attachment #598871 - Flags: review+
Attachment #598871 - Flags: feedback?
Attachment #598871 - Flags: feedback? → feedback?(bugs)
(Assignee)

Comment 19

5 years ago
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
Attachment #598871 - Flags: feedback?(bugs) → feedback+
(Assignee)

Comment 21

5 years ago
(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
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ee509ada03b

Updated

5 years ago
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/6ee509ada03b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.