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)

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.

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?

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

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.

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

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

> Yes, object attributes like "level", "setsize" and "posinset". Geckoisms. :-) We don't get this across the board.

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

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

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

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

Joanie do you still want the parent of relation?

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

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.

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

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

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

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

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.

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

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

(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

(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

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

patch 4 was from antoher bug

(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

Trev, what is the status?

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?

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

(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