527461 - Implement RELATION_NODE_PARENT_OF

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Joanmarie Diggs]

Atk [1], at-spi [2], and gtk+/gail [3] now have RELATION_NODE_PARENT_OF, which is a reciprocal relation to NODE_CHILD_OF. This new relation provides ATs with significant performance improvement in trees/tables which manage their descendants. It should be implemented by Gecko.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=569427 (atk)
[2] https://bugzilla.gnome.org/show_bug.cgi?id=569428 (at-spi)
[3] https://bugzilla.gnome.org/show_bug.cgi?id=569430 (gtk+/gail)

Comment 1

[alexander :surkov (:asurkov)]

We need to ping IA2 group to see if they find this relation useful. From implementation point of view it would be easier to implement this both for IA2 and ATK.

Comment 2

[James Teh [:Jamie]]

In Windows, we use NODE_CHILD_OF because accParent (the real accessible object parent) is sometimes broken by the default MSAA hierarchy (e.g. window objects). However, I didn't think that child relationships would suffer from this for Gecko objects, as they can always safely return the right children. I don't understand how this works for ATK/AT-SPI. What benefit does NODE_PARENT_OF offer over the normal methods for retrieving children?

Comment 3

[Joanmarie Diggs]

(In reply to comment #2)
> don't understand how this works for ATK/AT-SPI. What benefit does
> NODE_PARENT_OF offer over the normal methods for retrieving children?

Probably it's an atk/at-spi thing. As I explained in the original atk/at-spi/gail bugs, in a tree which manages its descendants, the items contained in the expanded node are not children in the accessible hierarchy. Therefore, the only way to determine the number of *functional*/displayed children of an expanded node is to traverse the tree row by row looking for the NODE_CHILD_OF relationship and seeing if it happens to point to the expanded item under consideration. This is at best tedious and slow. In enormous trees in which most items are expanded, the system can grind to a halt. Literally.

Comment 4

[James Teh [:Jamie]]

Ah, okay. I thought that Gecko rendered tree children as children in the a11y hierarchy, but it seems I am wrong. I see that Gecko also "flattens" trees, so this idea makes sense. NVDA doesn't speak the number of items within an expanded branch, which is why we've never hit this problem. (We do speak the number of items in the current branch using IAccessible2::groupPosition. Does ATK/AT-SPI have an equivalent to this?)

Anyway, I think this makes sense for IA2 as well.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> Ah, okay. I thought that Gecko rendered tree children as children in the a11y
> hierarchy, but it seems I am wrong. I see that Gecko also "flattens" trees, so
> this idea makes sense. NVDA doesn't speak the number of items within an
> expanded branch, which is why we've never hit this problem. (We do speak the
> number of items in the current branch using IAccessible2::groupPosition. Does
> ATK/AT-SPI have an equivalent to this?)

Yes, object attributes like "level", "setsize" and "posinset".

> Anyway, I think this makes sense for IA2 as well.

Jamie, please comment this to IA2 mail list.

Comment 6

[Joanmarie Diggs]

> (We do speak the
> number of items in the current branch using IAccessible2::groupPosition. Does
> ATK/AT-SPI have an equivalent to this?)

Different flavor of the same issue. In any object which does *not* manage its own descendants, we can simply do a getIndexInParent. In a tree which does manage its own descendants, we don't have a hierarchical parent-to-child relationship and thus getIndexInParent is useless in that regard. But, now that we have NODE_PARENT_OF, we should be able to ask the child who its parent is, ask the parent for a list of its children, and get the position via the child's index w.r.t. that list. <shrugs and smiles>
 
> Anyway, I think this makes sense for IA2 as well.

Yea! Thanks!!

Comment 7

[Joanmarie Diggs]

> Yes, object attributes like "level", "setsize" and "posinset".

Geckoisms. :-) We don't get this across the board.

Comment 8

[James Teh [:Jamie]]

(In reply to comment #6)
> > (We do speak the
> > number of items in the current branch using IAccessible2::groupPosition. Does
> > ATK/AT-SPI have an equivalent to this?)
> Different flavor of the same issue. In any object which does *not* manage its
> own descendants, we can simply do a getIndexInParent. In a tree which does
> manage its own descendants, we don't have a hierarchical parent-to-child
> relationship and thus getIndexInParent is useless in that regard.
With IAccessible2::groupPosition, we can ask any given item for the number of similar items in that group. If i understand you correctly, you can't do this in ATK/AT-SPI. This makes the case somewhat weaker for NODE_PARENT_OF in IA2, as IAccessible2::groupPosition solves the big problem here. However, as I noted, it makes sense for the sake of symmetry if nothing else.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> > Yes, object attributes like "level", "setsize" and "posinset".
> 
> Geckoisms. :-) We don't get this across the board.

Yeah? :) I thought we introduced this especially for Orca.

Comment 10

[Joanmarie Diggs]

(In reply to comment #9)
> (In reply to comment #7)
> > > Yes, object attributes like "level", "setsize" and "posinset".
> > 
> > Geckoisms. :-) We don't get this across the board.
> 
> Yeah? :) I thought we introduced this especially for Orca.

I'm not sure what the history is there. But what I mean by "across the board" is that OOo doesn't do it, and standard Gtk+ apps don't do it, and no one else does it for Orca. :-)

(I had taken Jamie's question to be about the standard/official atk/at-spi implementation.)

Sorry for any confusion!

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

I'm not sure of the history of "level", "setsize" and "posinset" either, but note they are in ARIA spec.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Joanie do you still want the parent of relation?

Comment 13

[Joanmarie Diggs]

(In reply to David Bolter [:davidb] from comment #12)
> Joanie do you still want the parent of relation?

Yes please.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch

ARIA test suite failure: https://wiki.mozilla.org/Accessibility/ARIA1.0TestSuiteFailures#109

It's not dual in implementation for node_child_of relation:
1) We expose node_child_of on direct children of ARIA tree. It doesn't seem make sense (Jamie, ping!).
2) We expose node_child_of relation for pure MSAA for window traversal. It doesn't make sense for node_parent_of since it's pure ATK thing currently.

Comment 15

[Trevor Saunders (:tbsaunde)]

> It's not dual in implementation for node_child_of relation:
> 1) We expose node_child_of on direct children of ARIA tree. It doesn't seem
> make sense (Jamie, ping!).

Im somewhat concerned about this, it would seem to break use case in comment 3 because if you have something like

<div role=tree>
<div role=treeitem>
<div role=treeitem>item 1.1</div>
</div>
<div role=treeitem aria-level=2>item 1.2</div>
</div>

then I think if you try to iterate over all the children using NODE_PARENT_OF you only get the second child which isn't what you want.

> 2) We expose node_child_of relation for pure MSAA for window traversal. It
> doesn't make sense for node_parent_of since it's pure ATK thing currently.

that seems fine.

Comment 16

[Trevor Saunders (:tbsaunde)]

Comment on attachment 715340 [details] [diff] [review]
patch

>--- a/accessible/src/atk/AccessibleWrap.cpp
>+++ b/accessible/src/atk/AccessibleWrap.cpp
>@@ -851,24 +851,25 @@ refRelationSetCB(AtkObject *aAtkObj)
> 
>   AccessibleWrap* accWrap = GetAccessibleWrap(aAtkObj);
>   if (!accWrap)
>     return relation_set;
> 
>   uint32_t relationTypes[] = {

statitc const?

>+    case nsIAccessibleRelation::RELATION_NODE_PARENT_OF: {
>+      Relation rel(new IDRefsIterator(mDoc, mContent, nsGkAtoms::aria_owns));
>+
>+      // ARIA tree or treegrid can change hierarchy by @aria-level.
>+      if (mRoleMapEntry && (mRoleMapEntry->role == roles::OUTLINEITEM ||
>+          mRoleMapEntry->role == roles::ROW)) {
>+        Accessible* nextSibl = mParent->GetChildAt(mIndexInParent + 1);
>+        if (nextSibl) {
>+          AccGroupInfo* nextSiblGroupInfo = nextSibl->GetGroupInfo();
>+          if (nextSiblGroupInfo->ConceptualParent() == this) {

how do you know nextSiblGroupInfo isn't null?

> Accessible::GetRelations(nsIArray **aRelations)
> {
>   NS_ENSURE_ARG_POINTER(aRelations);
>   *aRelations = nullptr;
> 
>   if (IsDefunct())
>     return NS_ERROR_FAILURE;
> 
>   nsCOMPtr<nsIMutableArray> relations = do_CreateInstance(NS_ARRAY_CONTRACTID);
>   NS_ENSURE_TRUE(relations, NS_ERROR_OUT_OF_MEMORY);
> 
>-  for (uint32_t relType = nsIAccessibleRelation::RELATION_FIRST;
>-       relType < nsIAccessibleRelation::RELATION_LAST;
>-       ++relType) {
>-
>+  uint32_t relationTypes[] = {

static const?  I think it would be nicer to just keep RELATION_LAST, but I guess this is fine.

>+/**
>+ * Relations exposed to IAccessible2.
>+ */
>+static uint32_t sRelationTypesForIA2[] = {

const?

>       testRelation("treeitem4", RELATION_NODE_CHILD_OF, "tree");
>+      testRelation("treeitem3", RELATION_NODE_CHILD_OF, "tree");
>       testRelation("treeitem5", RELATION_NODE_CHILD_OF, "treeitem4");
>+      testRelation("treeitem4", RELATION_NODE_PARENT_OF, "treeitem5");

how is it supposed to work?  it just crashes with null nextSiblGroupInfo locally)

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #15)
> > It's not dual in implementation for node_child_of relation:
> > 1) We expose node_child_of on direct children of ARIA tree. It doesn't seem
> > make sense (Jamie, ping!).
> 
> Im somewhat concerned about this, it would seem to break use case in comment
> 3 because if you have something like

ok, I see the point. For example, it should be useful when direct children alternate with logical grand children like

<div role="tree">
  <div role="treeitem" aria-level="1">item1</div>
  <div role="treeitem" aria-level="2">item1.1</div>
  <div role="treeitem" aria-level="1">item2</div>
</div>

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch2

1) Added node_parent_of relation for XUL trees (I assume that's why Joanie need this bug)
2) AccGroupInfo::FirstItemOf is not dual to AccGroupInfo constructor (I think that TEXT_LEAF is not needed anymore)
3) Added iterators for node_parent_of relations

Comment 19

[Trevor Saunders (:tbsaunde)]

Comment on attachment 715901 [details] [diff] [review]
patch2

>+++ b/accessible/src/atk/AccessibleWrap.cpp
>@@ -848,27 +848,28 @@ refRelationSetCB(AtkObject *aAtkObj)
> {
>   AtkRelationSet* relation_set =
>     ATK_OBJECT_CLASS(parent_class)->ref_relation_set(aAtkObj);
> 
>   AccessibleWrap* accWrap = GetAccessibleWrap(aAtkObj);
>   if (!accWrap)
>     return relation_set;
> 
>-  uint32_t relationTypes[] = {
>+  const uint32_t relationTypes[] = {

why not make it static?

>     nsIAccessibleRelation::RELATION_LABELLED_BY,
>     nsIAccessibleRelation::RELATION_LABEL_FOR,
>+    nsIAccessibleRelation::RELATION_DESCRIBED_BY,
>+    nsIAccessibleRelation::RELATION_DESCRIPTION_FOR,
>     nsIAccessibleRelation::RELATION_NODE_CHILD_OF,
>+    nsIAccessibleRelation::RELATION_NODE_PARENT_OF,
>     nsIAccessibleRelation::RELATION_CONTROLLED_BY,
>     nsIAccessibleRelation::RELATION_CONTROLLER_FOR,
>-    nsIAccessibleRelation::RELATION_EMBEDS,
>     nsIAccessibleRelation::RELATION_FLOWS_TO,
>     nsIAccessibleRelation::RELATION_FLOWS_FROM,
>-    nsIAccessibleRelation::RELATION_DESCRIBED_BY,
>-    nsIAccessibleRelation::RELATION_DESCRIPTION_FOR,
>+    nsIAccessibleRelation::RELATION_EMBEDS,
>   };
> 
>   for (uint32_t i = 0; i < ArrayLength(relationTypes); i++) {
>     AtkRelationType atkType = static_cast<AtkRelationType>(relationTypes[i]);
>     AtkRelation* atkRelation =
>       atk_relation_set_get_relation_by_type(relation_set, atkType);

so, you broke this because it assumes atk_relation_labeled_by == nsIAccessibleRelation::RELATION_LABELED_BY etc

>+Accessible*
>+AccGroupInfo::FirstItemOf(Accessible* aContainer)
>+{
>+  // ARIA trees can be arranged by ARIA groups, otherwise aria-level works.
>+  a11y::role containerRole = aContainer->Role();
>+  Accessible* item = aContainer->NextSibling();
>+  if (item) {
>+    if (containerRole == roles::OUTLINEITEM && item->Role() == roles::GROUPING)
>+      item = item->FirstChild();
>+
>+    AccGroupInfo* itemGroupInfo = item->GetGroupInfo();
>+    if (itemGroupInfo && itemGroupInfo->ConceptualParent() == aContainer)
>+      return item;
>+  }
>+
>+  // Otherwise it can be a direct child.
>+  item = aContainer->FirstChild();
>+  if (item && IsConceptualParent(BaseRole(item->Role()), containerRole))
>+    return item;
>+
>+  return nullptr;

so what about crazy case where next sibling / first child isn't visible but next accessible is?  I believe we'll say then that the container is the conceptual parent of the accessible, but not the reverse.  I mean this logic doesn't seem to be a mirror of AccGroupInfo::AccGroupInfo()

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #19)

> >-  uint32_t relationTypes[] = {
> >+  const uint32_t relationTypes[] = {
> 
> why not make it static?

thanks again :)

> >   for (uint32_t i = 0; i < ArrayLength(relationTypes); i++) {
> >     AtkRelationType atkType = static_cast<AtkRelationType>(relationTypes[i]);
> >     AtkRelation* atkRelation =
> >       atk_relation_set_get_relation_by_type(relation_set, atkType);
> 
> so, you broke this because it assumes atk_relation_labeled_by ==
> nsIAccessibleRelation::RELATION_LABELED_BY etc

thank you for the catch, I totally missed it

> so what about crazy case where next sibling / first child isn't visible but
> next accessible is?  I believe we'll say then that the container is the
> conceptual parent of the accessible, but not the reverse.  I mean this logic
> doesn't seem to be a mirror of AccGroupInfo::AccGroupInfo()

right but you know I dislike that invisible thing (and text_leaf thing), it's expensive and it seems I can't think of any example where it works (expect unlikely calls in the middle when accessible is still alive but its frame is not). Maybe we should remove them instead?

Comment 21

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 22

[alexander :surkov (:asurkov)]

You know we'd need to update our atk headers before I can add static asserts you talked about since there's no ATK_RELATION_NODE_PARENT_OF constant.

Comment 23

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #22)
> You know we'd need to update our atk headers before I can add static asserts
> you talked about since there's no ATK_RELATION_NODE_PARENT_OF constant.

I can just update that header it should be safe and not hard, or you can just not add an assert for that one.

(In reply to alexander :surkov from comment #20)
> > so what about crazy case where next sibling / first child isn't visible but
> > next accessible is?  I believe we'll say then that the container is the
> > conceptual parent of the accessible, but not the reverse.  I mean this logic
> > doesn't seem to be a mirror of AccGroupInfo::AccGroupInfo()
> 
> right but you know I dislike that invisible thing (and text_leaf thing),
> it's expensive and it seems I can't think of any example where it works
> (expect unlikely calls in the middle when accessible is still alive but its
> frame is not). Maybe we should remove them instead?

I guess we can consider that

Comment 24

[Trevor Saunders (:tbsaunde)]

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

>+  // Keep in sync with AtkRelationType enum.
>+  static const uint32_t relationTypes[] = {
>     nsIAccessibleRelation::RELATION_CONTROLLED_BY,
>     nsIAccessibleRelation::RELATION_CONTROLLER_FOR,
>-    nsIAccessibleRelation::RELATION_EMBEDS,
>+    nsIAccessibleRelation::RELATION_LABEL_FOR,
>+    nsIAccessibleRelation::RELATION_LABELLED_BY,
>+    nsIAccessibleRelation::RELATION_MEMBER_OF,
>+    nsIAccessibleRelation::RELATION_NODE_CHILD_OF,
>     nsIAccessibleRelation::RELATION_FLOWS_TO,
>     nsIAccessibleRelation::RELATION_FLOWS_FROM,
>+    nsIAccessibleRelation::RELATION_SUBWINDOW_OF,
>+    nsIAccessibleRelation::RELATION_EMBEDS,
>+    nsIAccessibleRelation::RELATION_EMBEDDED_BY,
>+    nsIAccessibleRelation::RELATION_POPUP_FOR,
>+    nsIAccessibleRelation::RELATION_PARENT_WINDOW_OF,
>     nsIAccessibleRelation::RELATION_DESCRIBED_BY,
>     nsIAccessibleRelation::RELATION_DESCRIPTION_FOR,
>+    nsIAccessibleRelation::RELATION_NODE_PARENT_OF
>   };
> 
>   for (uint32_t i = 0; i < ArrayLength(relationTypes); i++) {
>-    AtkRelationType atkType = static_cast<AtkRelationType>(relationTypes[i]);
>+    // Shift to 1 to skip ATK_RELATION_NULL.
>+    AtkRelationType atkType = static_cast<AtkRelationType>(i + 1);

no, that doesn't work, ATK_RELATION_LABEL_FOR for example is 3, but nsIAccessibleRelation::RELATION_LABEL_FOR is 1, and ATK_RELATION_CONTROLED_BY sis 1 while nsIAccessibleRelation::RELATION_CONTROLED_BY is 6.

>+AccGroupInfo::FirstItemOf(Accessible* aContainer)
>+{
>+  // ARIA trees can be arranged by ARIA groups, otherwise aria-level works.
>+  a11y::role containerRole = aContainer->Role();
>+  Accessible* item = aContainer->NextSibling();
>+  if (item) {
>+    if (containerRole == roles::OUTLINEITEM && item->Role() == roles::GROUPING)
>+      item = item->FirstChild();
>+
>+    AccGroupInfo* itemGroupInfo = item->GetGroupInfo();
>+    if (itemGroupInfo && itemGroupInfo->ConceptualParent() == aContainer)
>+      return item;
>+  }
>+
>+  // Otherwise it can be a direct child.
>+  item = aContainer->FirstChild();
>+  if (item && IsConceptualParent(BaseRole(item->Role()), containerRole))
>+    return item;

this is presumably the common case right? so why can't it be first?  It would seem more correct to in case someone does something crazy like

<div role=treeitem>
<div role=treeitem>option1</div>
</div>
<div role=treeitem aria-level=2>
item 2
</div>

>+AccGroupInfo::NextItemTo(Accessible* aItem)
>+{
>+  AccGroupInfo* groupInfo = aItem->GetGroupInfo();
>+  if (!groupInfo)
>+    return nullptr;
>+
>+  // If the item in middle of the group then search next item in siblings.
>+  if (groupInfo->PosInSet() >= groupInfo->SetSize())
>+    return nullptr;
>+
>+  Accessible* nextItem = nullptr;
>+  Accessible* parent = aItem->Parent();
>+  int32_t idx = aItem->IndexInParent() + 1;
>+  while ((nextItem = parent->GetChildAt(idx++))) {

please make ++ its own statement (just make it a for loop is probbly the clearest thing)

>+    AccGroupInfo* nextGroupInfo = nextItem->GetGroupInfo();
>+    if (nextGroupInfo &&
>+        nextGroupInfo->ConceptualParent() == groupInfo->ConceptualParent()) {
>+      return nextItem;
>+    }
>+  }

in addition to the case above isn't this broken for the even crazier

<div role=treeitem>
<div role=treeitem>option 1</div>
</div>
<div role=group>
<div role=treeitem>
option 2</div>
</div>
<div role=treeitem aria-level=2>option 3</div>

>   uint32_t mSetSize;
>   Accessible* mParent;

it makes me pretty sad, but is that safe?  what happens if someone has
<div role=treeitem></div>
<div role=treeitem aria-level=2>option 1</div>

and then removes the child tree item from the tree?

>+Accessible*
>+XULTreeItemIterator::Next()
>+{
>+  while (mCurrRowIdx < mRowCount) {
>+    int32_t level = 0;
>+    mTreeView->GetLevel(mCurrRowIdx, &level);
>+
>+    if (level == mContainerLevel + 1)
>+      return mXULTree->GetTreeItemAccessible(mCurrRowIdx++);

seperate statemenet please

>+XULTreeAccessible::RelationByType(uint32_t aType)
>+{
>+  if (!mTreeView)
>+    return Relation();

hm, couldn't it still have a label or something?

Comment 25

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #23)
> (In reply to alexander :surkov from comment #22)
> > You know we'd need to update our atk headers before I can add static asserts
> > you talked about since there's no ATK_RELATION_NODE_PARENT_OF constant.
> 
> I can just update that header it should be safe and not hard, or you can
> just not add an assert for that one.

I filed bug 843926.

> (In reply to alexander :surkov from comment #20)
> > > so what about crazy case where next sibling / first child isn't visible but
> > > next accessible is?  I believe we'll say then that the container is the
> > > conceptual parent of the accessible, but not the reverse.  I mean this logic
> > > doesn't seem to be a mirror of AccGroupInfo::AccGroupInfo()
> > 
> > right but you know I dislike that invisible thing (and text_leaf thing),
> > it's expensive and it seems I can't think of any example where it works
> > (expect unlikely calls in the middle when accessible is still alive but its
> > frame is not). Maybe we should remove them instead?
> 
> I guess we can consider that

I think I should file a bug.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #24)

> >     nsIAccessibleRelation::RELATION_DESCRIPTION_FOR,
> >+    nsIAccessibleRelation::RELATION_NODE_PARENT_OF
> >   };
> > 
> >   for (uint32_t i = 0; i < ArrayLength(relationTypes); i++) {
> >-    AtkRelationType atkType = static_cast<AtkRelationType>(relationTypes[i]);
> >+    // Shift to 1 to skip ATK_RELATION_NULL.
> >+    AtkRelationType atkType = static_cast<AtkRelationType>(i + 1);
> 
> no, that doesn't work, ATK_RELATION_LABEL_FOR for example is 3, but
> nsIAccessibleRelation::RELATION_LABEL_FOR is 1, and
> ATK_RELATION_CONTROLED_BY sis 1 while
> nsIAccessibleRelation::RELATION_CONTROLED_BY is 6.

yes, ATK relation type is array index + 1, Gecko relation type is a value at the given index.

> this is presumably the common case right? so why can't it be first?  It
> would seem more correct to in case someone does something crazy like
> 
> <div role=treeitem>
> <div role=treeitem>option1</div>
> </div>
> <div role=treeitem aria-level=2>
> item 2
> </div>

this example isn't valid since treeitem aren't allowed to contain tree items. You have two ways to introduce hierarchy:

<div role=treeitem aria-level=1></div>
<div role=treeitem aria-level=2></div> - a child of previous item

or

<div role=treeitem></div>
<div role="group">
  <div role=treeitem></div> - a child of previous item
</div>


> >+AccGroupInfo::NextItemTo(Accessible* aItem)
> >+  while ((nextItem = parent->GetChildAt(idx++))) {
> 
> please make ++ its own statement (just make it a for loop is probbly the
> clearest thing)

ok

> >+    AccGroupInfo* nextGroupInfo = nextItem->GetGroupInfo();
> >+    if (nextGroupInfo &&
> >+        nextGroupInfo->ConceptualParent() == groupInfo->ConceptualParent()) {
> >+      return nextItem;
> >+    }
> >+  }
> 
> in addition to the case above isn't this broken for the even crazier
> 
> <div role=treeitem>
> <div role=treeitem>option 1</div>
> </div>
> <div role=group>
> <div role=treeitem>
> option 2</div>
> </div>
> <div role=treeitem aria-level=2>option 3</div>

I don't think we are good at mixed hierarchies. But I hope users don't have a point to use them both on the same tree.

> >   uint32_t mSetSize;
> >   Accessible* mParent;
> 
> it makes me pretty sad, but is that safe?  what happens if someone has
> <div role=treeitem></div>
> <div role=treeitem aria-level=2>option 1</div>
> 
> and then removes the child tree item from the tree?

we need to try, it doesn't look safe

> >+Accessible*
> >+XULTreeItemIterator::Next()
> >+{
> >+  while (mCurrRowIdx < mRowCount) {
> >+    int32_t level = 0;
> >+    mTreeView->GetLevel(mCurrRowIdx, &level);
> >+
> >+    if (level == mContainerLevel + 1)
> >+      return mXULTree->GetTreeItemAccessible(mCurrRowIdx++);
> 
> seperate statemenet please

ok but why you don;t like them

> >+XULTreeAccessible::RelationByType(uint32_t aType)
> >+{
> >+  if (!mTreeView)
> >+    return Relation();
> 
> hm, couldn't it still have a label or something?

good point

Comment 27

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #26)
> (In reply to Trevor Saunders (:tbsaunde) from comment #24)
> 
> > >     nsIAccessibleRelation::RELATION_DESCRIPTION_FOR,
> > >+    nsIAccessibleRelation::RELATION_NODE_PARENT_OF
> > >   };
> > > 
> > >   for (uint32_t i = 0; i < ArrayLength(relationTypes); i++) {
> > >-    AtkRelationType atkType = static_cast<AtkRelationType>(relationTypes[i]);
> > >+    // Shift to 1 to skip ATK_RELATION_NULL.
> > >+    AtkRelationType atkType = static_cast<AtkRelationType>(i + 1);
> > 
> > no, that doesn't work, ATK_RELATION_LABEL_FOR for example is 3, but
> > nsIAccessibleRelation::RELATION_LABEL_FOR is 1, and
> > ATK_RELATION_CONTROLED_BY sis 1 while
> > nsIAccessibleRelation::RELATION_CONTROLED_BY is 6.
> 
> yes, ATK relation type is array index + 1, Gecko relation type is a value at
> the given index.

ah, I see you changed how it works.

> > this is presumably the common case right? so why can't it be first?  It
> > would seem more correct to in case someone does something crazy like
> > 
> > <div role=treeitem>
> > <div role=treeitem>option1</div>
> > </div>
> > <div role=treeitem aria-level=2>
> > item 2
> > </div>
> 
> this example isn't valid since treeitem aren't allowed to contain tree
> items. You have two ways to introduce hierarchy:

hm, that's strange API / unfortunate though I guess it makes a little sense :/

> > >+    AccGroupInfo* nextGroupInfo = nextItem->GetGroupInfo();
> > >+    if (nextGroupInfo &&
> > >+        nextGroupInfo->ConceptualParent() == groupInfo->ConceptualParent()) {
> > >+      return nextItem;
> > >+    }
> > >+  }
> > 
> > in addition to the case above isn't this broken for the even crazier
> > 
> > <div role=treeitem>
> > <div role=treeitem>option 1</div>
> > </div>
> > <div role=group>
> > <div role=treeitem>
> > option 2</div>
> > </div>
> > <div role=treeitem aria-level=2>option 3</div>
> 
> I don't think we are good at mixed hierarchies. But I hope users don't have
> a point to use them both on the same tree.

yeah, I think that's true, and I don't really care either

> > >   uint32_t mSetSize;
> > >   Accessible* mParent;
> > 
> > it makes me pretty sad, but is that safe?  what happens if someone has
> > <div role=treeitem></div>
> > <div role=treeitem aria-level=2>option 1</div>
> > 
> > and then removes the child tree item from the tree?
> 
> we need to try, it doesn't look safe

I think InvalidateChildren() may save us, but not sure

> > >+Accessible*
> > >+XULTreeItemIterator::Next()
> > >+{
> > >+  while (mCurrRowIdx < mRowCount) {
> > >+    int32_t level = 0;
> > >+    mTreeView->GetLevel(mCurrRowIdx, &level);
> > >+
> > >+    if (level == mContainerLevel + 1)
> > >+      return mXULTree->GetTreeItemAccessible(mCurrRowIdx++);
> > 
> > seperate statemenet please
> 
> ok but why you don;t like them

I can never remember what the order means

Comment 28

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #27)

> > > >   uint32_t mSetSize;
> > > >   Accessible* mParent;
> > > 
> > > it makes me pretty sad, but is that safe?  what happens if someone has
> > > <div role=treeitem></div>
> > > <div role=treeitem aria-level=2>option 1</div>
> > > 
> > > and then removes the child tree item from the tree?
> > 
> > we need to try, it doesn't look safe
> 
> I think InvalidateChildren() may save us, but not sure

I'll do a shot. It looks like we should crash.

> > > >+    if (level == mContainerLevel + 1)
> > > >+      return mXULTree->GetTreeItemAccessible(mCurrRowIdx++);
> > > 
> > > seperate statemenet please
> > 
> > ok but why you don;t like them
> 
> I can never remember what the order means

actually I'm not sure how to change it nice. I don't like to have temporary variable for result or increment an index and then minus 1 it for the call.

Comment 29

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 30

[alexander :surkov (:asurkov)]

Attachment: patch5

patch 4 was from antoher bug

Comment 31

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #28)
> (In reply to Trevor Saunders (:tbsaunde) from comment #27)
> 
> > > > >   uint32_t mSetSize;
> > > > >   Accessible* mParent;
> > > > 
> > > > it makes me pretty sad, but is that safe?  what happens if someone has
> > > > <div role=treeitem></div>
> > > > <div role=treeitem aria-level=2>option 1</div>
> > > > 
> > > > and then removes the child tree item from the tree?
> > > 
> > > we need to try, it doesn't look safe
> > 
> > I think InvalidateChildren() may save us, but not sure
> 
> I'll do a shot. It looks like we should crash.

bug 844023 filed

Comment 32

[alexander :surkov (:asurkov)]

Trev, what is the status?

Comment 33

[Trevor Saunders (:tbsaunde)]

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

>+  // Keep in sync with AtkRelationType enum.
>+  static const uint32_t relationTypes[] = {

why not just add static asserts for what you can without the node parent of relation?

>+Accessible*
>+AccGroupInfo::NextItemTo(Accessible* aItem)

this stuff makes me sad and my eyes glaze over but I don't really have a better idea or care anymore :(

> Accessible::GetRelations(nsIArray **aRelations)
> {
>   NS_ENSURE_ARG_POINTER(aRelations);
>   *aRelations = nullptr;
> 
>   if (IsDefunct())
>     return NS_ERROR_FAILURE;
> 
>   nsCOMPtr<nsIMutableArray> relations = do_CreateInstance(NS_ARRAY_CONTRACTID);
>   NS_ENSURE_TRUE(relations, NS_ERROR_OUT_OF_MEMORY);
> 
>-  for (uint32_t relType = nsIAccessibleRelation::RELATION_FIRST;
>-       relType < nsIAccessibleRelation::RELATION_LAST;
>-       ++relType) {
>-
>+  const uint32_t relationTypes[] = {

why not make it static?

Comment 34

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #33)

> >+  // Keep in sync with AtkRelationType enum.
> >+  static const uint32_t relationTypes[] = {
> 
> why not just add static asserts for what you can without the node parent of
> relation?

ok.

> >+Accessible*
> >+AccGroupInfo::NextItemTo(Accessible* aItem)
> 
> this stuff makes me sad and my eyes glaze over but I don't really have a
> better idea or care anymore :(

maybe it will be changed next one hundred years :)

> >+  const uint32_t relationTypes[] = {
> 
> why not make it static?

where do you find those? :)

Comment 35

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #33)
> Comment on attachment 716942 [details] [diff] [review]
> patch5
> 
> >+  // Keep in sync with AtkRelationType enum.
> >+  static const uint32_t relationTypes[] = {
> 
> why not just add static asserts for what you can without the node parent of
> relation?

you know static_assert doesn't like static const arrays and generates errors

../../../../accessible/src/atk/AccessibleWrap.cpp: In function 'AtkRelationSet* refRelationSetCB(AtkObject*)':
../../../../accessible/src/atk/AccessibleWrap.cpp:876:18: error: 'relationTypes' cannot appear in a constant-expression
../../../../accessible/src/atk/AccessibleWrap.cpp:876:62: error: an array reference cannot appear in a constant-expression

Comment 36

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2d9e36ff5c1b

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d9e36ff5c1b