Closed Bug 673389 Opened 13 years ago Closed 13 years ago

AccGroupInfo::ConceptualParent() shouldn't consider random parent accessibles to be a parent

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(1 file, 10 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #547664 - Flags: review?(surkov.alexander)
it doesn't look like this patch against trunk
Attached patch patch (obsolete) — Splinter Review
odd, is this better?
Attachment #547664 - Attachment is obsolete: true
Attachment #547664 - Flags: review?(surkov.alexander)
Attachment #547671 - Flags: review?(surkov.alexander)
(In reply to comment #3)
> Created attachment 547671 [details] [diff] [review] [review]
> patch
> 
> odd, is this better?

it's the same
Attached patch patch (obsolete) — Splinter Review
I see what happend now.
Attachment #547671 - Attachment is obsolete: true
Attachment #547671 - Flags: review?(surkov.alexander)
Attachment #547679 - Flags: review?(surkov.alexander)
Comment on attachment 547679 [details] [diff] [review]
patch

the code below this check is executed for group role only
Attachment #547679 - Flags: review?(surkov.alexander) → review-
Attached patch patch2 (obsolete) — Splinter Review
does this make more sense?
Attachment #547681 - Flags: review?(surkov.alexander)
(In reply to comment #7)
> Created attachment 547681 [details] [diff] [review] [review]
> patch2
> 
> does this make more sense?

it's the same
Attached patch patch3 (obsolete) — Splinter Review
add tests and commit the change
Attachment #547679 - Attachment is obsolete: true
Attachment #547681 - Attachment is obsolete: true
Attachment #547681 - Flags: review?(surkov.alexander)
Attachment #547686 - Flags: review?(surkov.alexander)
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
Attachment #547686 - Flags: review?(surkov.alexander) → review-
Attached patch patch4 (obsolete) — Splinter Review
address comments
Attachment #547686 - Attachment is obsolete: true
Attachment #547705 - Flags: review?(surkov.alexander)
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
Attachment #547705 - Flags: review?(surkov.alexander)
(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.
Attached patch patch (obsolete) — Splinter Review
rebase to trunk from on top of 641838
Attachment #547705 - Attachment is obsolete: true
Attachment #547898 - Flags: review?(surkov.alexander)
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"
Attachment #547898 - Flags: review?(surkov.alexander)
Attached patch patch (obsolete) — Splinter Review
Attachment #547898 - Attachment is obsolete: true
Attachment #547916 - Flags: review?(surkov.alexander)
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
Attachment #547916 - Flags: review?(surkov.alexander)
(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?
(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
> > > ::: 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?
(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
(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.
Attached patch patch (obsolete) — Splinter Review
Attachment #547916 - Attachment is obsolete: true
Attachment #548004 - Flags: review?(surkov.alexander)
(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 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
Attachment #548004 - Flags: review?(surkov.alexander)
(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.
Attached patch bug 673389 (obsolete) — Splinter Review
patch
Attachment #548004 - Attachment is obsolete: true
Attachment #548128 - Flags: review?(surkov.alexander)
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.
Attachment #548128 - Flags: review?(surkov.alexander)
Attached patch bug 673389Splinter Review
Attachment #548128 - Attachment is obsolete: true
Attachment #548131 - Flags: review?(surkov.alexander)
Comment on attachment 548131 [details] [diff] [review]
bug 673389

r=me
Attachment #548131 - Flags: review?(surkov.alexander) → review+
landed - http://hg.mozilla.org/mozilla-central/rev/c19fbc9ec649
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.