Last Comment Bug 673389 - AccGroupInfo::ConceptualParent() shouldn't consider random parent accessibles to be a parent
: AccGroupInfo::ConceptualParent() shouldn't consider random parent accessibles...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks: 641838
  Show dependency treegraph
 
Reported: 2011-07-22 05:11 PDT by Trevor Saunders (:tbsaunde)
Modified: 2011-08-03 20:59 PDT (History)
2 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.16 KB, patch)
2011-07-22 05:13 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (1.16 KB, patch)
2011-07-22 05:53 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (1.02 KB, patch)
2011-07-22 06:42 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review-
Details | Diff | Splinter Review
patch2 (1.02 KB, patch)
2011-07-22 06:49 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch3 (3.26 KB, patch)
2011-07-22 07:31 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review-
Details | Diff | Splinter Review
patch4 (6.30 KB, patch)
2011-07-22 08:48 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (6.93 KB, patch)
2011-07-22 23:59 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (8.43 KB, patch)
2011-07-23 04:32 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (8.32 KB, patch)
2011-07-24 06:51 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
bug 673389 (8.47 KB, patch)
2011-07-25 01:57 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
bug 673389 (8.57 KB, patch)
2011-07-25 02:58 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2011-07-22 05:11:30 PDT
AccGroupInfo.cpp:150 instead of checking that the parent of the accessible isn't a grouping we should check that the parent is something that could reasonably be the conceptual parent.
Comment 1 Trevor Saunders (:tbsaunde) 2011-07-22 05:13:19 PDT
Created attachment 547664 [details] [diff] [review]
patch
Comment 2 alexander :surkov 2011-07-22 05:15:44 PDT
it doesn't look like this patch against trunk
Comment 3 Trevor Saunders (:tbsaunde) 2011-07-22 05:53:39 PDT
Created attachment 547671 [details] [diff] [review]
patch

odd, is this better?
Comment 4 alexander :surkov 2011-07-22 06:15:50 PDT
(In reply to comment #3)
> Created attachment 547671 [details] [diff] [review] [review]
> patch
> 
> odd, is this better?

it's the same
Comment 5 Trevor Saunders (:tbsaunde) 2011-07-22 06:42:21 PDT
Created attachment 547679 [details] [diff] [review]
patch

I see what happend now.
Comment 6 alexander :surkov 2011-07-22 06:44:36 PDT
Comment on attachment 547679 [details] [diff] [review]
patch

the code below this check is executed for group role only
Comment 7 Trevor Saunders (:tbsaunde) 2011-07-22 06:49:43 PDT
Created attachment 547681 [details] [diff] [review]
patch2

does this make more sense?
Comment 8 alexander :surkov 2011-07-22 06:51:07 PDT
(In reply to comment #7)
> Created attachment 547681 [details] [diff] [review] [review]
> patch2
> 
> does this make more sense?

it's the same
Comment 9 Trevor Saunders (:tbsaunde) 2011-07-22 07:31:00 PDT
Created attachment 547686 [details] [diff] [review]
patch3

add tests and commit the change
Comment 10 alexander :surkov 2011-07-22 07:56:25 PDT
Comment on attachment 547686 [details] [diff] [review]
patch3


>   // In the case of ARIA tree, a tree can be arranged by using ARIA groups
>   // to organize levels. In this case the parent of the tree item will be
>   // a group and the previous treeitem of that should be the tree item
>   // parent. Or, if the parent is something other than a tree we will
>   // return that.
> 
>-  if (parentRole != nsIAccessibleRole::ROLE_GROUPING) {
>+  if (parentRole == nsIAccessibleRole::ROLE_LIST
>+      || parentRole == nsIAccessibleRole::ROLE_OUTLINE
>+      || parentRole == nsIAccessibleRole::ROLE_COMBOBOX_LIST
>+      || parentRole == nsIAccessibleRole::ROLE_LISTBOX) {
>     mParent = parent;
>     return;

this code shouldn't be under that comment

refer to CreateGroupInfo for possible child roles, so you can restore parent roles from this

>+  } else if (parentRole != nsIAccessibleRole::ROLE_GROUPING) {
>+    return;

if ()
  return
else if ()
  return

is funny

>       var iframeAcc = getAccessible("iframe", null, iframeElmObj);
>       var iframeDoc = iframeElmObj.value.contentDocument;
>       var iframeDocAcc = getAccessible(iframeDoc);
>       testRelation(iframeDocAcc, RELATION_NODE_CHILD_OF, iframeAcc);
> 
>+      // RELATION_NODE_CHILD_OF for accessible with no special parent"node child of"
>+      testRelation("treeitem6", RELATION_NODE_CHILD_OF);

doesn't it make sense to keep it with other treeitems tests.
please pass null explicetly
is special word right here? maybe expected, correct or something
space between parent"node child of"
please add html:a for this bug
Comment 11 Trevor Saunders (:tbsaunde) 2011-07-22 08:48:08 PDT
Created attachment 547705 [details] [diff] [review]
patch4

address comments
Comment 12 alexander :surkov 2011-07-22 09:23:36 PDT
Comment on attachment 547705 [details] [diff] [review]
patch4

Review of attachment 547705 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/AccGroupInfo.cpp
@@ +146,5 @@
> +      || parentRole == nsIAccessibleRole::ROLE_COMBOBOX_LIST
> +      || parentRole == nsIAccessibleRole::ROLE_LISTBOX
> +      || parentRole == nsIAccessibleRole::ROLE_PAGETABLIST)
> +    mParent = parent;
> +

I'd expect to have something like we have above, otherwise it's funny that let's say treeitem parented by pagetablist is legal.

::: accessible/src/base/nsAccessible.cpp
@@ +2064,5 @@
>          AccGroupInfo* groupInfo = GetGroupInfo();
>          if (!groupInfo)
>            return rel;
>  
> +        rel.AppendIter(new SingleAccIterator(groupInfo->ConceptualParent()));

where do ears grow from? :)

::: accessible/tests/mochitest/relations/test_general.xul
@@ +150,5 @@
>          Mozilla Bug 475298
>        </a><br/>
> +      <a target="_blank"
> +         href="https://bugzilla.mozilla.org/show_bug.cgi?id=673389"
> +         title="special case item without container">

not too descriptive
Comment 13 alexander :surkov 2011-07-22 09:25:36 PDT
(In reply to comment #12)

> ::: accessible/src/base/nsAccessible.cpp
> @@ +2064,5 @@
> >          AccGroupInfo* groupInfo = GetGroupInfo();
> >          if (!groupInfo)
> >            return rel;
> >  
> > +        rel.AppendIter(new SingleAccIterator(groupInfo->ConceptualParent()));
> 
> where do ears grow from? :)

I suggest to put relation patch on top of this, otherwise this makes these patches Siamese twins.
Comment 14 Trevor Saunders (:tbsaunde) 2011-07-22 23:59:39 PDT
Created attachment 547898 [details] [diff] [review]
patch

rebase to trunk from on top of 641838
Comment 15 alexander :surkov 2011-07-23 02:57:50 PDT
Comment on attachment 547898 [details] [diff] [review]
patch

>diff --git a/accessible/src/base/AccGroupInfo.cpp b/accessible/src/base/AccGroupInfo.cpp
>index 191487f..d71c159 100644
>--- a/accessible/src/base/AccGroupInfo.cpp
>+++ b/accessible/src/base/AccGroupInfo.cpp
>@@ -136,27 +136,42 @@ AccGroupInfo::AccGroupInfo(nsAccessible* aItem, PRUint32 aRole) :
>   // In the case of ARIA row in treegrid, return treegrid since ARIA
>   // groups aren't used to organize levels in ARIA treegrids.
>   if (aRole == nsIAccessibleRole::ROLE_ROW &&
>       parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) {
>     mParent = parent;
>     return;
>   }
> 
>+  if ((parentRole == nsIAccessibleRole::ROLE_LIST
>+       && aRole == nsIAccessibleRole::ROLE_LISTITEM)
>+      || (parentRole == nsIAccessibleRole::ROLE_OUTLINE
>+        && aRole == nsIAccessibleRole::ROLE_OUTLINEITEM)
>+      || (parentRole == nsIAccessibleRole::ROLE_COMBOBOX_LIST
>+        && aRole == nsIAccessibleRole::ROLE_COMBOBOX_OPTION)
>+      || (parentRole == nsIAccessibleRole::ROLE_LISTBOX
>+        && aRole == nsIAccessibleRole::ROLE_OPTION)
>+      || (parentRole == nsIAccessibleRole::ROLE_PAGETABLIST
>+        && aRole == nsIAccessibleRole::ROLE_PAGETAB)
>+      || (parentRole == nsIAccessibleRole::ROLE_ROW
>+        && (aRole == nsIAccessibleRole::ROLE_CELL
>+          || aRole == nsIAccessibleRole::ROLE_GRID_CELL))
>+          || ((parentRole == nsIAccessibleRole::ROLE_TABLE
>+          || parentRole == nsIAccessibleRole::ROLE_TREE_TABLE)
>+          && aRole == nsIAccessibleRole::ROLE_ROW))
>+    mParent = parent;

Maybe it's nicer to put into helper function like BaseRole method. You could do more readable things like 
if (role == Role1 && parentRole == Role2)
  return true;

if (bla bla)
  return true;

also no menuitems, and cell and grid_cell aren't children of table and tree_table. Btw, you shouldn't keep row and tree_table checks separately. That requires to change commenting in nice way.

also like shouldn't start from operators, put || and && to the end of line

>+      // RELATION NODE CHILD OF for accessible without expected container
>+      testRelation("treeitem6", RELATION_NODE_CHILD_OF, null);

maybe "no node_child_of relation when accessible is contained by not excpected parent" or similar

>+      <a target="_blank"
>+         href="https://bugzilla.mozilla.org/show_bug.cgi?id=673389"
>+         title="node child of on an item not in a container"

maybe "node_child_of relation on an item not in a proper container"
Comment 16 Trevor Saunders (:tbsaunde) 2011-07-23 04:32:07 PDT
Created attachment 547916 [details] [diff] [review]
patch
Comment 17 alexander :surkov 2011-07-23 05:12:17 PDT
Comment on attachment 547916 [details] [diff] [review]
patch

Review of attachment 547916 [details] [diff] [review]:
-----------------------------------------------------------------

next round?

::: accessible/src/base/AccGroupInfo.cpp
@@ +138,5 @@
>    if (aRole == nsIAccessibleRole::ROLE_ROW &&
>        parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) {
>      mParent = parent;
>      return;
>    }

this code is part of IsConceptualParent() except return statement but you can deal with it later. Also that allows you to not get parentRole twice

also move this comment after the comment below like "Note: in the case of "

@@ +141,5 @@
>      return;
>    }
>  
> +  if (IsConceptualParent(aRole, parent))
> +      mParent = parent;

new line please

@@ +143,5 @@
>  
> +  if (IsConceptualParent(aRole, parent))
> +      mParent = parent;
> +  if (parentRole != nsIAccessibleRole::ROLE_GROUPING)
> +    return;

add a check for role == outline_item, it appears this role is what all code for.

don't move it above the comment

@@ +149,5 @@
>    // In the case of ARIA tree, a tree can be arranged by using ARIA groups
>    // to organize levels. In this case the parent of the tree item will be
>    // a group and the previous treeitem of that should be the tree item
>    // parent. Or, if the parent is something other than a tree we will
>    // return that.

"Of, if the parent" isn't applicable anymore

@@ +178,5 @@
> +  PRUint32 parentRole = aParent->Role();
> +
> +  if (parentRole == nsIAccessibleRole::ROLE_LIST &&
> +      aRole == nsIAccessibleRole::ROLE_LISTITEM)
> +    return true;

please fix indentation for everything after this

@@ +181,5 @@
> +      aRole == nsIAccessibleRole::ROLE_LISTITEM)
> +    return true;
> +      if (parentRole == nsIAccessibleRole::ROLE_OUTLINE &&
> +          aRole == nsIAccessibleRole::ROLE_OUTLINEITEM)
> +        return true;

only this makes code related with grouping role not valid, need to have one more check

and please move table and outline check on top because primary all code is for them

@@ +199,5 @@
> +          if ((parentRole == nsIAccessibleRole::ROLE_TABLE ||
> +                parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) &&
> +              aRole == nsIAccessibleRole::ROLE_ROW)
> +                return true;
> +          if ((parentRole == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||

I don't think parent menuitem ever contains menuitem, I think it contains menupopup. No?

@@ +200,5 @@
> +                parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) &&
> +              aRole == nsIAccessibleRole::ROLE_ROW)
> +                return true;
> +          if ((parentRole == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||
> +                parentRole == nsIAccessibleRole::ROLE_POPUP_MENU ||

ROLE_MENUPOPUP, it's better to check the patch is compiled before asking for r :)

@@ +201,5 @@
> +              aRole == nsIAccessibleRole::ROLE_ROW)
> +                return true;
> +          if ((parentRole == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||
> +                parentRole == nsIAccessibleRole::ROLE_POPUP_MENU ||
> +                parentRole == nsIAccessibleRole::ROLE_BUTTONMENU ||

does buttonmenu contains menuitems?

@@ +203,5 @@
> +          if ((parentRole == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||
> +                parentRole == nsIAccessibleRole::ROLE_POPUP_MENU ||
> +                parentRole == nsIAccessibleRole::ROLE_BUTTONMENU ||
> +                parentRole == nsIAccessibleRole::ROLE_MENUPOPUP) &&
> +              aRole == nsIAccessibleRole::ROLE_MENUITEM)

use BaseRole instead

::: accessible/src/base/AccGroupInfo.h
@@ +87,5 @@
>        return nsIAccessibleRole::ROLE_MENUITEM;
>      return aRole;
>    }
>  
> +  static bool IsConceptualParent(PRUint32 aRole, nsAccessible* aParent);

comment please?

::: accessible/tests/mochitest/relations/test_general.xul
@@ +68,5 @@
>        testRelation("treeitem3", RELATION_NODE_CHILD_OF, "tree");
>        testRelation("treeitem4", RELATION_NODE_CHILD_OF, "tree");
>        testRelation("treeitem5", RELATION_NODE_CHILD_OF, "treeitem4");
>  
> +      // RELATION NODE CHILD OF for accessible without expected container

not addressed from previous review

@@ +150,5 @@
>          Mozilla Bug 475298
>        </a><br/>
> +      <a target="_blank"
> +         href="https://bugzilla.mozilla.org/show_bug.cgi?id=673389"
> +         title="node child of on an item not in a container"

too
Comment 18 Trevor Saunders (:tbsaunde) 2011-07-23 05:40:58 PDT
(In reply to comment #17)
> Comment on attachment 547916 [details] [diff] [review] [review]
> patch
> 
> Review of attachment 547916 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> next round?

if you like though I'm not sure I should keep acting has if I have a license to write code before sleeping ;)

> ::: accessible/src/base/AccGroupInfo.cpp
> @@ +138,5 @@
> >    if (aRole == nsIAccessibleRole::ROLE_ROW &&
> >        parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) {
> >      mParent = parent;
> >      return;
> >    }
> 
> this code is part of IsConceptualParent() except return statement but you
> can deal with it later. Also that allows you to not get parentRole twice

but the return is delt with by if (parentRole != role_grouping) right so that code can just get rm'd right?

> also move this comment after the comment below like "Note: in the case of "

this comment == ?

> > +  if (IsConceptualParent(aRole, parent))
> > +      mParent = parent;
> > +  if (parentRole != nsIAccessibleRole::ROLE_GROUPING)
> > +    return;
> 
> add a check for role == outline_item, it appears this role is what all code
> for.

you mean

if (role != role_grouping || role != role_outlineitem)
return;

> don't move it above the comment

this == that last if right?

> @@ +149,5 @@
> >    // In the case of ARIA tree, a tree can be arranged by using ARIA groups
> >    // to organize levels. In this case the parent of the tree item will be
> >    // a group and the previous treeitem of that should be the tree item
> >    // parent. Or, if the parent is something other than a tree we will
> >    // return that.
> 
> "Of, if the parent" isn't applicable anymore

you mean  "or if the parent isn't a tree..." right? just delete it right?

> @@ +181,5 @@
> > +      aRole == nsIAccessibleRole::ROLE_LISTITEM)
> > +    return true;
> > +      if (parentRole == nsIAccessibleRole::ROLE_OUTLINE &&
> > +          aRole == nsIAccessibleRole::ROLE_OUTLINEITEM)
> > +        return true;
> 
> only this makes code related with grouping role not valid, need to have one
> more check

hm, wont the setting of mParent get written over in the constructor in that case?

> and please move table and outline check on top because primary all code is
> for them

you mean  those are the most common case we want to be fastest? 

> @@ +199,5 @@
> > +          if ((parentRole == nsIAccessibleRole::ROLE_TABLE ||
> > +                parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) &&
> > +              aRole == nsIAccessibleRole::ROLE_ROW)
> > +                return true;
> > +          if ((parentRole == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||
> 
> I don't think parent menuitem ever contains menuitem, I think it contains
> menupopup. No?

I'm not sure but I'll believe you ;)

> @@ +200,5 @@
> > +                parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) &&
> > +              aRole == nsIAccessibleRole::ROLE_ROW)
> > +                return true;
> > +          if ((parentRole == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||
> > +                parentRole == nsIAccessibleRole::ROLE_POPUP_MENU ||
> 
> ROLE_MENUPOPUP, it's better to check the patch is compiled before asking for
> r :)

huh, I was pretty sure this did compile localy :/

> @@ +201,5 @@
> > +              aRole == nsIAccessibleRole::ROLE_ROW)
> > +                return true;
> > +          if ((parentRole == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||
> > +                parentRole == nsIAccessibleRole::ROLE_POPUP_MENU ||
> > +                parentRole == nsIAccessibleRole::ROLE_BUTTONMENU ||
> 
> does buttonmenu contains menuitems?

maybe? or maybe it contains  a menupopup which contains the menuitems? the second seems more likely to me.

> @@ +203,5 @@
> > +          if ((parentRole == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||
> > +                parentRole == nsIAccessibleRole::ROLE_POPUP_MENU ||
> > +                parentRole == nsIAccessibleRole::ROLE_BUTTONMENU ||
> > +                parentRole == nsIAccessibleRole::ROLE_MENUPOPUP) &&
> > +              aRole == nsIAccessibleRole::ROLE_MENUITEM)
> 
> use BaseRole instead

BaseRole() only deals with converting menuitem roles to menuitem no?
Comment 19 alexander :surkov 2011-07-23 06:03:28 PDT
(In reply to comment #18)
> > next round?
> 
> if you like though I'm not sure I should keep acting has if I have a license
> to write code before sleeping ;)

you don't need to do that today :)

> > ::: accessible/src/base/AccGroupInfo.cpp
> > @@ +138,5 @@
> > >    if (aRole == nsIAccessibleRole::ROLE_ROW &&
> > >        parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) {
> > >      mParent = parent;
> > >      return;
> > >    }
> > 
> > this code is part of IsConceptualParent() except return statement but you
> > can deal with it later. Also that allows you to not get parentRole twice
> 
> but the return is delt with by if (parentRole != role_grouping) right so
> that code can just get rm'd right?

you deal with it below, like additional check for outlineitem role.

> > also move this comment after the comment below like "Note: in the case of "
> 
> this comment == ?

that is started by words "In the case of ".

> 
> > > +  if (IsConceptualParent(aRole, parent))
> > > +      mParent = parent;
> > > +  if (parentRole != nsIAccessibleRole::ROLE_GROUPING)
> > > +    return;
> > 
> > add a check for role == outline_item, it appears this role is what all code
> > for.
> 
> you mean
> 
> if (role != role_grouping || role != role_outlineitem)
> return

yeah, that should be what we want, right?

> 
> > don't move it above the comment
> 
> this == that last if right?

right, if (parentRole != nsIAccessibleRole::ROLE_GROUPING)

> > @@ +149,5 @@
> > >    // In the case of ARIA tree, a tree can be arranged by using ARIA groups
> > >    // to organize levels. In this case the parent of the tree item will be
> > >    // a group and the previous treeitem of that should be the tree item
> > >    // parent. Or, if the parent is something other than a tree we will
> > >    // return that.
> > 
> > "Of, if the parent" isn't applicable anymore
> 
> you mean  "or if the parent isn't a tree..." right? just delete it right?

yes

> > @@ +181,5 @@
> > > +      aRole == nsIAccessibleRole::ROLE_LISTITEM)
> > > +    return true;
> > > +      if (parentRole == nsIAccessibleRole::ROLE_OUTLINE &&
> > > +          aRole == nsIAccessibleRole::ROLE_OUTLINEITEM)
> > > +        return true;
> > 
> > only this makes code related with grouping role not valid, need to have one
> > more check
> 
> hm, wont the setting of mParent get written over in the constructor in that
> case?

I misread this code, so ignore

> 
> > and please move table and outline check on top because primary all code is
> > for them
> 
> you mean  those are the most common case we want to be fastest? 
> 
> > @@ +199,5 @@
> > > +          if ((parentRole == nsIAccessibleRole::ROLE_TABLE ||
> > > +                parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) &&
> > > +              aRole == nsIAccessibleRole::ROLE_ROW)
> > > +                return true;
> > > +          if ((parentRole == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||
> > 
> > I don't think parent menuitem ever contains menuitem, I think it contains
> > menupopup. No?
> 
> I'm not sure but I'll believe you ;)

that should be valid for XUL, not sure about ARIA.

> > @@ +201,5 @@
> > > +              aRole == nsIAccessibleRole::ROLE_ROW)
> > > +                return true;
> > > +          if ((parentRole == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||
> > > +                parentRole == nsIAccessibleRole::ROLE_POPUP_MENU ||
> > > +                parentRole == nsIAccessibleRole::ROLE_BUTTONMENU ||
> > 
> > does buttonmenu contains menuitems?
> 
> maybe? or maybe it contains  a menupopup which contains the menuitems? the
> second seems more likely to me.

I think menupopup contains them

> 
> > @@ +203,5 @@
> > > +          if ((parentRole == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||
> > > +                parentRole == nsIAccessibleRole::ROLE_POPUP_MENU ||
> > > +                parentRole == nsIAccessibleRole::ROLE_BUTTONMENU ||
> > > +                parentRole == nsIAccessibleRole::ROLE_MENUPOPUP) &&
> > > +              aRole == nsIAccessibleRole::ROLE_MENUITEM)
> > 
> > use BaseRole instead
> 
> BaseRole() only deals with converting menuitem roles to menuitem no?

correct
Comment 20 Trevor Saunders (:tbsaunde) 2011-07-23 06:13:08 PDT
> > > ::: accessible/src/base/AccGroupInfo.cpp
> > > @@ +138,5 @@
> > > >    if (aRole == nsIAccessibleRole::ROLE_ROW &&
> > > >        parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) {
> > > >      mParent = parent;
> > > >      return;
> > > >    }
> > > 
> > > this code is part of IsConceptualParent() except return statement but you
> > > can deal with it later. Also that allows you to not get parentRole twice
> > 
> > but the return is delt with by if (parentRole != role_grouping) right so
> > that code can just get rm'd right?
> 
> you deal with it below, like additional check for outlineitem role.

I'm still not sure about  what to do here.

> > > > +  if (IsConceptualParent(aRole, parent))
> > > > +      mParent = parent;
> > > > +  if (parentRole != nsIAccessibleRole::ROLE_GROUPING)
> > > > +    return;
> > > 
> > > add a check for role == outline_item, it appears this role is what all code
> > > for.
> > 
> > you mean
> > 
> > if (role != role_grouping || role != role_outlineitem)
> > return
> 
> yeah, that should be what we want, right?

actually I not so sure since the code following seems to look at the  accessible before the parent, so maybe the parents role doesn't matter in this case?

> > > @@ +203,5 @@
> > > > +          if ((parentRole == nsIAccessibleRole::ROLE_PARENT_MENUITEM ||
> > > > +                parentRole == nsIAccessibleRole::ROLE_POPUP_MENU ||
> > > > +                parentRole == nsIAccessibleRole::ROLE_BUTTONMENU ||
> > > > +                parentRole == nsIAccessibleRole::ROLE_MENUPOPUP) &&
> > > > +              aRole == nsIAccessibleRole::ROLE_MENUITEM)
> > > 
> > > use BaseRole instead
> > 
> > BaseRole() only deals with converting menuitem roles to menuitem no?
> 
> correct

so how does it help here?
Comment 21 alexander :surkov 2011-07-23 06:43:55 PDT
(In reply to comment #20)

> > you deal with it below, like additional check for outlineitem role.
> 
> I'm still not sure about  what to do here.

just remove this code

> 
> > > > > +  if (IsConceptualParent(aRole, parent))
> > > > > +      mParent = parent;
> > > > > +  if (parentRole != nsIAccessibleRole::ROLE_GROUPING)
> > > > > +    return;
> > > > 
> > > > add a check for role == outline_item, it appears this role is what all code
> > > > for.
> > > 
> > > you mean
> > > 
> > > if (role != role_grouping || role != role_outlineitem)
> > > return
> > 
> > yeah, that should be what we want, right?
> 
> actually I not so sure since the code following seems to look at the 
> accessible before the parent, so maybe the parents role doesn't matter in
> this case?

matters, because grouping roles are used to introduce the hierarchy

> > > > use BaseRole instead
> > > 
> > > BaseRole() only deals with converting menuitem roles to menuitem no?
> > 
> > correct
> 
> so how does it help here?

otherwise you don't handle checkbox and radio menutiems
Comment 22 Trevor Saunders (:tbsaunde) 2011-07-24 06:43:29 PDT
(In reply to comment #21)
> (In reply to comment #20)
> 
> > > you deal with it below, like additional check for outlineitem role.
> > 
> > I'm still not sure about  what to do here.
> 
> just remove this code
> 
> > 
> > > > > > +  if (IsConceptualParent(aRole, parent))
> > > > > > +      mParent = parent;
> > > > > > +  if (parentRole != nsIAccessibleRole::ROLE_GROUPING)
> > > > > > +    return;
> > > > > 
> > > > > add a check for role == outline_item, it appears this role is what all code
> > > > > for.
> > > > 
> > > > you mean
> > > > 
> > > > if (role != role_grouping || role != role_outlineitem)
> > > > return
> > > 
> > > yeah, that should be what we want, right?
> > 
> > actually I not so sure since the code following seems to look at the 
> > accessible before the parent, so maybe the parents role doesn't matter in
> > this case?
> 
> matters, because grouping roles are used to introduce the hierarchy
> 
> > > > > use BaseRole instead
> > > > 
> > > > BaseRole() only deals with converting menuitem roles to menuitem no?
> > > 
> > > correct
> > 
> > so how does it help here?
> 
> otherwise you don't handle checkbox and radio menutiems

well, CreateAccGroupInfo() calls new AccGroupInfo(blah, baseRole(role)); so I think unless you construct a AccGroupInfo yourself that case should already be handled, but I gues I can add if you want.
Comment 23 Trevor Saunders (:tbsaunde) 2011-07-24 06:51:25 PDT
Created attachment 548004 [details] [diff] [review]
patch
Comment 24 alexander :surkov 2011-07-24 07:05:18 PDT
(In reply to comment #22)

> > > so how does it help here?
> > 
> > otherwise you don't handle checkbox and radio menutiems
> 
> well, CreateAccGroupInfo() calls new AccGroupInfo(blah, baseRole(role)); so
> I think unless you construct a AccGroupInfo yourself that case should
> already be handled, but I gues I can add if you want.

agree
Comment 25 alexander :surkov 2011-07-24 07:15:36 PDT
Comment on attachment 548004 [details] [diff] [review]
patch

Review of attachment 548004 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/AccGroupInfo.cpp
@@ +139,5 @@
>        parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) {
>      mParent = parent;
>      return;
>    }
>  

again you don't need this code because
1) mParent = parent; can be handled by IsConceptualParent
2) return; can be handled by 
if (parentRole != ROLE_GROUPING || role != ROLE_OUTLINEITEM)

following this you never run this code for menuitems and other fellows

@@ +176,5 @@
> +bool
> +AccGroupInfo::IsConceptualParent(PRUint32 aRole, nsAccessible* aParent)
> +{
> +  PRUint32 parentRole = aParent->Role();
> +

btw, any specific reason to call parent->Role() twice, you can pass it as argument here

@@ +187,5 @@
> +    return true;
> +  if ((parentRole == nsIAccessibleRole::ROLE_TABLE ||
> +       parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) &&
> +      aRole == nsIAccessibleRole::ROLE_ROW)
> +    return true;

move this one before (parentRole == ROLE_ROW)

@@ +202,5 @@
> +      aRole == nsIAccessibleRole::ROLE_PAGETAB)
> +    return true;
> +       if ((parentRole == nsIAccessibleRole::ROLE_POPUP_MENU ||
> +       parentRole == nsIAccessibleRole::ROLE_MENUPOPUP) &&
> +      aRole == nsIAccessibleRole::ROLE_MENUITEM)

wrong indentation

::: accessible/src/base/AccGroupInfo.h
@@ +89,5 @@
>    }
>  
> +  /**
> +   * check that it makes sense for the given accessible to be the conceptual
> +   * parent of an accessible of the given role.

check -> Check

maybe simplify it: Return true if

::: accessible/tests/mochitest/relations/test_general.xul
@@ +69,5 @@
>        testRelation("treeitem4", RELATION_NODE_CHILD_OF, "tree");
>        testRelation("treeitem5", RELATION_NODE_CHILD_OF, "treeitem4");
>  
> +      // no relation node child of for accessible contained in an unexpected
> +      // parent

node child of -> node_child_of, that's easier to read

@@ +151,5 @@
>          Mozilla Bug 475298
>        </a><br/>
> +      <a target="_blank"
> +         href="https://bugzilla.mozilla.org/show_bug.cgi?id=673389"
> +         title="node child of on an item not in a proper container"

same too
Comment 26 Trevor Saunders (:tbsaunde) 2011-07-25 00:14:15 PDT
(In reply to comment #25)
> Comment on attachment 548004 [details] [diff] [review] [review]
> patch
> 
> Review of attachment 548004 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/AccGroupInfo.cpp
> @@ +139,5 @@
> >        parentRole == nsIAccessibleRole::ROLE_TREE_TABLE) {
> >      mParent = parent;
> >      return;
> >    }
> >  
> 
> again you don't need this code because
> 1) mParent = parent; can be handled by IsConceptualParent
> 2) return; can be handled by 
> if (parentRole != ROLE_GROUPING || role != ROLE_OUTLINEITEM)
> 
> following this you never run this code for menuitems and other fellows

oops, thought I  had rm'd will fix.
Comment 27 Trevor Saunders (:tbsaunde) 2011-07-25 01:57:13 PDT
Created attachment 548128 [details] [diff] [review]
bug 673389

patch
Comment 28 alexander :surkov 2011-07-25 02:34:57 PDT
Comment on attachment 548128 [details] [diff] [review]
bug 673389

Review of attachment 548128 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/AccGroupInfo.cpp
@@ +133,3 @@
>    PRUint32 parentRole = parent->Role();
> +  if (IsConceptualParent(aRole, parentRole))
> +      mParent = parent;

nit: wrong indentation

@@ +140,5 @@
>  
>    // In the case of ARIA tree, a tree can be arranged by using ARIA groups
>    // to organize levels. In this case the parent of the tree item will be
>    // a group and the previous treeitem of that should be the tree item
> +  // parent.

move this comment before the check

also "In the case of ARIA tree (not ARIA treegrid)"

@@ +190,5 @@
> +      aRole == nsIAccessibleRole::ROLE_PAGETAB)
> +    return true;
> +       if ((aParentRole == nsIAccessibleRole::ROLE_POPUP_MENU ||
> +       aParentRole == nsIAccessibleRole::ROLE_MENUPOPUP) &&
> +      aRole == nsIAccessibleRole::ROLE_MENUITEM)

again: wrong indentation of three lines above

::: accessible/src/base/AccGroupInfo.h
@@ +90,5 @@
>  
> +  /**
> +   * return true  if it makes sense for the given accessible to be the
> +   * conceptual parent of an accessible of the given role.
> +   */

Maybe: Return true if the given parent role is conceptual parent of the given role.
Comment 29 Trevor Saunders (:tbsaunde) 2011-07-25 02:58:46 PDT
Created attachment 548131 [details] [diff] [review]
bug 673389
Comment 30 alexander :surkov 2011-07-25 03:01:47 PDT
Comment on attachment 548131 [details] [diff] [review]
bug 673389

r=me
Comment 31 alexander :surkov 2011-08-03 20:59:44 PDT
landed - http://hg.mozilla.org/mozilla-central/rev/c19fbc9ec649

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